1405148 - Firefox 57.0b3 does not render small caps with the -apple-system font (using font-feature-settings or font-variant-caps)

Status: RESOLVED FIXED

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

Description

[David Cornu]

Attachment: 56.0 vs 57.0b3

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

Running Firefox Developer Edition 57.0b3 (64-bit) on macOS 10.12.6 (16G29).

Rendered an element using `font-feature-settings: 'smcp'`behind an `@supports`

Code used: https://jsbin.com/bagarihusi/1/edit?html,css,output




Actual results:

- In version 57.0b3, the `<h1>` will be rendered in lowercase
- In version 56.0, the `<h1>` will be rendered in small caps


Expected results:

Because of the `@supports` directive, the browser should have either skipped it altogether due to missing support for `font-feature-settings` or rendered the text correctly in small caps.

Comment 1

[Jonathan Kew [:jfkthame]]

AIUI, @supports only tests whether the CSS property is supported, but not whether the particular font involved (which is not known at the time @supports is evaluated, and may be a mixture of fonts even for a single element if fallback comes into play) will implement a specific feature tag.

As of Firefox 57, the -apple-system font (the "hidden" fonts .SFNS Text or .SFNS Display, depending on size) is rendered via Core Text rather than HarfBuzz, in order to get proper support for the (Apple-only) version is uses for its kerning. But 'smcp' is not supported via that shaping back-end.

In general, font-variant:small-caps is a better option, as it will apply the 'smcp' feature if available and synthesize "fake" small caps otherwise, unlike font-feature-settings:"smcp" which will simply leave the text as lowercase if unavailable/unsupported.

However, I notice that font-variant:small-caps also fails with the -apple-system font. That's a definite bug, which I think we can readily fix (although the simple fix will result in "fake" small-caps rather than the true small-cap glyphs from the font).

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Make gfxFontEntry::SupportsOpenTypeFeature a virtual method, and override on MacOS for AAT fonts that we'll shape with Core Text instead of HarfBuzz

This fixes the most glaring failure: that font-variant:small-caps has no effect on the -apple-system font, because we detect the presence of an OpenType 'smcp' feature, but then shape via Core Text where we don't support this. With this fix, font-variant:small-caps will produce synthetic small caps with -apple-system, just like in other fonts that lack any small-caps feature support. Note that attempting to control features with font-feature-settings still won't have any effect in the Core Text backend.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Map the 'smcp' OpenType feature to its AAT feature-selector equivalent when shaping via Core Text

For better small-caps rendering, we can map the OpenType 'smcp' tag to the appropriate AAT feature type/selector combination, and apply this when shaping with Core Text. This gives us proper small-caps in the -apple-system font (whether specified with font-variant:small-caps or using the low-level font-feature-settings property).

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - Use Core Text API to check for presence of small-caps feature in AAT fonts

To similarly support true small-caps in 'morx'-based AAT fonts like Hoefler Text, we need to check for the presence of the feature using Core Text APIs rather than just relying on the OpenType GSUB table.

Comment 5

[Jonathan Kew [:jfkthame]]

FTR, this is a result of bug 1394236, which switched the shaping of the -apple-system from HarfBuzz to Core Text (in order to support its kern table). Prior to that, we shaped this font with HarfBuzz and hence supported its GSUB 'smcp' feature (but failed to kern it).

The patches here will result in a "best of both worlds" situation where we maintain the correct kerning, and also support small-caps.

Comment 6

[Astley Chen (inactive)]

Jonathan, it seems you are not fixing small caps issue using font-feature-settings:'smcp' but a more general practice using font-variant-caps. Would you rephrase the bug summary or create a new bug as the follow-on?

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #6)
> Jonathan, it seems you are not fixing small caps issue using
> font-feature-settings:'smcp' but a more general practice using
> font-variant-caps. Would you rephrase the bug summary or create a new bug as
> the follow-on?

The patches here fix the issue for small-caps whether they're specified using font-variant-caps or font-feature-settings.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2671b1108d73
part 1 - Make gfxFontEntry::SupportsOpenTypeFeature a virtual method, and override on MacOS for AAT fonts that we'll shape with Core Text instead of HarfBuzz. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec596adde75
part 2 - Map the 'smcp' OpenType feature to its AAT feature-selector equivalent when shaping via Core Text. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/73b85fa99b0c
part 3 - Use Core Text API to check for presence of small-caps feature in AAT fonts. r=jrmuizel

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2671b1108d73
https://hg.mozilla.org/mozilla-central/rev/5ec596adde75
https://hg.mozilla.org/mozilla-central/rev/73b85fa99b0c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for backport to 57 or can it ride the 58 train?

