Remove an HTML comment without leaving a space in its place

CHANGELOG-GRAVITYPDF.md

@@ -117,7 +117,7 @@ New features
 * **`:not()`, `:is()` and `:where()`.** Each takes a selector list whose selectors may have combinators, such as `p:not(.note, div > p)` or `:is(h2, h3) + p`. `:is()` and `:where()` leave out a selector they cannot read, as browsers do. `:not()` and `:is()` weigh as their most specific argument, and `:where()` as nothing. Rules using them were dropped before. [#538] [#628]
 * **Pseudo-classes that look at what follows an element.** `:last-child`, `:nth-last-child()`, `:only-child`, `:last-of-type`, `:nth-last-of-type()`, `:only-of-type` and `:empty` match in the flow, in tables, headers and footers, and positioned blocks. Rules using them were dropped before, so a document that has them changes. Set `'cssMode' => \Mpdf\CssMode::LEGACY` to keep dropping them. [#537] [#634]
   * `WriteHTML()` reads ahead through the HTML it is given to count each element's children. An element still open at the end of a call that leaves it open is not known in full, so these do not match its children in that call, and nor does `:not()` of them.
-  * `:empty` matches as in browsers: an element holding white space is not empty. In the default mode mPDF's CSS reader turns each HTML comment into a space, so an element holding only a comment is not empty either. A table cell holding only white space is empty, as mPDF strips it.
+  * `:empty` matches as in browsers: an element holding white space is not empty, and one holding only a comment is. A table cell holding only white space is empty, as mPDF strips it.
 
 Performance
 -----------
@@ -178,6 +178,9 @@ Bugfixes
   * A byte order mark at the start of a stylesheet, as `WriteHTML(file_get_contents('style.css'), HTMLParserMode::HEADER_CSS)` passes one on, no longer spoils the first rule.
   * An `@import` is loaded only from the stylesheet's own rules, not from a comment, a string or a block.
   * A `style` attribute is split the same way, so `style="font-family: 'a;b', serif; color: red"` applies both, and a value may run over several lines.
+* An HTML comment between two words drew a space: `Book<!-- x -->keeper` read "Book keeper", and `<p><!-- x --></p>` held a space. A comment now leaves nothing in its place, as in a browser and as `HTMLParserMode::HTML_BODY` already had it, in either `cssMode`. A document that relied on the space can put one next to the comment. [#636] [#637]
+  * In the default parser mode, a `<!--` inside a `<script>`, as in `var s = "<!--"`, drew part of the script and lost the text up to the end of the next comment.
+  * In `HTMLParserMode::HTML_BODY`, a `<script>` tag inside a comment lost the text up to the next script's end.
 * `td:nth-child()` and `th:nth-child()` counted grid columns, so a `colspan` or `rowspan` before a cell made the rule miss it and reach the cell after. They now count the cells of the row. `:first-child` on a `tr`, `td` or `th` now works, as `:nth-child(1)`. [#528] [#572]
 * Some colour values were read wrongly. [#552] [#570]
   * `#rgba` was read from the wrong digits: `#f008` drew `rgb(240, 8, 0)`. `#rrggbbaa` dropped its alpha. Both are now read with their alpha.
@@ -849,6 +852,8 @@ These changes do not change output.
 [#632]: https://github.com/GravityPDF/mpdf/issues/632
 [#633]: https://github.com/GravityPDF/mpdf/pull/633
 [#634]: https://github.com/GravityPDF/mpdf/pull/634
+[#636]: https://github.com/GravityPDF/mpdf/issues/636
+[#637]: https://github.com/GravityPDF/mpdf/pull/637
 [#635]: https://github.com/GravityPDF/mpdf/pull/635
 [#639]: https://github.com/GravityPDF/mpdf/pull/639
 [#638]: https://github.com/GravityPDF/mpdf/pull/638

src/Css/CommentParser.php

@@ -4,20 +4,30 @@
 
 class CommentParser
 {
+	/**
+	 * A <script> element, for a regular expression with the s and i modifiers
+	 */
+	const SCRIPT = '<script\b.*?<\/script\s*>';
+
 	/**
 	 * Remove mPDF-specific and general HTML comments from content.
 	 *
-	 * Removes <!--mpdf and mpdf--> markers and all HTML comments.
+	 * Removes the <!--mpdf and mpdf--> markers, keeping what is between them, then every HTML comment, leaving nothing
+	 * in its place, as a browser does. A <script> element's content is text, so a comment marker in it opens no comment.
 	 *
 	 * @param string $html HTML content to clean
 	 * @return string HTML with comments removed
 	 */
 	public function removeHtmlComments($html)
 	{
-		$html = preg_replace('/<!--mpdf/i', '', $html);
-		$html = preg_replace('/mpdf-->/i', '', $html);
-		$html = preg_replace('/<!--.*?-->/s', ' ', $html);
-		return $html;
+		$html = preg_replace('/<!--mpdf|mpdf-->/i', '', $html);
+
+		if (!preg_match('/<!--/', $html)) {
+			return $html;
+		}
+
+		// (*SKIP)(*F) steps over a script whole, so the search for a comment carries on after it
+		return preg_replace('/' . self::SCRIPT . '(*SKIP)(*F)|<!--.*?-->/si', '', $html);
 	}
 
 	/**

src/Mpdf.php

@@ -6,6 +6,7 @@
 use Mpdf\Config\FontVariables;
 use Mpdf\Conversion;
 use Mpdf\Css\Border;
+use Mpdf\Css\CommentParser;
 use Mpdf\Color\IccProfile;
 use Mpdf\Css\TextVars;
 use Mpdf\Fonts\Color\ColorFormats;
@@ -28256,15 +28257,9 @@ function AdjustHTML($html, $tabSpaces = 8)
 			}
 		}
 
-		// Remove javascript code from HTML (should not appear in the PDF file)
-		$html = preg_replace('/<script.*?<\/script>/is', '', $html);
-
-		// Remove special comments
-		$html = preg_replace('/<!--mpdf/i', '', $html);
-		$html = preg_replace('/mpdf-->/i', '', $html);
-
-		// Remove comments from HTML (should not appear in the PDF file)
-		$html = preg_replace('/<!--.*?-->/s', '', $html);
+		// Remove comments, then javascript code, from HTML (neither should appear in the PDF file)
+		$html = (new CommentParser())->removeHtmlComments($html);
+		$html = preg_replace('/' . CommentParser::SCRIPT . '/si', '', $html);
 
 		$html = preg_replace('/\f/', '', $html); // replace formfeed by nothing
 		$html = preg_replace('/\r/', '', $html); // replace carriage return by nothing

src/TracksOpenElements.php

@@ -324,8 +324,7 @@ private function readAhead(array $tokens, $floor, $standIn, $close)
 
 		foreach ($tokens as $i => $token) {
 			if ($i % 2 === 0) {
-				// White space is text, as browsers have it. Comments are gone by now, though the CSS reader leaves a space
-				// for each
+				// White space is text, as browsers have it. Comments are gone by now
 				if ($token !== '') {
 					$this->openElements[count($this->openElements) - 1]['empty'] = false;
 				}

tests/Mpdf/Css/CommentParserTest.php

@@ -0,0 +1,48 @@
+<?php
+
+namespace Mpdf\Css;
+
+use Yoast\PHPUnitPolyfills\TestCases\TestCase;
+
+/**
+ * Removing HTML comments from a document before it is read
+ */
+class CommentParserTest extends TestCase
+{
+
+	/**
+	 * Each comment goes with nothing left in its place, the mPDF markers go and leave what is between them, and the
+	 * content of a script is left as it is
+	 *
+	 * @dataProvider documents
+	 *
+	 * @param string $html
+	 * @param string $expected
+	 */
+	public function testRemoveHtmlComments($html, $expected)
+	{
+		$this->assertSame($expected, (new CommentParser())->removeHtmlComments($html));
+	}
+
+	/**
+	 * Documents with comments, and what is left of each
+	 *
+	 * @return array[] Each [html, expected]
+	 */
+	public function documents()
+	{
+		return [
+			'between two words' => ['a<!-- x -->b', 'ab'],
+			'an element holding only a comment is left empty' => ['<p><!-- x --></p>', '<p></p>'],
+			'over several lines, keeping the line breaks around it' => ["a\n<!--\nx\n-->\nb", "a\n\nb"],
+			'mPDF content' => ['a<!--mpdf <b>b</b> mpdf-->c', 'a <b>b</b> c'],
+			'mPDF markers in capitals' => ['a<!--MPDF b MPDF-->c', 'a b c'],
+			'a conditional comment' => ['a<!--[if mso]><p>x</p><![endif]-->b', 'ab'],
+			'a conditional comment that shows its content' => ['a<!--[if !mso]><!-->b<!--<![endif]-->c', 'abc'],
+			'a comment marker in a script' => ['<script>s = "<!--";</script>a<!-- x -->b', '<script>s = "<!--";</script>ab'],
+			'a script tag in a comment' => ['a<!-- <script> -->b<script>x</script>', 'ab<script>x</script>'],
+			'no comment' => ['<p>a b</p>', '<p>a b</p>'],
+		];
+	}
+
+}

tests/Mpdf/HtmlCommentTest.php

@@ -0,0 +1,97 @@
+<?php
+
+namespace Mpdf;
+
+use Yoast\PHPUnitPolyfills\TestCases\TestCase;
+
+/**
+ * An HTML comment is removed without leaving anything in its place, as a browser does, in both parser modes and both
+ * CSS modes: a document with a comment draws what it draws without it
+ */
+class HtmlCommentTest extends TestCase
+{
+
+	/**
+	 * The text drawn, and where each line of it is drawn, are those of the same document written as a browser reads it
+	 *
+	 * @dataProvider comments
+	 *
+	 * @param string $html
+	 * @param string $asRead The same document as a browser reads it, with no comment in it
+	 * @param int $mode The parser mode, an HTMLParserMode constant
+	 * @param string $cssMode
+	 */
+	public function testACommentLeavesNothingBehind($html, $asRead, $mode, $cssMode)
+	{
+		$drawn = $this->draw($html, $mode, $cssMode);
+
+		$this->assertNotEmpty($drawn[0]);
+		$this->assertSame($this->draw($asRead, $mode, $cssMode), $drawn);
+	}
+
+	/**
+	 * Each document with a comment, and the same document as a browser reads it, in each parser mode and CSS mode
+	 *
+	 * @return array[] Each [html, as read, parser mode, CSS mode]
+	 */
+	public function comments()
+	{
+		$documents = [
+			'between two words' => ['<p>a<!-- x -->b</p>', '<p>ab</p>'],
+			'at the start of a text run' => ['<p><b>a</b><!-- x -->b</p>', '<p><b>a</b>b</p>'],
+			'at the end of a text run' => ['<p>a<!-- x --><b>b</b></p>', '<p>a<b>b</b></p>'],
+			'between inline elements' => ['<p><b>a</b><!-- x --><i>b</i></p>', '<p><b>a</b><i>b</i></p>'],
+			'over several lines' => ["<p>a<!--\nx\n-->b</p>", '<p>ab</p>'],
+			'with line breaks around it' => ["<p>a\n<!-- x -->\nb</p>", "<p>a\nb</p>"],
+			'inside pre, between two words' => ["<pre>a<!-- x -->b\nc</pre>", "<pre>ab\nc</pre>"],
+			'inside pre, keeping the line breaks around it' => ["<pre>a\n<!-- x -->\nb</pre>", "<pre>a\n\nb</pre>"],
+			'inside a table cell' => ['<table><tr><td>a<!-- x -->b</td></tr></table>', '<table><tr><td>ab</td></tr></table>'],
+			'mPDF content, whose markers go and whose content stays' => ['<p>a<!--mpdf <b>b</b> mpdf-->c</p>', '<p>a <b>b</b> c</p>'],
+			'a comment inside mPDF content' => ['<p>a<!--mpdf b<!-- x -->c mpdf-->d</p>', '<p>a bc d</p>'],
+			'a conditional comment' => ['<p>a<!--[if mso]>x<![endif]-->b</p>', '<p>ab</p>'],
+			'a conditional comment that shows its content' => ['<p>a<!--[if !mso]><!-->b<!--<![endif]-->c</p>', '<p>abc</p>'],
+			'a comment marker in a script' => ['<script>var s = "<!--";</script><p>a<!-- x -->b</p>', '<p>ab</p>'],
+			'a script tag in a comment' => ['<p>a<!-- <script> -->b</p><script>x</script><p>c</p>', '<p>ab</p><p>c</p>'],
+		];
+
+		$cases = [];
+		foreach ($documents as $name => $document) {
+			foreach ($this->modes() as $modeName => $mode) {
+				$cases[$name . ', ' . $modeName] = array_merge($document, $mode);
+			}
+		}
+
+		return $cases;
+	}
+
+	/**
+	 * Each parser mode that reads a document, with each CSS mode
+	 *
+	 * @return array[] Each [parser mode, CSS mode]
+	 */
+	private function modes()
+	{
+		return [
+			'default mode, standard' => [HTMLParserMode::DEFAULT_MODE, CssMode::STANDARD],
+			'default mode, legacy' => [HTMLParserMode::DEFAULT_MODE, CssMode::LEGACY],
+			'body mode, standard' => [HTMLParserMode::HTML_BODY, CssMode::STANDARD],
+			'body mode, legacy' => [HTMLParserMode::HTML_BODY, CssMode::LEGACY],
+		];
+	}
+
+	/**
+	 * @param string $html
+	 * @param int $mode The parser mode, an HTMLParserMode constant
+	 * @param string $cssMode
+	 *
+	 * @return array[] The text of each line drawn, and the page, left and right edges and top of each
+	 */
+	private function draw($html, $mode, $cssMode)
+	{
+		$mpdf = new TextRecordingMpdf(['mode' => 'c', 'cssMode' => $cssMode]);
+		$mpdf->WriteHTML($html, $mode);
+
+		return [$mpdf->drawnText, $mpdf->drawnBoxes];
+	}
+
+}

tests/Mpdf/LookAheadSelectorTest.php

@@ -239,10 +239,10 @@ public function testTheKeptBlockUnwindsForEveryCase()
 	}
 
 	/**
-	 * :empty matches an element with no child elements and no text, as browsers have it: white space is text. A
-	 * comment is not, but in the default mode the CSS reader turns each comment into a space before the HTML is read,
-	 * so an element holding one is not empty; the body mode, which does not read CSS, drops comments outright. The
-	 * white space at the end of a table cell is stripped before the HTML is read, so a cell holding only that is empty
+	 * :empty matches an element with no child elements and no text, as browsers have it: white space is text, and a
+	 * comment is not, so an element holding only a comment is empty and one holding comments and white space is not.
+	 * The white space at the end of a table cell is stripped before the HTML is read, so a cell holding only that is
+	 * empty
 	 *
 	 * @dataProvider emptyElements
 	 *
@@ -257,8 +257,8 @@ public function testWhatCountsAsEmpty($element, $empty)
 	}
 
 	/**
-	 * HTML written in the body mode does not go through the CSS reader, so a comment is dropped and the element that
-	 * held only the comment is empty, as in a browser
+	 * HTML written in the body mode drops a comment too, so the element that held only the comment is empty, as in a
+	 * browser
 	 */
 	public function testAnElementHoldingOnlyACommentInTheBodyMode()
 	{
@@ -280,7 +280,7 @@ public function emptyElements()
 			'nothing in it' => ['<p></p>', true],
 			'a space' => ['<p> </p>', false],
 			'line breaks and tabs' => ["<p>\n\t\n</p>", false],
-			'a comment, which the CSS reader turns into a space' => ['<p><!-- nothing --></p>', false],
+			'a comment, which leaves nothing in its place' => ['<p><!-- nothing --></p>', true],
 			'comments and white space' => ['<p> <!-- one --> <!-- two --> </p>', false],
 			'a closed start tag' => ['<div />', true],
 			'a void element' => ['<hr />', true],

tests/Snapshots/HtmlCommentSnapshotTest.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Snapshots;
+
+/**
+ * HTML comments removed with nothing left in their place, as a browser removes them, and the mPDF comment markers
+ * removed with their content kept.
+ *
+ * @group snapshot
+ */
+class HtmlCommentSnapshotTest extends Snapshot
+{
+
+	/**
+	 * The fixture's name
+	 *
+	 * @return string
+	 */
+	public function getId()
+	{
+		return 'html-comment';
+	}
+
+	/**
+	 * Words split by a comment, each under a caption saying what should be seen
+	 */
+	public function generatePdf()
+	{
+		$html = '
+		<style>
+			body { font-size: 11pt; }
+			h1 { font-size: 15pt; margin: 0 0 2mm 0; }
+			h2 { margin: 5mm 0 1mm 0; font-size: 10pt; }
+			p.caption { margin: 0 0 2mm 0; font-size: 8pt; color: #606060; }
+			div.case { border: 0.2mm solid #999999; padding: 1mm 2mm; }
+			div.case p { margin: 1mm 0; }
+			pre { margin: 1mm 0; }
+			table { border-collapse: collapse; }
+			td { border: 0.2mm solid #999999; padding: 1mm 3mm; }
+		</style>
+
+		<h1>HTML comments</h1>
+
+		<h2>Between two words</h2>
+		<p class="caption">Reads "Bookkeeper", with no space.</p>
+		<div class="case"><p>Book<!-- a comment -->keeper</p></div>
+
+		<h2>Between inline elements</h2>
+		<p class="caption">Reads "Bookkeeper", with "Book" in bold and "keeper" in italics, and no space between them.</p>
+		<div class="case"><p><b>Book</b><!-- a comment --><i>keeper</i></p></div>
+
+		<h2>In a table cell</h2>
+		<p class="caption">The cell reads "Bookkeeper", with no space.</p>
+		<table><tr><td>Book<!-- a comment -->keeper</td></tr></table>
+
+		<h2>In preformatted text</h2>
+		<p class="caption">Three lines: "Bookkeeper" with no space, an empty line, and "keeper".</p>
+		<div class="case"><pre>Book<!-- a comment -->keeper
+<!-- a comment on its own line -->
+keeper</pre></div>
+
+		<h2>A conditional comment</h2>
+		<p class="caption">Reads "Bookkeeper", with no space and no "hidden".</p>
+		<div class="case"><p>Book<!--[if mso]>hidden<![endif]-->keeper</p></div>
+
+		<h2>A comment marker in a script</h2>
+		<p class="caption">Reads "Bookkeeper", with no space and none of the script.</p>
+		<script>var marker = "<!--";</script>
+		<div class="case"><p>Book<!-- a comment -->keeper</p></div>
+
+		<h2>mPDF content</h2>
+		<p class="caption">Reads "Shown only by mPDF, and Bookkeeper": the markers go and their content stays, and the comment inside it leaves no space.</p>
+		<div class="case"><!--mpdf <p>Shown only by mPDF, and Book<!-- a comment -->keeper</p> mpdf--></div>
+		';
+
+		$this->mpdf = $this->createMpdf();
+
+		$this->mpdf->WriteHTML($html);
+	}
+
+}