Closed Bug 1317445 Opened 3 years ago Closed 2 years ago

ship support for font-display descriptor for CSS @font-face rules

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: dbaron, Assigned: jfkthame, NeedInfo)

References

()

Details

(Keywords: css3, dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P2][behind pref layout.css.font-display.enabled])

Attachments

(1 file)

In bug 1157064 an implementation of font-display was landed.  We should figure out whether this is stable enough to ship, and if so, figure out what blocks shipping it and ship it (by enabling the "layout.css.font-display.enabled" preference).
Keywords: DevAdvocacy
Whiteboard: [DevRel:P3]
Whiteboard: [DevRel:P3] → [DevRel:P3][behind pref layout.css.font-display.enabled]
David, looks like pref ON on nightly & aurora first would be able to stress out stability issues or blocking issues before shipping it. What do you think ?
Flags: needinfo?(dbaron)
The first thing is to figure out the current status of the feature in the CSS Working Group and in other browsers, and to look at how what we've implemented corresponds to the spec.
Flags: needinfo?(dbaron)
Whiteboard: [DevRel:P3][behind pref layout.css.font-display.enabled] → [DevRel:P2][behind pref layout.css.font-display.enabled]
See Also: → 1355345
Shipped in Chrome 60, 24 July 2017. This fits in performance work as it helps with perceived performance issues of slow loading fonts. Even higher impact is expected on mobile.

ni? :jet to clarify where this is in the queue. As far as I understand we have an implementation and might just need a review of how well it matches the spec and/or web-platform-tests.
Flags: needinfo?(bugs)
Note that the current CSS Fonts 4 draft has an open issue[1] regarding the values used for the font-display descriptor. We should try to get that resolved before anyone ships this to release.

[1] https://drafts.csswg.org/css-fonts-4/#issue-01b739ba
Oh, but you said:

> Shipped in Chrome 60, 24 July 2017.

That probably means the CSS Fonts 4 issue has been pre-empted. Or maybe the CSS WG reached a resolution recently (time to go check minutes...) and the document just hasn't been updated yet.
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> That probably means the CSS Fonts 4 issue has been pre-empted. Or maybe the
> CSS WG reached a resolution recently (time to go check minutes...) and the
> document just hasn't been updated yet.

Thanks, Jonathan. I filed this spec issue:
https://github.com/w3c/csswg-drafts/issues/1676

Can you write the pref-flip patch for this one? Thx!
Assignee: nobody → jfkthame
Flags: needinfo?(bugs)
I posted to www-style regarding the issue mentioned in comment 4 (https://lists.w3.org/Archives/Public/www-style/2017Aug/0003.html), but unless there's a sudden flurry of interest in discussing the names, we should probably just ship this as it stands.
Attachment #8892956 - Flags: review?(dbaron)
Even if enabling this prefs, Gecko doesn't seem to pass WPT (testing/web-platform/tests/css-font-display/).

Also, our test harness has bugs for Windows (bug 1386496) and macOS (bug 1386743).
Also note that, it seems we don't currently support using pref to disable font-display descriptor at all in stylo...

I'm unsure whether we should add that pref support. It may not be that hard, though.
(In reply to Makoto Kato [:m_kato] from comment #8)
> Even if enabling this prefs, Gecko doesn't seem to pass WPT
> (testing/web-platform/tests/css-font-display/).

Does Chrome pass these tests?
Flags: needinfo?(jfkthame)
(Or, somewhat more generally, how interoperable are we with Chrome, based on the tests?  Is the problem that the tests are wrong?)
According to http://wpt.fyi/, Chrome 62.0 doesn't seem to pass font-display test yet.
Note that in addition to the wpt test, we have a couple of basic tests in layout/reftests/font-face/font-display-* which seem to generally work as expected -- although they're fragile/intermittent due to the timing-related nature of this feature.

AFAICT, our behavior when this is enabled turns out to be pretty similar to Chrome's (as it's intended to be). Given the nature of this feature (not really a new layout feature, more of a tweak that can affect the "feel" of how pages load, particularly with slower connection), I'm leaning towards preffing it on so that we can see what sort of feedback (if any) it gets when people begin to experience the effect it has on sites.

And we should try to work with the Blink team to understand what the issue is with the WPT tests, and get them fixed up to actually be useful (or understand why neither of our implementations pass them as they stand). Running the testcase interactively in the browser, I see that Firefox and Chrome do end up showing slightly different results, but I don't really understand either of them in terms of what the test purports to be checking.
Flags: needinfo?(jfkthame)
I'd like to get this turned on for 58 to give it more bake time in case of problems. Can we get a Try run and see what state the tests are with this change? I'm concerned about the fragile/intermittent failures spreading beyond the font-display-* tests when we flip this pref.
Flags: needinfo?(jfkthame)
Depends on: 1403254
I've started a try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7336a44314c3b10d0a29d977c09befd945c0ef6.

I doubt there will be any significant impact on tests; we still fail the web-platform test for this feature (as does Chrome; I don't really understand the test), but pass our own basic reftest (which previously force-enabled the pref), except that it's disabled on linux32 for frequent failures (not really surprising, given the inherent timing-dependency). The font-display reftest will remain a bit intermittent, I expect, until we figure out some other approach that can tolerate the occasionally-extreme timing variations. But flipping this pref shouldn't affect other tests in any way.

Maybe if we could arrange to always run the font-display reftest as the first test in a chunk, it would be more reliable (on the basis that it wouldn't encounter the kind of pauses we get when dozens/hundreds of documents loaded for previous testcases are being torn down).
Flags: needinfo?(jfkthame)
:dbaron, given that the failing WPT test seems to be primarily an issue of a fragile/broken test that doesn't currently pass anywhere, are you OK with us flipping this pref to begin to get some exposure here?
Flags: needinfo?(dbaron)
Jonathan, happy to assist with making this a pref-flip experiment. (thanks for the suggestion, Harald)
Flags: needinfo?(jfkthame)
Comment on attachment 8892956 [details] [diff] [review]
Enable support for the 'font-display' descriptor in @font-face rules

OK, enabling sounds good, but we should also get working tests into WPT somehow.
Flags: needinfo?(dbaron)
Attachment #8892956 - Flags: review?(dbaron) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47b7fed9767
Enable support for the 'font-display' descriptor in @font-face rules. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/b47b7fed9767
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assuming we keep this enabled all the way to Release, we should update the Browser compatibility note on the MDN page[1] to indicate the new status.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-display
Flags: needinfo?(jfkthame)
A few thoughts on getting more stable tests for this feature:  I think it may be possible to combine two ideas in order to make tests that work better:

 (1) Use mochitest's sjs capability to retprn font data only after certain other events have happened.  I think this is doable, although I'm not 100% sure.  See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F

 (2) Make the font timing respond to DOMWindowUtils::AdvanceTimeAndRefresh.  (I think we should probably do this for other time-based things as well.)  See https://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/interfaces/base/nsIDOMWindowUtils.idl#1486-1516

Also, perhaps a new bug should be filed for better tests?
Flags: needinfo?(jfkthame)
I've documented this:

I have added the descriptor to our browser compatibility data repo, and added the macro to the reference page so the correct browser compat data will show up once the PR is merged:

https://github.com/mdn/browser-compat-data/pull/560
https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-display

I've removed font-display from the experimental features page:

https://developer.mozilla.org/en-US/Firefox/Experimental_features

I've added it to the main @font-face page:

https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face

Last, I've added a note to the Fx58 rel notes:

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