Comment 11

[Jonathan Kew [:jfkthame]]

A couple of weeks ago, I'd have said yes, let's backport to avoid shipping a regression. But from my understanding, 57 is fairly locked-down at this point and only really critical issues (potential chemspill-level) fixes will be uplifted, in which case I don't think this meets the bar.

(If I'm misunderstanding and you think we should still take this, I'd be fine with nominating it; but I don't want to bother release-drivers with uplift requests that will just be dismissed because they're not critical stability or high-impact regression issues.)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

It's a new regression in 57, so I think it at least warrants consideration. A good risk analysis would be helpful, though, since there's a good amount of added code.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8914691 [details] [diff] [review]
part 1 - Make gfxFontEntry::SupportsOpenTypeFeature a virtual method, and override on MacOS for AAT fonts that we'll shape with Core Text instead of HarfBuzz

Approval Request Comment (for the three patches as a set)

[Feature/Bug causing the regression]: bug 1394236

[User impact if declined]: any content that tries to use small-caps with the -apple-system font will just render lowercase instead

[Is this code covered by automated tests?]: the specific small-caps behavior is not explicitly tested, but overall this font code is exercised by all macOS tests

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: low risk, though not nonexistent

[Why is the change risky/not risky?]: This is macOS-only code, so other platforms are not put at risk. On macOS, new code to use the Core Text font-feature APIs is introduced; potentially, this could have bugs, or could expose us to underlying bugs in Core Text/macOS fonts. However, given that many of the standard system fonts on macOS are AAT-enabled and therefore go through this code path, it seems likely that any serious issues would have been detected pretty quickly; the gfxMacFont/gfxCoreTextShaper code is in constant use whenever the browser runs on macOS.

[String changes made/needed]: none

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> [Why is the change risky/not risky?]: This is macOS-only code, so other
> platforms are not put at risk. On macOS, new code to use the Core Text
> font-feature APIs is introduced; potentially, this could have bugs, or could
> expose us to underlying bugs in Core Text/macOS fonts. However, given that
> many of the standard system fonts on macOS are AAT-enabled and therefore go
> through this code path, it seems likely that any serious issues would have
> been detected pretty quickly; the gfxMacFont/gfxCoreTextShaper code is in
> constant use whenever the browser runs on macOS.

And having written that, we now get bug 1408078, which is a crash in the new code added here! :(

Fortunately, it's a trivial fix (I'll post a patch momentarily), but it means that if we do want to uplift this, we'll need that fix as well. So, a correction:

[List of other uplifts needed for the feature/fix]: bug 1408078

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914691 [details] [diff] [review]
part 1 - Make gfxFontEntry::SupportsOpenTypeFeature a virtual method, and override on MacOS for AAT fonts that we'll shape with Core Text instead of HarfBuzz

recent regression so taking it, will also a+ the patch in bug 1408078

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8914692 [details] [diff] [review]
part 2 - Map the 'smcp' OpenType feature to its AAT feature-selector equivalent when shaping via Core Text

Approval Request Comment

See comment 13 above, which was explicitly meant to address all 3 patches as a set, but there's no way (AFAIK) to set flags on all the patches at once. Hence requesting the approval-beta flag here as well to avoid confusion.

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8914693 [details] [diff] [review]
part 3 - Use Core Text API to check for presence of small-caps feature in AAT fonts

Approval Request Comment

See comment 13 above, which was explicitly meant to address all 3 patches as a set, but there's no way (AFAIK) to set flags on all the patches at once. Hence requesting the approval-beta flag here as well to avoid confusion.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c27448f0b21
https://hg.mozilla.org/releases/mozilla-beta/rev/225b27eb2930
https://hg.mozilla.org/releases/mozilla-beta/rev/c016aa440637

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> [Is this code covered by automated tests?]: the specific small-caps behavior
> is not explicitly tested, but overall this font code is exercised by all
> macOS tests
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Jonathan's assessment on manual testing needs and the fact that the area affected by this fix has automated coverage.