Do not drop the character after U+FFFE or U+FFFF in Font.prototype.encodeString#21538
Conversation
…codeString encodeString has the same surrogate-pair guard that encodeToXmlString had before mozilla#21526: `unicode > 0xd7ff && (unicode < 0xe000 || unicode > 0xfffd)`. That predicate is also true for U+FFFE and U+FFFF, which are single UTF-16 code units, not surrogate pairs. The extra `i++` then steps over the character that follows them, so it is silently dropped from the font-encoded output used when saving or printing a PDF. For example, encoding a string that is U+FFFF followed by "A", with a font that has a glyph for both, returns an encoded result ending in "A" on this branch but drops the "A" on master. Same fix as mozilla#21526: the correct test for a real surrogate pair is `unicode > 0xffff`, since codePointAt only returns a value at or above 0x10000 for an actual pair. This keeps existing behavior for real surrogate pairs (e.g. emoji) and the U+FFFD boundary, and only stops the character after U+FFFE/U+FFFF from being dropped. Added test/unit/fonts_spec.js, since Font.prototype.encodeString had no direct unit test. It calls the method on a minimal fake `this` (only toUnicode/cMap are read), since building a full Font requires a complete properties/font-file setup that this bug doesn't depend on.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21538 +/- ##
=======================================
Coverage 89.40% 89.40%
=======================================
Files 262 262
Lines 66739 66739
=======================================
+ Hits 59668 59670 +2
+ Misses 7071 7069 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
timvandermeij
left a comment
timvandermeij
left a comment
There was a problem hiding this comment.
Looks good, with passing browser tests. Thank you for your contribution!
|
/botio browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/a905df14e884ce2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/82ba69d71c84aa9/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/a905df14e884ce2/output.txt Total script time: 0.75 mins
Image differences available at: http://54.241.84.105:8877/a905df14e884ce2/reftest-analyzer.html#web=eq.log |
|
/botio-linux browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/f5b121af177a719/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/f5b121af177a719/output.txt Total script time: 19.39 mins
Image differences available at: http://54.241.84.105:8877/f5b121af177a719/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/82ba69d71c84aa9/output.txt Total script time: 25.53 mins
|
4 participants
Font.prototype.encodeStringhas the same surrogate-pair guard thatencodeToXmlStringhad before #21526:unicode > 0xd7ff && (unicode < 0xe000 || unicode > 0xfffd). That predicate is also true for U+FFFE and U+FFFF, which are single UTF-16 code units, not surrogate pairs. The extrai++then steps over the character that follows them, so it gets silently dropped from the font-encoded output used when saving or printing a PDF.Same fix as #21526: the correct test for a real surrogate pair is
unicode > 0xffff, sincecodePointAtonly returns a value at or above0x10000for an actual pair. This keeps the existing behavior for real surrogate pairs (emoji, supplementary-plane CJK, etc.) and the U+FFFD boundary, and only stops the character after U+FFFE/U+FFFF from being dropped.Font.prototype.encodeStringdidn't have a direct unit test, so I addedtest/unit/fonts_spec.js. It calls the method on a minimal fakethis(onlytoUnicode/cMapare read by this method), since building a fullFontneeds a complete properties/font-file setup that this bug doesn't depend on.Verified locally: reverting only the
fonts.jsfix (keeping the new tests) turns the new test red (the encoded output is missing the trailing character), confirming the test catches the bug; with the fix it's green. A fulltest/unitrun (skippingapi_spec/pdf_spec/the two specs that need downloaded reference PDFs, unavailable in my sandbox) is unaffected: same 8 pre-existing environment-only failures and 15 pending specs as an unmodified checkout, before and after this change.eslint/prettier --checkare clean on the touched files.