Closed Bug 1435984 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 1 obsolete file)

2.69 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.52 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
22.69 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
7.21 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
5.89 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
5.46 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
2.06 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
emilio
: review+
Details
5.09 KB, patch
Details | Diff | Splinter Review
41 bytes, text/x-github-pull-request
Details | Review
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
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Note that a corresponding Stylo patch will also be needed; maybe :emilio can help us out with that.
Attachment #8949339 - Flags: review?(jwatt)
Attachment #8949342 - Flags: review?(jwatt)
Attachment #8949342 - Flags: review?(bzbarsky)
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?
Flags: needinfo?(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.
Flags: needinfo?(emilio) → needinfo?(xidorn+moz)
Attachment #8949343 - Flags: review?(lsalzman)
Attachment #8949344 - Flags: review?(lsalzman)
Attachment #8949479 - Flags: review?(lsalzman)
Attachment #8949552 - Flags: review?(lsalzman)
Attachment #8949343 - Flags: review?(lsalzman) → review+
Attachment #8949344 - Flags: review?(lsalzman) → review+
Attachment #8949479 - Flags: review?(lsalzman) → review+
Attachment #8949552 - Flags: review?(lsalzman) → review+
Comment on attachment 8949342 [details] [diff] [review]
patch 4 - Expose variationSettings on the FontFace webidl interface

r=me
Attachment #8949342 - Flags: review?(bzbarsky) → review+
Attachment #8949338 - Flags: review?(jwatt) → review+
Attachment #8949339 - Flags: review?(jwatt) → review+
Attachment #8949340 - Flags: review?(jwatt) → review+
Attachment #8949342 - Flags: review?(jwatt) → review+
Sorry for being late. I'll take a look at this.
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.
Flags: needinfo?(xidorn+moz) → needinfo?(jfkthame)
There is one problem... I probably forgot to have this behind the pref...
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!
Attachment #8950468 - Flags: review?(emilio) → review+
(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.
Flags: needinfo?(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.
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.
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.
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 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
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.)
Attachment #8949342 - Attachment is obsolete: true
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.
I mean, ParseDescriptor doesn't check the pref with Stylo either.
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...
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.
Attached file Servo PR
Pushed by xquan@mozilla.com:
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
(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!)
Flags: needinfo?(jfkthame)
(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 ;-)
(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.)
You need to log in before you can comment on or make changes to this bug.