1870240 - [COLRv1] Font with PaintColrGlyph cycle gets eagerly rejected

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Dominik Röttsches (drott)]

Attachment: cycle.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36

Steps to reproduce:

Display glyphs from a COLRv1 test font that, in another glyph, has a PaintColrGlyph cycle. Open index.html from the attached zip file.

(Found while testing coincident color stops and angles, further investigating issues like in issue 1817826).

Actual results:

No glyphs are shown, console complains about parsing errors with the font, sanitizer rejects font.

downloadable font: COLR: Cycle detected in PaintColrGlyph (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: COLR: Failed to parse referenced color glyph 176 (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: COLR: Failed to parse referenced color glyph 177 (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: COLR: Failed to parse paint for base glyph ID 176 (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: COLR: Failed to parse base glyph list (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: COLR: Failed to parse table (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf
downloadable font: rejected by sanitizer (font-family: "ctest" style:normal weight:400 stretch:100 src index:0) source: file:///Users/drott/dev/ff_colr/test_glyphs-glyf_colr_1.ttf

Expected results:

The font should be accepted, the two glyphs of the test page should be drawn. Paint cycles should be rejected on a per glyph basis.

A PaintColrGlyph cycle is a problem that is specific to a particular set of glyphs involved in creating the cycle.

The COLRv1 spec talks about how the remainder of the graph should be drawn if possible:
https://learn.microsoft.com/en-us/typography/opentype/spec/colr#color-glyphs-as-a-directed-acyclic-graph:~:text=If%20the%20graph,formed%20and%20valid.

This part of the spec can perhaps be read as a tendency of the spec being in favor of trying to not only render the remainder of the same glyph, but also, not rejecting the whole font based on a PaintCycle in a set of glyphs.

Background: The test font we're using for testing Chrome's implementation recently received two test glyphs for testing paint cycles. https://github.com/googlefonts/color-fonts/pull/122
If FireFox rejects this font as a whole, the test font can no longer be used as a whole, but a separate version would need to be made to test paint cycles.

I'd be in favor of seeing a change in FF that rejects PaintCycles only on a per glyph basis.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Text and Fonts' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Jonathan Kew [:jfkthame]]

This is being rejected by the OTS COLR table validation here and here, isn't it? So if we want to change the behavior, that's the place to do it.

Indeed, I see we (briefly) discussed this back in https://github.com/khaledhosny/ots/commit/4e1d51e039ed92246da7e7d6ee775bab23c3d099.

Comment 3

[Jonathan Kew [:jfkthame]]

We landed a patch for this in https://github.com/khaledhosny/ots/pull/272, so all we need to do here is to vendor the latest upstream code.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1870240 - patch 1 - Vendor latest version of OTS code. r=#gfx-reviewers

Generated by running

./mach vendor --patch-mode none gfx/ots/moz.yaml

and committing the resulting changes.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1870240 - patch 2 - Remove obsolete OTS patch file ots-1850314.patch. r=#gfx-reviewers

This was merged upstream in https://github.com/khaledhosny/ots/pull/266.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1870240 - patch 3 - Re-apply local OTS patches. r=#gfx-reviewers

Generated by running

./mach vendor --patch-mode only gfx/ots/moz.yaml

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1870240 - patch 4 - Add a simple reftest to check that the test font loads successfully. r=#gfx-reviewers

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84df086ee5f6
patch 1 - Vendor latest version of OTS code. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/2025ae0d3820
patch 2 - Remove obsolete OTS patch file ots-1850314.patch. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/1734d4af3e8f
patch 3 - Re-apply local OTS patches. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/bf25285b1d73
patch 4 - Add a simple reftest to check that the test font loads successfully. r=gfx-reviewers,lsalzman

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/84df086ee5f6
https://hg.mozilla.org/mozilla-central/rev/2025ae0d3820
https://hg.mozilla.org/mozilla-central/rev/1734d4af3e8f
https://hg.mozilla.org/mozilla-central/rev/bf25285b1d73