Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FontFaceSet.ready resolve-time #4248

Open
gsnedders opened this issue Aug 28, 2019 · 9 comments
Open

FontFaceSet.ready resolve-time #4248

gsnedders opened this issue Aug 28, 2019 · 9 comments

Comments

@gsnedders
Copy link
Member

https://drafts.csswg.org/css-font-loading/#font-face-set-ready

This appears to be pretty vague as to when it resolves:

The ready attribute contains a Promise which is resolved when the document is done loading fonts, which provides a way for authors to avoid having to keep track of which fonts have or haven’t been loaded before examining content which may be affected by loading fonts.

At what point is the document "done loading fonts"? It would be good to add some cross-reference to something for this. Which document? Or is this meant to be informative and the normative definition that in "switch the FontFaceSet to loaded"?

Per web-platform-tests/wpt#17219 (comment), it seems Blink and Gecko wait until after the first layout, whereas WebKit resolves once it has loaded but before layout is done (should add tests for fallback!). The IDL block has a comment // async notification that font loading and layout operations are done but the "layout operations" part doesn't seem to be backed up by any normative text?

@emilio
Copy link
Collaborator

emilio commented Aug 28, 2019

@rniwa
Copy link

rniwa commented Jan 29, 2020

Also see #1081

@rniwa
Copy link

rniwa commented Jan 29, 2020

Why doesn't this definition refer to the concept of pending on the environment? That's the difference between when this promise is resolved vs. FontFaceSet is no loner pending on the environment.

@rniwa
Copy link

rniwa commented Jan 29, 2020

Looks like Gecko & Blink's implementations are racy with regards to load & DOMContentLoaded events whereas WebKit's implementation is incorrect (it doesn't wait for the document to finish loading).

@tabatkins
Copy link
Member

Or is this meant to be informative and the normative definition that in "switch the FontFaceSet to loaded"?

Yes, this; your quoted text is an informative introduction to the attribute; the algo later in the spec actually manipulates the promise (and does refer to "pending on the environment").

Does anything actually need to be done here, beyond what #1081 is already about?

@rniwa
Copy link

rniwa commented Jan 29, 2020

I think we need to make sure the informative section is noted as such. There is no annotation that it's non-normative so I thought this was the normative text.

@tabatkins
Copy link
Member

That's reasonable. I'll see if I can gin up some styles that indicate it's a vague description, like the domintro boxes in WHATWG specs.

@emilio
Copy link
Collaborator

emilio commented Jan 29, 2020

Looks like Gecko & Blink's implementations are racy with regards to load & DOMContentLoaded events whereas WebKit's implementation is incorrect (it doesn't wait for the document to finish loading).

Can you elaborate? You mean that the promise will be resolved if you add a stylesheet between DOMContentLoaded and load? I don't think that's the case (though it's subtle).

@rniwa
Copy link

rniwa commented Jan 31, 2020

Can you elaborate? You mean that the promise will be resolved if you add a stylesheet between DOMContentLoaded and load? I don't think that's the case (though it's subtle).

In the following example, Firefox would sometimes resolves "ready" before DOMContentLoaded is fired.

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<style>
@font-face {
    font-family: Ahem;
    src: url("./fonts/Ahem.ttf");
}
</style>
<script>
window.addEventListener("load", () => {
    document.getElementById('log').textContent += 'loaded: ' + document.fonts.check('10px Ahem') + '\n';
    document.getElementById('log').textContent += 'document.fonts.size: ' + document.fonts.size + '\n';
});
window.addEventListener("DOMContentLoaded", () => {
    document.getElementById('log').textContent += 'DOMContentLoaded: ' + document.fonts.check('10px Ahem') + '\n';
});
document.fonts.ready.then(() => {
    document.getElementById('log').textContent += 'ready: ' + document.fonts.check('10px Ahem') + '\n';
});
</script>
</head>
<body>
<pre id="log"></pre>
<div id="element1" style="font-size: 10px; font-family: Ahem;">hello</div>
</body>
</html>

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
…tion-markup/scripts/underover-parameters-3.html is a flakey failure

https://bugs.webkit.org/show_bug.cgi?id=205168

Reviewed by Simon Fraser.

Source/WebCore:

The flakiness of the test was caused by document.fonts.ready resolving without waiting for the document
to be loaded if FontFaceSet::FontFaceSet is created after the initial layout had been done.

r249295 introduced this racy code to address the previous behavior of WebKit, which didn't wait for
either document to finish loading or the initial layout to happen at all, and the WebKit workaround in
the offending WPT test was subsequentially removed, resulting in the flaky failure after the test was
synced up in r249760.

This patch address the underlying bug by making ready promise wait until Document::implicitClose is called.

Unfortunately, the specification is extremely vague in terms of when this promise is resolved but new
behavior of WebKit is more or less with Firefox (Chrome seems to have a similar race condition as WebKit).

See also: w3c/csswg-drafts#4248.

Test: fast/css/font-face-set-ready-after-document-load.html

* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::fontFaceSetIfExists): Renamed from optionalFontFaceSet for consistency.
* css/CSSFontSelector.h:
* css/FontFaceSet.cpp:
(WebCore::FontFaceSet::FontFaceSet): Fixed the bug.
(WebCore::FontFaceSet::documentDidFinishLoading): Renamed from didFirstLayout to reflect the new semantics.
(WebCore::FontFaceSet::completedLoading):
* css/FontFaceSet.h:
* dom/Document.cpp:
(WebCore::Document::implicitClose): Deployed makeRefPtr to be safe.

LayoutTests:

* fast/css/font-face-set-ready-after-document-load-expected.txt: Added.
* fast/css/font-face-set-ready-after-document-load.html: Added.


Canonical link: https://commits.webkit.org/220000@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@255414 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants