Bug 186804 – [Cocoa] Single characters don't get shaped in the fast text codepath

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Laurence Penney]

Created attachment 343053 [details]
A TrueType font with 2 variation axes and 2-dimensional FeatureVariations

FeatureVariations in variable fonts allow glyph substitution to happen based on location in the designspace, as specified by CSS font-variation-settings (and other methods of setting axis positions). The spec allows substitutions to be made in any "rectangular" region of the font's designspace, by means of sets of conditions. For example, a small rectangle within a 2-dimensional designspace can be defined by giving a min and max in axis 0 and a min and max in axis 1. In the language of the spec, this would be one ConditionSet containing two ConditionTables. From the spec:

https://docs.microsoft.com/en-us/typography/opentype/spec/chapter2#featurevariations-table
“For a given condition set, conditions are conjunctively related (boolean AND): all of the specified conditions must be met in order for the associated feature table substitution to be applied.”

As the spec states, for such multi-dimensional ConditionSets to resolve correctly, the ConditionTables must process the ConditionTables in a chain of AND relations. All of the ConditionTables need to resolve to true for the ConditionSet to resolve to true.

However, Safari seems to use a chain of OR relations, so that ConditionSets are resolveing to true too often.

This CodePen uses a test font that contains two glyphs: u and u.alt (a square looking like a missing glyph), to demonstrate the issue. When handled correctly, FeatureVariations in the font cause the "u" in parts of the 2-dimensional designspace to switch to "u.alt". The total region for the substitutions in this font are non-rectangular and non-contiguous, are are defined by multiple ConditionSets.

https://codepen.io/lorp/pen/RMmRVB

It works on Chrome, but on Safari we see no substitutions. See screengrabs and attached font.

[Laurence Penney]

*** Bug 186803 has been marked as a duplicate of this bug. ***

[Laurence Penney]

Created attachment 343054 [details]
2D FeatureVariations conditions in Chrome: good

[Laurence Penney]

Created attachment 343055 [details]
2D FeatureVariations conditions in Safari: bad

[Myles C. Maxfield]

Can you provide the .html file for your test? I'd like to see specifically what is going on.

[Radar WebKit Bug Importer]

<rdar://problem/41277621>

[Laurence Penney]

Created attachment 343149 [details]
HTML + CSS for a 2D grid representing 2 axes in the font, where FeatureVariations substitute the "u" glyph for "u.alt" in certain parts of the designspace

The JS simply offers a toggle for the 'rvrn' feature.

[Myles C. Maxfield]

