Text after a block in a table cell loses its OpenType positioning, and kerning can apply twice #872

Text straight after a block in a table cell loses its OpenType positioning. In some fonts the glyphs also come out in the wrong order. Separately, FC_KERNING stays set on $this->textvar, so later text can be kerned twice. Fix both in one PR, because fixing the first alone exposes the second in one more case.

Both predate #871. Line numbers below are at gravitypdf 5cb37461.

Bug 1: the line break takes the text's run

<table style="font-family: xbriyaz"><tr><td><div>Block</div>&#x628;&#x650;&#x633;&#x652;&#x645;&#x650;</td></tr></table>

In WriteHTML()'s text-node branch:

1. src/Mpdf.php:14931 sets $this->OTLdata from applyOTL() for the text node.
2. src/Mpdf.php:15001 calls _saveCellTextBuffer("\n") before the text is saved. That function moves any run into the chunk it saves and empties it, so the "\n" gets the text's run.
3. GetStringWidth() just below measures the text with no run, so its width is wrong.
4. The text is saved with no run. printbuffer() builds a basic one from the code points, with no GPOS and no real group.

The chunks in <td><div>Blk</div>AVAVA To Wa</td> are [Blk run=3][NL run=11 gpos=6][T run=0 gpos=0 FC_KERNING]: the newline holds the whole run.

All of these are needed to reach it: tableLevel and tdbegin are set, it isn't the specialcontent form branch, the text is non-empty after the ltrim, blockjustfinished is true, the cell's s is above 0 (the block ended with text on its last line), and the font has useOTL.

Routes that lose the run: text after a div, p, h1 or ul, th cells, tables nested one or two levels, whitespace before the text, and text in a <span> after the block.

Routes that keep it: an empty block as first child, text then an empty block, text in its own div, a <br> after the block or at the end inside it, and a nested table before the text.

Testing this needs care: DejaVu hides the kerning loss. With the run gone, FC_KERNING makes the text fall back to the kern table, whose values match GPOS in DejaVu ([(A)63(V)63…] both ways). Use marks or another font, as the test below does. Poppins, Hebrew in DejaVu and Kannada samples have no GPOS in their runs, so they can't show a loss. useOTL: 0 on dejavusans doesn't turn OTL off.

Letter-spacing and kashida justification also read this run, so they're probably affected too. That hasn't been run.

Bug 2: FC_KERNING stays set

_saveTextBuffer() (src/Mpdf.php:16983) and _saveCellTextBuffer() (src/Mpdf.php:17046) write FC_KERNING back to $this->textvar when a run has no GPOS. It stays set, so later text with its own GPOS kerning is kerned twice if the font also has a kern table.

No table is needed:

<p style="font-family: dejavusans; font-kerning: normal">Blk <span>AVAVA To Wa</span></p>

This gives 126 and 338 instead of 63 and 169, with useKerning on. <td>Blk<br>AVAVA To Wa</td> does the same.

Today's bug 1 hides this in Blk<div>Blk</div>AV…. Fixing bug 1 alone takes that case from 63 to 126.

Fix

This was run on PHP 8.5 against 96966579, the first commit of #871. The chunk logs and probes above come from that commit, with gravitypdf before #871 giving the same loss. The full suite gave 13543 tests passing, 31 skipped, with no baseline compared. All 258 snapshots passed.

