Skip to content

Conversation

@JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #474

This pull request focuses on improving the accuracy and robustness of the font table parsing logic, particularly around offset handling, and adds a new regression test for a previously reported issue. The most important changes include switching from ReadUInt16 to ReadOffset16 for certain font table offsets to better handle OpenType font specifications, fixing offset calculations when loading ligature glyphs, and introducing a test to prevent regressions for Issue #474.

Font Table Offset Handling Improvements:

  • Replaced ReadUInt16 with ReadOffset16 for ligatureCaretListOffset, markAttachClassDefOffset, and coverageOffset in GlyphDefinitionTable and LigatureCaretList to ensure correct parsing of OpenType offsets. [1] [2]
  • Fixed the calculation of ligature glyph offsets in LigatureCaretList.Load by adding the base offset to each ligature glyph offset, ensuring correct glyph data is loaded.
  • Simplified object instantiation in LigatureGlyph.Load by returning the constructed object directly.

Testing and Regression Prevention:

  • Added a new test Issues_474 to ensure that text layout with the ServiceNow font does not throw exceptions and renders correctly, preventing regressions for Issue Exception on FontCollection #474.
  • Registered a new test font resource ServiceNowWoff2 in TestFonts for use in the regression test.
  • Added a new reference output image for the test case.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical bug in ligature caret offset determination that was causing exceptions when parsing certain font files (Issue #474). The fix ensures that offsets within font tables are correctly calculated relative to their parent table's base offset, and improves semantic clarity by using ReadOffset16() for offset fields.

  • Changed offset reading from ReadUInt16() to ReadOffset16() for ligature caret list and mark attachment class definition offsets in GlyphDefinitionTable and coverage offset in LigatureCaretList
  • Fixed ligature glyph offset calculation in LigatureCaretList.Load() by adding the base offset to each ligature glyph offset
  • Simplified LigatureGlyph.Load() by returning the constructed object directly
  • Added regression test for Issue #474 with the ServiceNow font file

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/SixLabors.Fonts/Tables/AdvancedTypographic/GlyphDefinitionTable.cs Changed ReadUInt16() to ReadOffset16() for ligature caret list and mark attachment class definition offset fields
src/SixLabors.Fonts/Tables/AdvancedTypographic/LigatureCaretList.cs Changed coverage offset reading to use ReadOffset16(), fixed ligature glyph offset calculation by adding base offset, and reorganized object initialization
src/SixLabors.Fonts/Tables/AdvancedTypographic/LigatureGlyph.cs Simplified object instantiation by returning directly instead of storing in a temporary variable
tests/SixLabors.Fonts.Tests/TestFonts.cs Added ServiceNowWoff2 test font property
tests/SixLabors.Fonts.Tests/Issues/Issues_474.cs Added regression test that verifies text layout with ServiceNow font doesn't throw exceptions
tests/SixLabors.Fonts.Tests/Fonts/ServiceNow-Sans-Text-Bold-unmastered-1.1.woff Added test font file resource
tests/Images/ReferenceOutput/Test_Issue_474-.png Added reference output image for visual regression testing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@JimBobSquarePants JimBobSquarePants merged commit 3a21143 into main Nov 26, 2025
15 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/fix-474 branch November 26, 2025 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception on FontCollection

2 participants