Bug 225790 - document.fonts.ready is sometimes still resolved too quickly
Summary: document.fonts.ready is sometimes still resolved too quickly
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-13 16:50 PDT by Cameron McCormack (:heycam)
Modified: 2022-07-28 17:10 PDT (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-05-13 16:50:12 PDT
Here is a test: https://mcc.id.au/2021/font-load.html

If I open this in a debug MiniBrowser, I get this output:

FontFaceSet.status in onload:   loaded
FontFace.status in onload:      unloaded
FontFaceSet.status after ready: loading
FontFace.status after ready:    loading

So the ready promise was initially resolved, and then at some point after the document.fonts.ready.then() call, it was replaced with a fresh, unresolved promise, so that once we're in the promise handler, the font has started loading.

In a regular Safari, I occasionally get this output, but usually I get this (more expected) output:

FontFaceSet.status in onload:   loaded
FontFace.status in onload:      unloaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

Firefox and Chrome both delay the load event for font loads from style sheets, and so we get different output still.  Firefox:

FontFaceSet.status in onload:   loaded
FontFace.status in onload:      loaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

Chrome:

FontFaceSet.status in onload:   loading
FontFace.status in onload:      loaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

(Chrome's makes a little less sense to me, with the FontFaceSet still loading but the only FontFace in it loaded.)

The upshot of this is that if relying on font loads to have been triggered by content already in the document by the time of the load event, document.fonts.ready.then() is not sufficient in Safari to wait for them.  I'm working around this in bug 225728 for some MathML tests.

I'm not even sure though that WebKit's behavior here violates the spec.  But it probably is a compat problem.
Comment 1 Ryosuke Niwa 2021-05-13 17:05:43 PDT
Maybe the issue here is that CSSFontFace::m_status is initialized to Status::Loaded?
Comment 2 Ryosuke Niwa 2021-05-13 17:08:14 PDT
Oh, this is about document.fonts.
Comment 3 Ryosuke Niwa 2021-05-13 17:12:03 PDT
(In reply to Ryosuke Niwa from comment #1)
> Maybe the issue here is that CSSFontFace::m_status is initialized to
> Status::Loaded?

I think this is still the issue. In particular, we rely on style resolution to find fonts in use. So if there was no style resolution by the time Document::implicitClose() is called, then we may fail to detect any fonts available in the document.

The reason this is flaky in Safari is probably because Safari injects scripts to run the reader availability and other things so it's probably triggering a style resolution before load event is fired in some cases.
Comment 4 Ryosuke Niwa 2021-05-13 17:14:03 PDT
Hm... we do explicitly trigger a style resolution here:

Ref<FontFaceSet> Document::fonts()
{
    updateStyleIfNeeded();
    return fontSelector().fontFaceSet();
}
Comment 5 Cameron McCormack (:heycam) 2021-05-13 17:16:41 PDT
Do we need to flush layout as well?  Triggering font loads should be based on the specific characters used in the content.
Comment 6 Cameron McCormack (:heycam) 2021-05-13 17:17:40 PDT
Still, flushing here doesn't 100% solve the problem, since script could in theory do something like:

let fonts = document.fonts;
document.write("<p>some content that would trigger a font load</p>");
onload = function() {
  fonts.ready.then(...)
};
Comment 7 Ryosuke Niwa 2021-05-13 17:28:31 PDT
(In reply to Cameron McCormack (:heycam) from comment #6)
> Still, flushing here doesn't 100% solve the problem, since script could in
> theory do something like:
> 
> let fonts = document.fonts;
> document.write("<p>some content that would trigger a font load</p>");
> onload = function() {
>   fonts.ready.then(...)
> };

Right. It looks like the current spec seems to suggest that any @font-face rule which appears in a CSS will automatically adds a new entry to FontFaceSet of the document:
https://drafts.csswg.org/css-font-loading/#font-face-css-connection

This might be problematic given that this will end up loading all fonts regardless of whether they're used or not.

It still makes it ambiguous as to when @font-face rule will be added to FontFaceSet and what "status" should say in the intervening period. Namely, if all @font-face rules are defined in an external CSS resource, then there is no CSS-connected FontFace object at the time of accessing document.fonts above, which means that document.fonts.status will say loaded. Only when the stylesheet gets loaded, then do we find @font-face rules and therefore adds more entries to the font source.

Given that a new stylesheet content might be loaded at any moment while the document in question is loading, I don't think there is any reason to believe document.fonts.status will return a reliable information until load event is fired on the global object.
Comment 8 Cameron McCormack (:heycam) 2021-05-13 17:34:12 PDT
(In reply to Ryosuke Niwa from comment #7)
> Right. It looks like the current spec seems to suggest that any @font-face
> rule which appears in a CSS will automatically adds a new entry to
> FontFaceSet of the document:
> https://drafts.csswg.org/css-font-loading/#font-face-css-connection
> 
> This might be problematic given that this will end up loading all fonts
> regardless of whether they're used or not.

I don't think that's what would happen.  CSS connected FontFace objects should only start loading when the UA decides it needs them per https://drafts.csswg.org/css-font-loading/#font-face-set-css.

> It still makes it ambiguous as to when @font-face rule will be added to
> FontFaceSet and what "status" should say in the intervening period. Namely,
> if all @font-face rules are defined in an external CSS resource, then there
> is no CSS-connected FontFace object at the time of accessing document.fonts
> above, which means that document.fonts.status will say loaded. Only when the
> stylesheet gets loaded, then do we find @font-face rules and therefore adds
> more entries to the font source.

I agree it's underdefined when exactly the FontFace object should start loading.  But I think, implicitly (like all other APIs), inspecting the contents of the FontFaceSet should reflect all style sheets currently loaded (which means inspecting the FontFaceSet should flush style sheets).

The loading of the external style sheets in my test should block the load event, so by the time we inspect document.fonts I think it's guaranteed that the FontFace is in there (even if it's not clear what its loading state should be).

> Given that a new stylesheet content might be loaded at any moment while the
> document in question is loading, I don't think there is any reason to
> believe document.fonts.status will return a reliable information until load
> event is fired on the global object.

Agreed.
Comment 9 Cameron McCormack (:heycam) 2021-05-13 17:41:52 PDT
(In reply to Ryosuke Niwa from comment #4)
> Hm... we do explicitly trigger a style resolution here:
> 
> Ref<FontFaceSet> Document::fonts()
> {
>     updateStyleIfNeeded();

I tried replacing this with updateLayout(), but it still didn't help my testcase.  I got this output instead:

FontFaceSet.status in onload:   loading
FontFace.status in onload:      loading
FontFaceSet.status after ready: loading
FontFace.status after ready:    loading

which is even more confusing, since if FontFaceSet.status in onload is loading, then we should already have done the ready state promise replacement, and for this test, that replacement should only happen once.
Comment 10 Cameron McCormack (:heycam) 2021-05-13 18:21:31 PDT
I might be missing something but I can't find any code that does the replacement of FontFaceSet::m_readyPromise when we switch from FontFaceSet state "loaded" to "loading" or any of the requirements to wait for resolving when "pending on the environment".  This latter problem is what prevents the layout flush from making the testcase work.
Comment 11 Ryosuke Niwa 2021-05-13 20:00:28 PDT
(In reply to Cameron McCormack (:heycam) from comment #10)
> I might be missing something but I can't find any code that does the
> replacement of FontFaceSet::m_readyPromise when we switch from FontFaceSet
> state "loaded" to "loading" or any of the requirements to wait for resolving
> when "pending on the environment".  This latter problem is what prevents the
> layout flush from making the testcase work.

That is indeed a problem although it's also strange that FontFaceSet.prototype.promise can end up returning different objects each time it is accessed.
Comment 12 Radar WebKit Bug Importer 2021-05-20 16:51:18 PDT
<rdar://problem/78284129>
Comment 13 Ahmad Saleem 2022-07-24 09:28:04 PDT
Based on the test case from Comment 0, I am getting following results across all browsers:

NOTE - All testing performed within Private / Incognito Mode with Multiple refresh to trigger unloaded issue.

*** Safari 15.6 on macOS 12.5 ****

FontFaceSet.status in onload:   loaded
FontFace.status in onload:      loaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

*** Chrome Canary 106 ****

FontFaceSet.status in onload:   loading
FontFace.status in onload:      loaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

*** Firefox Nightly 104 ****

FontFaceSet.status in onload:   loading
FontFace.status in onload:      loaded
FontFaceSet.status after ready: loaded
FontFace.status after ready:    loaded

Just wanted to share updated results. I am not sure whether something change in web-spec or when Firefox decided to follow Chrome for FontFaceSet but this is as of today updated results. Thanks!
Comment 14 Ryosuke Niwa 2022-07-27 21:06:51 PDT
This appears to be config changed now that all of them say "loaded"