1435984 - Implement the font-variation-settings descriptor for @font-face rules

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Jonathan Kew [:jfkthame]]

In bug 1321022, we implemented the font-variations-setting property; but (like the analogous font-feature-settings) we should also have a matching descriptor for use in @font-face, so that settings can be associated with a particular font face as opposed to a particular element.

Spec (draft): https://drafts.csswg.org/css-fonts-4/#font-rend-desc

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Cache the font-variations.enabled pref in StylePrefs for ready access in descriptor parsing

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Support the font-variation-settings descriptor when parsing @font-face in the old Gecko style system

Note that a corresponding Stylo patch will also be needed; maybe :emilio can help us out with that.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Store variation settings from the @font-face rule in the gfxFontEntry for user fonts

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Expose variationSettings on the FontFace webidl interface

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 5 - Apply variation settings from the font entry when instantiating fonts on macOS

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 6 - Apply variation settings from the font entry when instantiation fonts for DirectWrite

Comment 7

[Jonathan Kew [:jfkthame]]

Currently, patch 2 here results in a compile failure in servo/ports/geckolib/glue.rs because eCSSFontDesc_FontVariationSettings is not handled. I can stub that out locally so that my tree still builds, and the rest of this works as expected when stylo is disabled, but of course that's not really useful...  So :emilio, is there any chance you could take care of the Servo side of this for us, to add support for eCSSFontDesc_FontVariationSettings there?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Sure, Xidorn may be able to do it faster since he did all the other font descriptor integration IIRC, but if he doesn't have the cycles I can give it a shot.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: patch 7 - Apply variation settings from the font entry in the gfxFT2Fonts (Android) back-end

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 8 - Apply variation settings from the font entry in the Linux (fontconfig) back-end

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8949342 [details] [diff] [review]
patch 4 - Expose variationSettings on the FontFace webidl interface

r=me

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Sorry for being late. I'll take a look at this.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

Review commit: https://reviewboard.mozilla.org/r/219732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219732/

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8950468 [details]
Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219732/diff/1-2/

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

jfkthame, landing code touching servo side still needs coordinated landing, which can be tricky especially without using autoland. Sorry about that. Do you have further change to your patches? If not, I'll land them together when my patch gets r+ed.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

There is one problem... I probably forgot to have this behind the pref...

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8950468 [details]
Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219732/diff/2-3/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8950468 [details]
Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

https://reviewboard.mozilla.org/r/219732/#review225488

Looks sensible, thanks so much!

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> jfkthame, landing code touching servo side still needs coordinated landing,
> which can be tricky especially without using autoland. Sorry about that. Do
> you have further change to your patches? If not, I'll land them together
> when my patch gets r+ed.

Thanks! I believe they're all ready to land.

Comment 20

[Jonathan Kew [:jfkthame]]

I pushed a try job with these patches, and it seems to be showing a worrying number of failures on font-loading-related tests, e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=40e6887f8c8c79c55822bd5aa995938652e232b7&selectedJob=161932501. So better not land these just yet; I'll see if I can reproduce this and figure out what's going wrong.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Actually I pushed a try run as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38eff191c6d82a4214b0e04ca25283edb0f97809

And I was just going to ni? you about that. It seems all issues happen when stylo is disabled, so it's something in the old style system code.

Comment 22

[Jonathan Kew [:jfkthame]]

Right, this doesn't seem to be related to the stylo side of it. I have a couple meetings coming up but hope to investigate this later this afternoon.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Neither stylo part nor the common part. Stylo effectively only does parsing in this case, so sounds like there is probably something wrong with the nsCSSParser.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8950468 [details]
Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

https://reviewboard.mozilla.org/r/219732/#review225712

