Closed Bug 1447163 Opened 2 years ago Closed 2 years ago

Enable support for OpenType variation fonts (CSS font-variation-settings, font-optical-sizing)

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

We have rendering support for variation fonts on up-to-date releases of all the major platforms: Win10 Fall Creators Update; macOS Sierra & later; Linux with recent FreeType; Android (using in-tree FreeType, so not dependent on OS version).

CSS support includes the low-level font-variation-settings property and @font-face descriptor, and also the font-optical-sizing property.

We have not yet extended the other CSS properties (font-weight, font-stretch, font-style) to interact with variation fonts (bug 1436048, bug 1436061), but this can be handled as a future enhancement; it is not required for a usable MVP. (Note that Chrome is also shipping variation font support without this aspect.)
Keywords: dev-doc-needed
Here's the patch to flip the prefs (and update the devtools property db, and a few test expectations). Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=e57171e7f2c7fb896295452cdc6d5c8f863dc435 shows a bunch of intermittent oranges, but nothing that looks related to this feature.
Attachment #8960715 - Flags: review?(dbaron)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8960715 [details] [diff] [review]
Enable support for OpenType variation fonts

>diff --git a/devtools/shared/css/generated/properties-db.js b/devtools/shared/css/generated/properties-db.js

I assume these changes are autogenerated per the instructions at the top of the file?

>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js

> // Whether to preserve OpenType variation tables in fonts (bypassing OTS)
>-pref("gfx.downloadable_fonts.keep_variation_tables", false);
>+pref("gfx.downloadable_fonts.keep_variation_tables", true);

This is a little concerning.  Is this saying that we'll be passing tables through without running them through OTS?  Is there a plan to fix that?  Doesn't this mean passing them through to system libraries?

>diff --git a/testing/web-platform/meta/css/css-fonts/variations/variable-box-font.html.ini b/testing/web-platform/meta/css/css-fonts/variations/variable-box-font.html.ini
>+  prefs: [gfx.downloadable_fonts.otl_validation:false]

Is this actually needed for all three tests?  If so, is there a bug on upstream web-platform-tests to use a valid font?  Or is it intentionally invalid?

>+  expected:
>+    if (os == "win"): FAIL
>+    if (os == "mac"): FAIL

Do you know why these pass only on Linux, and fails on other platforms?
Flags: needinfo?(jfkthame)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #2)
> Comment on attachment 8960715 [details] [diff] [review]
> Enable support for OpenType variation fonts
> 
> >diff --git a/devtools/shared/css/generated/properties-db.js b/devtools/shared/css/generated/properties-db.js
> 
> I assume these changes are autogenerated per the instructions at the top of
> the file?

Yes.

> >diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
> 
> > // Whether to preserve OpenType variation tables in fonts (bypassing OTS)
> >-pref("gfx.downloadable_fonts.keep_variation_tables", false);
> >+pref("gfx.downloadable_fonts.keep_variation_tables", true);
> 
> This is a little concerning.  Is this saying that we'll be passing tables
> through without running them through OTS?  Is there a plan to fix that? 
> Doesn't this mean passing them through to system libraries?

Yes, it does. Currently, to the best of my knowledge this is what all the other UAs do; e.g. see https://chromium.googlesource.com/chromium/src/+/lkcr/third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp#107. The general view seems to be that the shapers/rasterizers are robust enough nowadays that this is OK.

I filed bug 1341088 about adding validation in OTS for these tables, as I think it would be a worthwhile added level of protection, but would prefer not to block enabling the fonts on that, especially given that everyone else is shipping support without such a layer.

> >diff --git a/testing/web-platform/meta/css/css-fonts/variations/variable-box-font.html.ini b/testing/web-platform/meta/css/css-fonts/variations/variable-box-font.html.ini
> >+  prefs: [gfx.downloadable_fonts.otl_validation:false]
> 
> Is this actually needed for all three tests?  If so, is there a bug on
> upstream web-platform-tests to use a valid font?  Or is it intentionally
> invalid?

The font is rejected because there's a variations-related extension to OpenType layout tables that the OTS code doesn't yet know about; I'm intending to fix that, but for now some (not all) variation fonts run into this issue.

(Note that we already ship release builds with this pref set to false, because there are too many fonts in the wild that have minor anomalies that OTS will flag, even though harfbuzz will handle them safely.)

> 
> >+  expected:
> >+    if (os == "win"): FAIL
> >+    if (os == "mac"): FAIL
> 
> Do you know why these pass only on Linux, and fails on other platforms?