(In reply to Laurence Penney from comment #6)
> Created attachment 343149 [details]
> HTML + CSS for a 2D grid representing 2 axes in the font, where
> FeatureVariations substitute the "u" glyph for "u.alt" in certain parts of
> the designspace
> 
> The JS simply offers a toggle for the 'rvrn' feature.

Oh, you already provided a CodePen earlier... sorry for asking you to attach an .html file :( :( :( :( I clearly didn't read the description well enough

[Myles C. Maxfield]

We don’t run shaping in our simple text codepath for single characters. This is because the simple text codepath is intended only for kerning and ligatures, both of which require multiple characters. If you change all the “u” characters to “uu” then it looks the right way.

If you use font-feature-settings, we will take the hint that “hey, something complicated is happening” and will use our complex text codepath instead, which does run shaping for single characters. So, if you make the style say “font-feature-settings: ‘XYZW’” (a feature the font doesn’t actually have) it will also look as expected.

When the content in the codepen says “rvrn 0” we are passing that down into CoreText, but it looks like CoreText performs the substitution even if rvrn is disabled.

[Myles C. Maxfield]

I've filed <rdar://problem/41311572> about disabling rvrn still continues to perform substitutions. We can use this Bugzilla bug about how single characters don't perform shaping.

[Myles C. Maxfield]

rvrn can't be disabled, so the fact that Chrome lets it be disabled shows a Chrome bug.

[Myles C. Maxfield]

... and by "can't be disabled" I mean "is a required feature"

[Myles C. Maxfield]

I got a second duplicate report for this.

[Myles C. Maxfield]

Created attachment 343284 [details]
Second duplicate bug report

[Laurence Penney]

Thanks Myles. A few months back I was trying to work out how 'rvrn' worked on other browsers. It turns out that Edge behaves similarly to Safari, performing 'rvrn' only on "non-simple" text. As in Safari, you can force execution by adding random font-feature-settings.

I was also surprised that Edge and Chrome allow 'rvrn' to be disabled using font-feature-settings, given that the spec states: "Application of the 'rvrn' feature is mandatory in implementations that support OpenType Font Variations whenever a variable font is in use." The opinion of Edge and Chrome representatives seems to be that since font-feature-settings is low-level, it should be disableable (set to 0 or off) just like other OpenType feature, even if applications should never expose such a switch to users. I found this surprising, and believe Apple made the correct choice here.

[Myles C. Maxfield]

*** Bug 203768 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 434271 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 434271 [details]
Patch

Marking patch as r- because it needs a test.

[Myles C. Maxfield]

Created attachment 434274 [details]
Patch

[Myles C. Maxfield]

Laurence Penney says that this test font is licensed under Apache 2, and therefore can be committed to the WebKit repo (as a test font).

[Myles C. Maxfield]

Apache 2.0

[Myles C. Maxfield]

Thank you so much, Lawrence!

[Myles C. Maxfield]

*Laurence

[Myles C. Maxfield]

Right, we already have abseil-cpp checked into the tree, and it is Apache 2.0. So we should be good to go.

[Myles C. Maxfield]

Created attachment 435318 [details]
WIP

[Myles C. Maxfield]

Created attachment 435320 [details]
Patch

[Myles C. Maxfield]

Created attachment 435327 [details]
Patch

[Myles C. Maxfield]

The patch is passing all the tests, but I'm doing additional performance testing before this is ready to be marked as r?.

[Myles C. Maxfield]

0.2% of the calls to the shaper end up calling it with a single glyph.

[Myles C. Maxfield]

Given that:
1. This patch is having us call into the shaper 0.2% more often
2. The new calls are being called with a workload of 1 glyph, which is a smaller load than any of the other times the shaper is being called
3. The sum total of all shaping is a tiny fraction of all the time spent loading pages in our benchmarks

... It seems extremely unlikely that this patch would have any effect on performance.

[Myles C. Maxfield]

Created attachment 435950 [details]
Patch

[Myles C. Maxfield]

Oh, I measured it wrong before. The real number is 26%, not 0.2%. So it looks like there's more performance investigation that needs to be done.

[Myles C. Maxfield]

I recorded all the shaping calls in our loading benchmark (https://bugs.webkit.org/show_bug.cgi?id=207243) and found that this patch causes an additional 45 milliseconds of time spent shaping, across 13 pages, for an average of an additional 3.5 milliseconds per page. This seems like it could definitely cause a significant (1-2%) regression in the test.

[Myles C. Maxfield]

It looks like, of the shaping calls called with just one glyph, only 5% actually did any work (changed the glyph or advance or whatever).

[Myles C. Maxfield]

Every font which is being asked to shape just one single glyph has shaping tables (morx, GPOS, or GSUB).

[Myles C. Maxfield]

Comment on attachment 435950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435950&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:-98
> -    if (!force && glyphBufferSize <= lastGlyphCount + 1)

Clearing out kCTFontShapeWithKerning if there's only one glyph will improve performance to an acceptable level. We might even be able to do better, though!

[Myles C. Maxfield]

rdar://82195405 will help too.

[Myles C. Maxfield]

Created attachment 436062 [details]
Improved performance

[Myles C. Maxfield]

Created attachment 436063 [details]
Improved performance

[EWS]

Committed r281378 (240793@main): <https://commits.webkit.org/240793@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436063 [details].

[Myles C. Maxfield]

*** Bug 229367 has been marked as a duplicate of this bug. ***