Closed
Bug 1465983
Opened 7 years ago
Closed 7 years ago
Don't resolve FontFaceSet's ready promise in its constructor
Categories
(Core :: Layout, enhancement, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jwatt, Assigned: jwatt)
References
Details
Attachments
(2 files)
2.19 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
(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.
Assignee | ||
Comment 2•7 years ago
|
||
This fixes at least web-platform/tests/css/css-fonts/variations/font-weight-matching.html and possibly others.
Attachment #8982376 -
Flags: review?(cam)
Comment 3•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Attachment #8982376 -
Attachment description: patch → part 2 - Don't resolve FontFaceSet's ready promise in its constructor
Assignee | ||
Comment 4•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 6•7 years ago
|
||
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/94409f81ab0e
https://hg.mozilla.org/mozilla-central/rev/023d0b84b7a9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•