We are not running sufficiently up-to-date versions of either macOS or Windows in CI for these to pass. When we move to Win10 Fall Creators Update and macOS 10.13, they should start passing.

(Is the exact OS version available to test here? If so, we could make these version-specific annotations. Otherwise, we'll just need to remove them when our infrastructure moves forward.)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> I filed bug 1341088 about adding validation in OTS for these tables, as I
> think it would be a worthwhile added level of protection, but would prefer
> not to block enabling the fonts on that, especially given that everyone else
> is shipping support without such a layer.

Do those other engines have the same level of content process sandboxing that we do, or better?  Or is this out of the content process anyway?

> The font is rejected because there's a variations-related extension to
> OpenType layout tables that the OTS code doesn't yet know about; I'm
> intending to fix that, but for now some (not all) variation fonts run into
> this issue.

Is there a bug on file to remove this?

> (Note that we already ship release builds with this pref set to false,
> because there are too many fonts in the wild that have minor anomalies that
> OTS will flag, even though harfbuzz will handle them safely.)

https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/modules/libpref/init/all.js#828-833

Do those tables ever get handled by anything other than harfbuzz?

> We are not running sufficiently up-to-date versions of either macOS or
> Windows in CI for these to pass. When we move to Win10 Fall Creators Update
> and macOS 10.13, they should start passing.
> 
> (Is the exact OS version available to test here? If so, we could make these
> version-specific annotations. Otherwise, we'll just need to remove them when
> our infrastructure moves forward.)

https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests says the condition variables are from mozinfo and https://firefox-source-docs.mozilla.org/mozbase/mozinfo.html claims that version is exposed but it doesn't seem to be in https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mozinfo.py .  So I guess not.
Flags: needinfo?(jfkthame)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #5)
> (In reply to Jonathan Kew (:jfkthame) from comment #4)
> > I filed bug 1341088 about adding validation in OTS for these tables, as I
> > think it would be a worthwhile added level of protection, but would prefer
> > not to block enabling the fonts on that, especially given that everyone else
> > is shipping support without such a layer.
> 
> Do those other engines have the same level of content process sandboxing
> that we do, or better?  Or is this out of the content process anyway?

This will be handled in the content process whenever text is shaped and measured/painted. (In a webrender world, the resource will also be passed over to the rendering process for rasterization, so it'll be used there as well.)

I don't know how our content process sandboxing stacks up against Chrome's or Edge's these days... we use chromium's sandbox code, don't we? But perhaps not configured in the same way.

> > The font is rejected because there's a variations-related extension to
> > OpenType layout tables that the OTS code doesn't yet know about; I'm
> > intending to fix that, but for now some (not all) variation fonts run into
> > this issue.
> 
> Is there a bug on file to remove this?

As this is somewhat separate from bug 1341088, I just filed bug 1447600 about it.

> > (Note that we already ship release builds with this pref set to false,
> > because there are too many fonts in the wild that have minor anomalies that
> > OTS will flag, even though harfbuzz will handle them safely.)
> 
> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/modules/libpref/init/all.js#828-833
> 
> Do those tables ever get handled by anything other than harfbuzz?

Not AFAIK; all shaping for these fonts is done via harfbuzz.
Flags: needinfo?(jfkthame)
Comment on attachment 8960715 [details] [diff] [review]
Enable support for OpenType variation fonts

OK, r=dbaron, but I hope you can get the two OTS bugs landed (and validation of the variation tables enabled rather than bypassed) sooner rather than later.
Attachment #8960715 - Flags: review?(dbaron) → review+
Thanks. Yes, that would be good; I've got PRs in process, but if they seem to get stalled, we could even consider taking them into our tree while they're still pending upstream.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfed80b95dfb
Enable support for OpenType variation fonts. r=dbaron
Sigh.... the try run in comment 1 doesn't show the Wr tests being run with webrender; I guess that's newly-enabled (though still tier-2, so shouldn't be blocking).

Anyhow, this is expected to fail as variation-font rendering is not yet implemented there (bug 1442693). So I'll add the appropriate annotation when re-landing.
Flags: needinfo?(jfkthame)
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7d2767a24d
Enable support for OpenType variation fonts. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/2a7d2767a24d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Will Bamberg [:wbamberg] from comment #14)
> I've updated the compat data for these:
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-optical-
> sizing#Browser_compatibility
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-variation-
> settings#Browser_compatibility
> 
> Marking dev-doc-complete, but please comment if we need anything else here.

rel note added as well:

https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS
You need to log in before you can comment on or make changes to this bug.