::: servo/components/style/font_face.rs:416
(Diff revision 3)
>              font_feature_settings::SpecifiedValue::normal()
>          },
>  
> +        /// The variation settings of this font face.
> +        "font-variation-settings" variation_settings / mFontVariationSettings: FontVariationSettings = {
> +            font_variantion_settings::SpecifiedValue::normal()

"font_variantion_settings" looks like a typo here

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Expose variationSettings on the FontFace webidl interface

The failures in font-face tests were due to a missing prefs-check in patch 4; it was fine provided font-variation-settings were enabled, but not with the feature preffed off (which is currently our default setting). With this fixed, tryserver is much happier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f43da693bfcf29bb948e8a25bb99e4a39a9412c. The remaining failures look like typical intermittents, unrelated to the patches here. (The fix is "patch 4.1" in that try push, but merged into the updated version of patch 4 here.)

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Hmmm... that doesn't explain why nothing goes wrong with stylo... As far as I can see, ParseDescriptor doesn't check the pref for Stylo like in Gecko.

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

I mean, ParseDescriptor doesn't check the pref with Stylo either.

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8950468 [details]
Bug 1435984 - Integrate font-variation-settings descriptor with stylo.

https://reviewboard.mozilla.org/r/219732/#review225712

> "font_variantion_settings" looks like a typo here

This is indeed a typo. But it seems this code is completely ignored for Gecko... We should probably just remove it...

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

Hmm, so Gecko actually does an extra check in the parser, so it fails to parse the default value, and consequently FontFace fails to be constructed... that makes sense I guess. I don't think we should add the same check to Servo, but I do concern that if we forgot to add the check here and nothing fails, we would expose something to the web content by mistake.

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Servo PR

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/890893e95355
patch 0 - Add binding function for setting float value to nsCSSValue. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8ce0b5c90b11
patch 1 - Cache the font-variations.enabled pref in StylePrefs for ready access in descriptor parsing. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/3f431418dfe9
patch 2 - Support the font-variation-settings descriptor when parsing @font-face in the old Gecko style system. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/cb6b4a2f5170
patch 3 - Store variation settings from the @font-face rule in the gfxFontEntry for user fonts. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/0e636a5b909a
patch 4 - Expose variationSettings on the FontFace webidl interface. r=bz,jwatt
https://hg.mozilla.org/integration/autoland/rev/cecc23e078c6
patch 5 - Apply variation settings from the font entry when instantiating fonts on macOS. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/dc462be5c9a6
patch 6 - Apply variation settings from the font entry when instantiation fonts for DirectWrite. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/773e30e83ce1
patch 7 - Apply variation settings from the font entry in the gfxFT2Fonts (Android) back-end. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/2b4451a45ed7
patch 8 - Apply variation settings from the font entry in the Linux (fontconfig) back-end on a CLOSED TREE. r=lsalzman

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/890893e95355
https://hg.mozilla.org/mozilla-central/rev/8ce0b5c90b11
https://hg.mozilla.org/mozilla-central/rev/3f431418dfe9
https://hg.mozilla.org/mozilla-central/rev/cb6b4a2f5170
https://hg.mozilla.org/mozilla-central/rev/0e636a5b909a
https://hg.mozilla.org/mozilla-central/rev/cecc23e078c6
https://hg.mozilla.org/mozilla-central/rev/dc462be5c9a6
https://hg.mozilla.org/mozilla-central/rev/773e30e83ce1
https://hg.mozilla.org/mozilla-central/rev/2b4451a45ed7

Comment 33

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face
https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-variation-settings
https://github.com/mdn/browser-compat-data/pull/1207
https://developer.mozilla.org/en-US/Firefox/Experimental_features

Let me know if this looks OK. Thanks!

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #33)
> https://developer.mozilla.org/en-US/Firefox/Experimental_features

copy/paste issue: s/property/descriptor/ for the descriptor version of font-variation-settings.

The note "Functions only in Mac OS Sierra (and later)" is not correct, we now have variation font support (still preffed off at present) on sufficiently recent versions of all platforms. It landed first for macOS Sierra or later (bug 1321031), but is now also on Win10 Fall Creators Update or later (bug 1430632), Linux provided the system has a fairly recent FreeType library (bug 1427641), and Android (bug 1434697). This also applies to the similar note for the property.

(Sorry, should have flagged dev-doc-needed on each of those, I guess!)

Comment 35

[Chris Mills [:cmills]]

(In reply to Jonathan Kew (:jfkthame) from comment #34)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #33)
> > https://developer.mozilla.org/en-US/Firefox/Experimental_features
> 
> copy/paste issue: s/property/descriptor/ for the descriptor version of
> font-variation-settings.
> 
> The note "Functions only in Mac OS Sierra (and later)" is not correct, we
> now have variation font support (still preffed off at present) on
> sufficiently recent versions of all platforms. It landed first for macOS
> Sierra or later (bug 1321031), but is now also on Win10 Fall Creators Update
> or later (bug 1430632), Linux provided the system has a fairly recent
> FreeType library (bug 1427641), and Android (bug 1434697). This also applies
> to the similar note for the property.

Ah cool, I've fixed that stuff, thanks for the review!

I'm not sure how best to represent all the support minutiae. Maybe I should just add a note to the browser compat data tables.

Would the same be true for support in other browsers too?

> 
> (Sorry, should have flagged dev-doc-needed on each of those, I guess!)

In future, it would be good, cheers ;-)

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #35)
> Would the same be true for support in other browsers too?

Not necessarily.

I think Safari only supports it on 10.13+.

Not sure exactly what level of support Edge currently has, but fairly sure it'll be only on the latest Windows update.

However, I believe Chrome supports variation fonts even on older OS versions (Windows at least, maybe macOS?) by incorporating its own copy of FreeType and using that instead of the platform's font rasterizer when the OS lacks support. (We've considered doing that but have not yet had the resources to try actually implementing it, and unsure at this point how important it would be.)