David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 8 years ago

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).

	Astley Chen (inactive)
	Comment 1 • 8 years ago

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 ?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 2 • 8 years ago

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.

	:Harald Kirschner :digitarald
	Comment 3 • 7 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 4 • 7 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 5 • 7 years ago

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.

	Jet Villegas (inactive)
	Comment 6 • 7 years ago

(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!

	Jonathan Kew [:jfkthame] Assignee
	Comment 7 • 7 years ago

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.

	Makoto Kato [:m_kato]
	Comment 8 • 7 years ago

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).

	Xidorn Quan [:xidorn] UTC+11
	Comment 9 • 7 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 10 • 7 years ago

(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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 11 • 7 years ago

(Or, somewhat more generally, how interoperable are we with Chrome, based on the tests?  Is the problem that the tests are wrong?)

	Makoto Kato [:m_kato]
	Comment 12 • 7 years ago

According to http://wpt.fyi/, Chrome 62.0 doesn't seem to pass font-display test yet.

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 7 years ago

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.

	Jet Villegas (inactive)
	Comment 14 • 7 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 15 • 7 years ago

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).

	Jonathan Kew [:jfkthame] Assignee
	Comment 16 • 7 years ago

: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?

	Andreas Bovens [:abovens]
	Comment 17 • 7 years ago

Jonathan, happy to assist with making this a pref-flip experiment. (thanks for the suggestion, Harald)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 18 • 7 years ago

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.

	Pulsebot
	Comment 19 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b47b7fed9767

	Jonathan Kew [:jfkthame] Assignee
	Comment 21 • 7 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 22 • 7 years ago

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?

	Chris Mills [:cmills]
	Comment 23 • 7 years ago

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