Read four lookup keys for what the font states, not for what the read assumed (#113)

src/Otl.php

@@ -2061,7 +2061,9 @@ private function _applyGSUBsingleSubst($lookupID, $subtable, $ptr, $currGlyph, $
 			$DeltaGlyphID = $this->reader->readInt16();
 			$this->reader->seek($CoverageOffset);
 			$glyphs = $this->_getCoverageGID();
-			$GlyphID = $glyphs[$GlyphPos] + $DeltaGlyphID;
+			// The modulo is how a font names a glyph below the one it covers, or above the end of the
+			// range: Chiron Hei HK's 'hist' reaches glyph 1688 with 15324 from glyph 51900
+			$GlyphID = ($glyphs[$GlyphPos] + $DeltaGlyphID) & 0xFFFF;
 		}
 		// Format 2:
 		elseif ($SubstFormat == 2) { // Specified output glyph indices
@@ -4616,6 +4618,23 @@ private function marksOutsideFilteringSet($marks, $set)
 		return $keep ? ' ' . implode('| ', $keep) : '';
 	}
 
+	/**
+	 * The marks a lookup naming a mark attachment class skips: every mark outside that class, which is
+	 * what the parser keeps MarkAttachmentType as.
+	 *
+	 * A font may name a class GDEF does not define - Carlito and NATS set the flag without a
+	 * MarkAttachClassDef table at all - and then no mark is in the class, so the lookup skips every one
+	 * of them.
+	 *
+	 * @param int $class The mark attachment class the lookup's flags name
+	 *
+	 * @return string Its glyphs, space-prefixed and "|"-separated
+	 */
+	private function marksOutsideAttachmentClass($class)
+	{
+		return isset($this->MarkAttachmentType[$class]) ? $this->MarkAttachmentType[$class] : $this->GlyphClassMarks;
+	}
+
 	/**
 	 * The characters a Lookup's flag says to skip over, as a set keyed by codepoint.
 	 *
@@ -4660,10 +4679,8 @@ private function buildGCOMignoreList($flag, $MarkFilteringSet)
 		// Flag & 0xFF?? = MarkAttachmentType
 		if ($flag & 0xFF00) {
 			// "a lookup must ignore any mark glyphs that are not in the specified mark attachment class"
-			// $this->MarkAttachmentType is already adjusted for this i.e. contains all Marks except those in the MarkAttachmentClassDef table
-			$MarkAttachmentType = $flag >> 8;
 			$ignoreflag = $flag;
-			$str = $this->MarkAttachmentType[$MarkAttachmentType];
+			$str = $this->marksOutsideAttachmentClass($flag >> 8);
 		}
 
 		// Flag & 0x0010 = UseMarkFilteringSet
@@ -4726,8 +4743,7 @@ private function _checkGCOMignore($flag, $glyph, $MarkFilteringSet)
 		// Flag & 0xFF?? = MarkAttachmentType
 		if ($flag & 0xFF00) {
 			// "a lookup must ignore any mark glyphs that are not in the specified mark attachment class"
-			// $this->MarkAttachmentType is already adjusted for this i.e. contains all Marks except those in the MarkAttachmentClassDef table
-			if (strpos($this->MarkAttachmentType[($flag >> 8)], $glyph)) {
+			if (strpos($this->marksOutsideAttachmentClass($flag >> 8), $glyph)) {
 				$ignore = true;
 			}
 		}

src/TTFontFile.php

@@ -1698,16 +1698,21 @@ private function readGSUBrules(array &$Lookup)
 					$glyphs = $this->_getCoverage(false);
 					for ($g = 0; $g < count($glyphs); $g++) {
 						$replace = [];
-						$substitute = [];
 						$replace[] = unicode_hex($this->glyphToChar[$glyphs[$g]][0]);
 						// Flag = Ignore
 						if ($this->_checkGSUBignore($Lookup[$i]['Flag'], $replace[0], $Lookup[$i]['MarkFilteringSet'])) {
 							continue;
 						}
 						if (isset($Lookup[$i]['Subtable'][$c]['DeltaGlyphID'])) { // Format 1
-							$substitute[] = unicode_hex($this->glyphToChar[($glyphs[$g] + $Lookup[$i]['Subtable'][$c]['DeltaGlyphID'])][0]);
+							// The modulo is how a font names a glyph below the one it covers: Cactus
+							// Classical Serif reaches its extended em dash with -7504 from glyph 504
+							$gid = ($glyphs[$g] + $Lookup[$i]['Subtable'][$c]['DeltaGlyphID']) & 0xFFFF;
 						} else { // Format 2
-							$substitute[] = unicode_hex($this->glyphToChar[($Lookup[$i]['Subtable'][$c]['Glyphs'][$g])][0]);
+							$gid = $Lookup[$i]['Subtable'][$c]['Glyphs'][$g];
+						}
+						$substitute = $this->substituteGlyph($gid);
+						if ($substitute === null) {
+							continue;
 						}
 						$Lookup[$i]['Subtable'][$c]['subs'][] = ['Replace' => $replace, 'substitute' => $substitute];
 					}
@@ -1717,7 +1722,6 @@ private function readGSUBrules(array &$Lookup)
 					$glyphs = $this->_getCoverage();
 					for ($g = 0; $g < count($glyphs); $g++) {
 						$replace = [];
-						$substitute = [];
 						$replace[] = $glyphs[$g];
 						// Flag = Ignore
 						if ($this->_checkGSUBignore($Lookup[$i]['Flag'], $replace[0], $Lookup[$i]['MarkFilteringSet'])) {
@@ -1738,7 +1742,6 @@ private function readGSUBrules(array &$Lookup)
 					$glyphs = $this->_getCoverage();
 					for ($g = 0; $g < count($glyphs); $g++) {
 						$replace = [];
-						$substitute = [];
 						$replace[] = $glyphs[$g];
 						// Flag = Ignore
 						if ($this->_checkGSUBignore($Lookup[$i]['Flag'], $replace[0], $Lookup[$i]['MarkFilteringSet'])) {
@@ -1758,7 +1761,6 @@ private function readGSUBrules(array &$Lookup)
 					for ($s = 0; $s < $LigSetCount; $s++) {
 						for ($g = 0; $g < $Lookup[$i]['Subtable'][$c]['LigSet'][$s]['LigCount']; $g++) {
 							$replace = [];
-							$substitute = [];
 							$replace[] = $glyphs[$s];
 							// Flag = Ignore
 							if ($this->_checkGSUBignore($Lookup[$i]['Flag'], $replace[0], $Lookup[$i]['MarkFilteringSet'])) {
@@ -1774,10 +1776,10 @@ private function readGSUBrules(array &$Lookup)
 								$replace[] = $rpl;
 							}
 							$gid = $Lookup[$i]['Subtable'][$c]['LigSet'][$s]['Ligature'][$g]['LigGlyph'];
-							if (!isset($this->glyphToChar[$gid][0])) {
+							$substitute = $this->substituteGlyph($gid);
+							if ($substitute === null) {
 								continue;
 							}
-							$substitute[] = unicode_hex($this->glyphToChar[$gid][0]);
 							$Lookup[$i]['Subtable'][$c]['subs'][] = ['Replace' => $replace, 'substitute' => $substitute, 'CompCount' => $Lookup[$i]['Subtable'][$c]['LigSet'][$s]['Ligature'][$g]['CompCount']];
 						}
 					}
@@ -1977,7 +1979,6 @@ private function readGSUBrules(array &$Lookup)
 					$Lookup[$i]['Subtable'][$c]['CoverageInputGlyphs'] = [implode("|", $glyphs)];
 					for ($g = 0; $g < count($glyphs); $g++) {
 						$replace = [];
-						$substitute = [];
 						$replace[] = $glyphs[$g];
 						// Flag = Ignore
 						if ($this->_checkGSUBignore($Lookup[$i]['Flag'], $replace[0], $Lookup[$i]['MarkFilteringSet'])) {
@@ -1986,11 +1987,10 @@ private function readGSUBrules(array &$Lookup)
 						if (!isset($Lookup[$i]['Subtable'][$c]['SubstituteGlyphID'][$g])) {
 							continue;
 						} // The substitutes must run parallel to the Coverage table; either an error in the font, or something has gone wrong
-						$gid = $Lookup[$i]['Subtable'][$c]['SubstituteGlyphID'][$g];
-						if (!isset($this->glyphToChar[$gid][0])) {
+						$substitute = $this->substituteGlyph($Lookup[$i]['Subtable'][$c]['SubstituteGlyphID'][$g]);
+						if ($substitute === null) {
 							continue;
 						}
-						$substitute[] = unicode_hex($this->glyphToChar[$gid][0]);
 						$Lookup[$i]['Subtable'][$c]['subs'][] = ['Replace' => $replace, 'substitute' => $substitute];
 					}
 					for ($b = 0; $b < $Lookup[$i]['Subtable'][$c]['BacktrackGlyphCount']; $b++) {
@@ -2433,7 +2433,11 @@ function _getGSUBarray(array $Lookup, $lul, $scripttag)
 
 								$inputGlyphs = [];
 
-								$inputGlyphs[0] = $Lookup[$i]['Subtable'][$c]['InputClasses'][$inputClass];
+								if (isset($Lookup[$i]['Subtable'][$c]['InputClasses'][$inputClass])) {
+									$inputGlyphs[0] = $Lookup[$i]['Subtable'][$c]['InputClasses'][$inputClass];
+								} else {
+									$inputGlyphs[0] = '';
+								}
 								if ($rule['InputGlyphCount'] > 1) {
 									//  NB starts at 1
 									for ($gcl = 1; $gcl < $rule['InputGlyphCount']; $gcl++) {
@@ -2873,8 +2877,7 @@ function _checkGSUBignore($flag, $glyph, $MarkFilteringSet)
 		// Flag & 0xFF?? = MarkAttachmentType
 		if ($flag & 0xFF00) {
 			// "a lookup must ignore any mark glyphs that are not in the specified mark attachment class"
-			// $this->MarkAttachmentType is already adjusted for this i.e. contains all Marks except those in the MarkAttachmentClassDef table
-			if (strpos($this->MarkAttachmentType[($flag >> 8)], $glyph)) {
+			if (strpos($this->marksOutsideAttachmentClass($flag >> 8), $glyph)) {
 				$ignore = true;
 			}
 		}
@@ -2938,6 +2941,23 @@ private function marksOutsideFilteringSet($marks, $set)
 		return $keep ? ' ' . implode('| ', $keep) : '';
 	}
 
+	/**
+	 * The marks a lookup naming a mark attachment class skips: every mark outside that class, which
+	 * is what _getGDEFtables() keeps MarkAttachmentType as.
+	 *
+	 * A font may name a class GDEF does not define - Carlito and NATS set the flag without a
+	 * MarkAttachClassDef table at all - and then no mark is in the class, so the lookup skips every
+	 * one of them.
+	 *
+	 * @param int $class The mark attachment class the lookup's flags name
+	 *
+	 * @return string Its glyphs, space-prefixed and "|"-separated
+	 */
+	private function marksOutsideAttachmentClass($class)
+	{
+		return isset($this->MarkAttachmentType[$class]) ? $this->MarkAttachmentType[$class] : $this->GlyphClassMarks;
+	}
+
 	/**
 	 * The glyphs a lookup's flags say to skip, as a pattern that matches a run of them.
 	 *
@@ -2961,10 +2981,8 @@ function _getGSUBignoreString($flag, $MarkFilteringSet)
 		// Flag & 0xFF?? = MarkAttachmentType
 		if ($flag & 0xFF00) {
 			// "a lookup must ignore any mark glyphs that are not in the specified mark attachment class"
-			// $this->MarkAttachmentType is already adjusted for this i.e. contains all Marks except those in the MarkAttachmentClassDef table
-			$MarkAttachmentType = $flag >> 8;
 			$ignoreflag = $flag;
-			$str = $this->MarkAttachmentType[$MarkAttachmentType];
+			$str = $this->marksOutsideAttachmentClass($flag >> 8);
 		}
 
 		// Flag & 0x0010 = UseMarkFilteringSet
@@ -3436,9 +3454,11 @@ private function readScriptsAndFeatures($scriptListOffset, $featureListOffset)
 				$byFirstLookup = [];
 				foreach ($featureIndices as $featureIndex) {
 					$feature = $features[$featureIndex];
-					// A feature that runs no lookups has nothing to be ordered by and nothing to do.
-					// The spec permits one; no font in the 183 installed carries one, which is why the
-					// GSUB reader went without this guard for years and the GPOS one grew it.
+					// A feature that runs no lookups has nothing to be ordered by and nothing to do. The
+					// spec permits one and fonts in the wild carry one - Sedan SC's 'smcp' lists no
+					// lookups at all - so dropping it is the whole of what is right to do with it: keying
+					// the row by the lookup it has not got put it under '', which ksort() then ordered
+					// ahead of every real lookup index.
 					if (isset($feature['LookupListIndex'][0])) {
 						$byFirstLookup[$feature['LookupListIndex'][0]] = $feature;
 					}
@@ -3606,6 +3626,26 @@ protected function multipleSubstitutes(array $sequence)
 		return $substitute;
 	}
 
+	/**
+	 * One replacement glyph, as the character the shaper names it by.
+	 *
+	 * Every glyph of a font read with useOTL has a character: the ones the cmap does not reach are
+	 * mapped into the Private Use Area. So the only glyph id without one is a glyph the font has not
+	 * got, which Single, Ligature, Alternate and Reverse Chaining Substitution can all name.
+	 *
+	 * @param int $gid The glyph the rule replaces its match with
+	 *
+	 * @return array|null It as hex in a list of one, or null to record nothing for this one
+	 */
+	private function substituteGlyph($gid)
+	{
+		if (!isset($this->glyphToChar[$gid][0])) {
+			return null;
+		}
+
+		return [unicode_hex($this->glyphToChar[$gid][0])];
+	}
+
 	/**
 	 * What an Alternate Substitution puts in place of the glyph it covers.
 	 *
@@ -3618,13 +3658,7 @@ protected function multipleSubstitutes(array $sequence)
 	 */
 	protected function alternateSubstitutes(array $alternateSet)
 	{
-		$gid = $alternateSet['SubstituteGlyphID'][0];
-
-		if (!isset($this->glyphToChar[$gid][0])) {
-			return null;
-		}
-
-		return [unicode_hex($this->glyphToChar[$gid][0])];
+		return $this->substituteGlyph($alternateSet['SubstituteGlyphID'][0]);
 	}
 
 	/**

tests/Mpdf/DeltaGlyphIdTest.php

@@ -0,0 +1,76 @@
+<?php
+
+namespace Mpdf;
+
+/**
+ * A Single Substitution Format 1 states its replacement as a number to add to the glyph id, and the
+ * spec adds it modulo 65536 - which is how a font names a glyph below the one it covers, or above
+ * the end of the range. Cactus Classical Serif reaches its extended em dash with -7504 from glyph
+ * 504, Noto Sans SignWriting its alternate forms with -28287 from glyphs 8 to 27, and the three
+ * Chiron families wrap both ways.
+ *
+ * Both halves of mPDF added without the modulo. The parser then read glyphToChar outside its keys
+ * and recorded the rule as substituting U+0000; the shaper read glyphIDtoUni past its end and drew
+ * nothing at all - `font-variant-alternates: historical-forms` over U+3127 in any of the three
+ * Chiron faces lost the character.
+ */
+class DeltaGlyphIdTest extends \Yoast\PHPUnitPolyfills\TestCases\TestCase
+{
+
+	/** U+0627 ARABIC LETTER ALEF, the character the font's one lookup covers */
+	const ALEF = 0x0627;
+
+	/** U+0628 ARABIC LETTER BEH, which the lookup does not cover */
+	const BEH = 0x0628;
+
+	/**
+	 * The glyph the wrap lands on. It is the only one in the font with no character of its own, so
+	 * the parser maps it into the Private Use Area.
+	 */
+	const ALEF_FINA = 0xE000;
+
+	/**
+	 * @param int[] $codepoints
+	 *
+	 * @return int[] the codepoints of the line as it is handed to the drawing code
+	 */
+	private function drawn($codepoints)
+	{
+		$html = '';
+		foreach ($codepoints as $codepoint) {
+			$html .= sprintf('&#x%04X;', $codepoint);
+		}
+
+		$mpdf = new TextRecordingMpdf([
+			'mode' => 'utf-8',
+			'fontDir' => [__DIR__ . '/../data/ttf'],
+			'fontdata' => ['gsub11wrap' => [
+				'R' => 'NotoSansArabic-GSUB11Wrap-Synthetic.ttf',
+				'useOTL' => 0xFF,
+			]],
+			'default_font' => 'gsub11wrap',
+		]);
+		$mpdf->WriteHTML('<p>' . $html . '</p>');
+
+		return array_values(unpack('N*', mb_convert_encoding($mpdf->drawnText[0], 'UTF-32BE', 'UTF-8')));
+	}
+
+	/**
+	 * The lookup covers glyph 32770 and adds 32767, so the sum is 65537 and the substitute is glyph
+	 * 1. Without the modulo the letter drew as nothing at all.
+	 */
+	public function testTheGlyphTheDeltaWrapsOntoIsDrawn()
+	{
+		$this->assertSame([self::ALEF_FINA], $this->drawn([self::ALEF]));
+	}
+
+	/**
+	 * Text is drawn in visual order, which for Arabic is the reverse of the order it is written in.
+	 * The letter the lookup does not cover is left where it is.
+	 */
+	public function testALetterTheLookupDoesNotCoverIsLeftAlone()
+	{
+		$this->assertSame([self::ALEF_FINA, self::BEH], $this->drawn([self::BEH, self::ALEF]));
+	}
+
+}

tests/Mpdf/MarkAttachmentTypeTest.php

@@ -0,0 +1,62 @@
+<?php
+
+namespace Mpdf;
+
+/**
+ * A lookup flag can name a mark attachment class, which means "skip every mark that is not in it".
+ * Which marks those are is GDEF's MarkAttachClassDef to say, and a font may set the flag without
+ * defining that table at all - Carlito and NATS both do. No mark is then in the named class, so a
+ * lookup carrying the flag skips all of them.
+ *
+ * mPDF read the class out of MarkAttachmentType, got null for a class the table never built, and
+ * skipped nothing. Carlito's 'ccmp' replaces i and j with their dotless forms before a mark above,
+ * and that rule fired - where HarfBuzz, reading the same flag, leaves the dot on.
+ */
+class MarkAttachmentTypeTest extends \Yoast\PHPUnitPolyfills\TestCases\TestCase
+{
+
+	/** U+0069 LATIN SMALL LETTER I, which the font's 'ccmp' covers */
+	const I = 0x0069;
+
+	/** U+0313 COMBINING COMMA ABOVE, one of the marks that rule looks ahead for */
+	const COMMA_ABOVE = 0x0313;
+
+	/**
+	 * Draw the characters in Carlito cut down to the letters its flagged 'ccmp' lookups cover and the
+	 * one mark, which keeps GDEF's lack of a MarkAttachClassDef along with the flag that names one.
+	 *
+	 * @param int[] $codepoints
+	 *
+	 * @return int[] the codepoints of the line as it is handed to the drawing code
+	 */
+	private function drawn($codepoints)
+	{
+		$html = '';
+		foreach ($codepoints as $codepoint) {
+			$html .= sprintf('&#x%04X;', $codepoint);
+		}
+
+		$mpdf = new TextRecordingMpdf([
+			'mode' => 'utf-8',
+			'fontDir' => [__DIR__ . '/../data/ttf'],
+			'fontdata' => ['markattachment' => [
+				'R' => 'Carlito-MarkAttachmentType-Subset.ttf',
+				'useOTL' => 0xFF,
+			]],
+			'default_font' => 'markattachment',
+		]);
+		$mpdf->WriteHTML('<p>' . $html . '</p>');
+
+		return array_values(unpack('N*', mb_convert_encoding($mpdf->drawnText[0], 'UTF-32BE', 'UTF-8')));
+	}
+
+	/**
+	 * The two characters draw as themselves: the lookup that would replace the i skips the mark its
+	 * lookahead needs, so it cannot match. `hb-shape` draws the same pair of glyphs.
+	 */
+	public function testALookupNamingAnUndefinedMarkAttachmentClassSkipsTheMarkItLooksAheadFor()
+	{
+		$this->assertSame([self::I, self::COMMA_ABOVE], $this->drawn([self::I, self::COMMA_ABOVE]));
+	}
+
+}