--- a/src/Mpdf.php
+++ b/src/Mpdf.php
@@ WriteHTML(), text-node branch, table cell @@
 							if ($this->blockjustfinished && $this->cell[$this->row][$this->col]['s'] > 0) {
+								// The line break is not this text, which keeps its own run and the kerning that goes with it
+								$otlData = $this->OTLdata;
+								$textvar = $this->textvar;
+								$this->OTLdata = [];
 								$this->_saveCellTextBuffer("\n");
+								$this->OTLdata = $otlData;
+								$this->textvar = $textvar;
@@ _saveTextBuffer() and _saveCellTextBuffer(), the same change in each @@
 		// e.g. Latin script when useOTL set as 0x80
+		$arr[8] = $this->textvar; // mPDF 5.7.1
 		if (isset($this->OTLtags['Plus']) && strpos($this->OTLtags['Plus'], 'kern') !== false && empty($this->OTLdata['GPOSinfo'])) {
-			$this->textvar = ($this->textvar | TextVars::FC_KERNING);
+			$arr[8] = ($arr[8] | TextVars::FC_KERNING);
 		}
-		$arr[8] = $this->textvar; // mPDF 5.7.1

With bug 1's fix alone, the $textvar save and restore is needed: without it DejaVu kerned twice. With bug 2's fix it should be unnecessary, so try removing it. That's inferred, not run.

With this in place, every after-block case in the probe matrix matched the text alone in its cell, and every double-kerning case probed was fixed.

Why it wasn't done in #871: this changes kerning output, including outside tables. Expect output changes for any document that hits bug 2.

Check when you start:

• If Draw an inline object that follows a U+FEFF that leaves no text #871 has merged, _saveTextBuffer() no longer resets OTLdata, and its save is one line ($arr[18] = empty($this->OTLdata) ? null : $this->OTLdata;). So the context above differs slightly.
• Draw an inline object that follows a U+FEFF that leaves no text #871 keeps the reset in _saveCellTextBuffer() only because of this "\n" save, with a comment saying so. Once the line break no longer gets the run, that reset should be redundant. Remove it and the comment, and match _saveTextBuffer(), if the suite agrees. The reset in the textarea branch and $this->OTLdata = null at the top of the text branch stay: MultiCell() branches on is_array($OTLdata).

Tests

Add this as tests/Mpdf/TextAfterBlockInCellTest.php. It has 8 cases, and 7 fail before the fix. The kerning row passes either way, because DejaVu hides that loss. Rename the private drawnText(), because PageStreams already has a method of that name, which this one overrides.

<?php

namespace Mpdf;

/**
 * The line break a table cell saves for text that follows a block must leave the text its own OTL run
 */
class TextAfterBlockInCellTest extends \Yoast\PHPUnitPolyfills\TestCases\TestCase
{

	use PageStreams;

	/**
	 * The operators that draw text in the first page of a document, without their positions
	 *
	 * @param string $html
	 *
	 * @return string[]
	 */
	private function drawnText($html)
	{
		$mpdf = $this->mpdf([
			'mode' => '',
			'useKerning' => true,
			'autoLangToFont' => false,
			'autoScriptToLang' => false,
		]);
		$mpdf->shrink_tables_to_fit = 1;
		$mpdf->WriteHTML($html);
		preg_match_all('/BT [\d.\-]+ [\d.\-]+ Td(.*?)ET/s', $mpdf->pages[1], $matches);
		$mpdf->cleanup();

		return $matches[1];
	}

	/**
	 * A cell of the given content in the given font
	 *
	 * @param string $font
	 * @param string $content
	 * @param string $attr
	 *
	 * @return string
	 */
	private function cell($font, $content, $attr = '')
	{
		return '<table ' . $attr . ' style="width:100%;font-kerning:normal;font-family:' . $font . '"><tr><td>' . $content . '</td></tr></table>';
	}

	/**
	 * Shaped text in each script that shows what the run carries
	 *
	 * @return array
	 */
	public function samples()
	{
		$arabic = '&#x628;&#x650;&#x633;&#x652;&#x645;&#x650; &#x627;&#x644;&#x644;&#x651;&#x64E;&#x647;&#x650;';

		return [
			'kerning, kern table and GPOS' => ['dejavusans', 'AVAVA To Wa', ''],
			'Latin marks' => ['dejavusans', 'x&#x323;y&#x301;q&#x323;', ''],
			'Arabic marks, rtl' => ['dejavusans', $arabic, 'dir="rtl"'],
			'Arabic glyphs with no Unicode of their own, ltr' => ['xbriyaz', $arabic, ''],
			'Arabic glyphs with no Unicode of their own, rtl' => ['xbriyaz', $arabic, 'dir="rtl"'],
			'Thai marks' => ['garuda', '&#xE17;&#xE35;&#xE48;&#xE19;&#xE35;&#xE48; &#xE19;&#xE49;&#xE33;', ''],
			'Devanagari' => ['freeserif', '&#x915;&#x94D;&#x937;&#x93F; &#x930;&#x94D;&#x915;&#x93F; &#x926;&#x941;', ''],
		];
	}

	/**
	 * The text after the block draws as the same text in a cell of its own
	 *
	 * @dataProvider samples
	 *
	 * @param string $font
	 * @param string $text
	 * @param string $attr
	 */
	public function testTextAfterABlockKeepsItsRun($font, $text, $attr)
	{
		$alone = $this->drawnText($this->cell($font, $text, $attr));
		$afterBlock = $this->drawnText($this->cell($font, '<div>Blk</div>' . $text, $attr));

		$this->assertCount(1, $alone);
		$this->assertCount(2, $afterBlock);
		$this->assertSame($alone[0], $afterBlock[1]);
	}

	/**
	 * The line break does not turn the kern table on for the text nodes that follow it, on top of their own GPOS
	 */
	public function testALaterTextNodeIsNotKernedTwice()
	{
		$kerned = 'AVAVA To Wa';
		$alone = $this->drawnText($this->cell('dejavusans', $kerned . '<span> ' . $kerned . '</span>'));
		$afterBlock = $this->drawnText($this->cell('dejavusans', '<div>Blk</div>' . $kerned . '<span> ' . $kerned . '</span>'));

		$this->assertSame($alone[1], $afterBlock[2]);
	}
}

Still to add:

• A test for bug 2 with no table, using the <p>Blk <span>… document above.
• A captioned snapshot, as no fixture covers this. Put each script in a two-column row, with the text alone on the left and after a block on the right, then rasterise it and look. This is how the bug was checked, and the unfixed LTR Arabic is plainly scrambled. Thai looked the same at 80 DPI, so don't rely on Thai to show it in the fixture.
• A CHANGELOG-GRAVITYPDF.md entry under Bugfixes, for both symptoms.

Run the whole suite and read any snapshot that changes rather than refreshing it. Only PHP 8.5 has been run. composer cs and PHPStan haven't been run on this patch.