Closed Bug 1465983 Opened 2 years ago Closed 2 years ago

Don't resolve FontFaceSet's ready promise in its constructor

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(2 files)

Currently we resolve FontFaceSet's ready promise in its constructor. Doing a bit of bug archeology it seems that happened after heycam sent this email to the list:

https://lists.w3.org/Archives/Public/www-style/2014Jul/0047.html

at which point Tab updated the spec:

https://github.com/w3c/csswg-drafts/commit/e4cdd12c2e19ae6d24a22582ccc189783dfce4da

However, doing this will usually break waiting on document.fonts.ready before the initial loading of a document and its fonts. Any code that waits at that point will trigger immediately, before the fonts have actually loaded.

Tab also seems to have realized too, since he changed the spec to require the promise to be pending 4 days later:

https://github.com/w3c/csswg-drafts/commit/60433e453c26f7add86cba35240beca1426ddd19

I don't see any discussion about that on the list though.
(In reply to Jonathan Watt [:jwatt] from comment #0)
> Currently we resolve FontFaceSet's ready promise in its constructor.

Not quite technically correct (digging through old versions of the code, I got a bit confused). We actually currently set mResolveLazilyCreatedReadyPromise to true, but the effect is the same when the Promise is created by FontFaceSet::GetReady, since we'll then resolve it immediately after creating it.
This fixes at least web-platform/tests/css/css-fonts/variations/font-weight-matching.html and possibly others.
Attachment #8982376 - Flags: review?(cam)
Depends on: 1465997
Comment on attachment 8982376 [details] [diff] [review]
part 2 - Don't resolve FontFaceSet's ready promise in its constructor

Review of attachment 8982376 [details] [diff] [review]:
-----------------------------------------------------------------

If this fixes WPT tests I guess you'll need .ini updates.
Attachment #8982376 - Flags: review?(cam) → review+
Depends on: 1466251
Priority: -- → P2
Attachment #8982376 - Attachment description: patch → part 2 - Don't resolve FontFaceSet's ready promise in its constructor
I'll likely land preemptively today given you r+'ed a similar issue in bug 1466723, but requesting retrospective r? just to make sure you saw this.
Attachment #8983374 - Flags: review?(cam)
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94409f81ab0e
part 1 - Fix race in test_font_loading_api.html. rs=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/023d0b84b7a9
part 2 - Don't resolve FontFaceSet's ready promise in its constructor. r=heycam
Comment on attachment 8983374 [details] [diff] [review]
part 1 - Fix race in test_font_loading_api.html.

Review of attachment 8983374 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_font_loading_api.html
@@ +229,3 @@
>      var p = Promise.resolve();
>      sourceDocuments.forEach(function({ doc, what }) {
> +      p = p.then(doc.fonts.ready).then(function() {

Oops, it seems:

  p.then(doc.fonts.ready)

is essentially the same as not calling .then() at all. The 'onFulfilled' argument must be a function that returns doc.fonts.ready, it can't simply be doc.fonts.ready itself. I'll land a follow-up to fix this (and fix bug 1466903).
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4825bdfd6927
part 3 - Fix the use of Promise.then() in test_font_loading_api.html. r=orange
Blocks: 1466903
https://hg.mozilla.org/mozilla-central/rev/94409f81ab0e
https://hg.mozilla.org/mozilla-central/rev/023d0b84b7a9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8983374 [details] [diff] [review]
part 1 - Fix race in test_font_loading_api.html.

This plus the followup fix seems fine.

Makes me realise though that it would be nice to re-write this whole test file to use async/await...
Attachment #8983374 - Flags: review?(cam) → review+
You need to log in before you can comment on or make changes to this bug.