Bug 225728 – some MathML WPTs need tweaking to avoid font loading race in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Cameron McCormack (:heycam)]

There are some MathML WPTs that rely on a web font, and they use document.fonts.ready from inside a window load event handler to wait until the font has been loaded before starting the test.  In WebKit, it looks like this is insufficient to wait for the font to be loaded, because (I'm assuming!) there is no guarantee that layout has been flushed by the time the load event handler runs, and despite relevant content being in the document that should cause a font load to start, the FontFaceSet does not wait for that flush to happen before resolving the ready Promise.  It probably should wait for that, but to avoid running into this font loading problem (since the WPTs aren't testing font loading specifically), let's tweak the way the tests wait.

[Simon Fraser (smfr)]

But maybe also file a bug on the timing of document.fonts.ready? That should have its own WPT test, perhaps?

[Cameron McCormack (:heycam)]

Created attachment 428452 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Cameron McCormack (:heycam)]

Yes, I will file a bug about that.  WPTs for the Font Loading API unfortunately don't have great coverage.

[Cameron McCormack (:heycam)]

After reading through the spec some more, I was wrong about the ready promise.  If there are loads in flight, then it does indeed need to wait for layout to have been flushed (so that it's no longer "pending on the environment").  But there is no requirement to replace a settled ready promise with a fresh, pending one as soon as content is inserted into the document.  The spec isn't very strict on when loads get kicked off due to content in the document, it just says:

> When a user-agent needs to load a font face, it must do so by calling the load() method of the corresponding FontFace object.

So I think by the letter of the spec, the current WebKit behavior is allowed.  In fact, it's probably not great to be replacing the ready promise every time some content in the document is changed.

That does make it hard to use the ready promise in conjunction with font loads initiated from content (as opposed to explicit load() calls).

[Cameron McCormack (:heycam)]

And gsnedders points out in https://github.com/w3c/csswg-drafts/issues/1088#issuecomment-292997364 that "Chrome and Firefox delay the load event for a used @font-face, Safari and Edge do not".  So I do think the safe thing to do with these tests is to explicitly load() the FontFaces and wait on them to complete.

[Cameron McCormack (:heycam)]

Created attachment 428462 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 428452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428452&action=review

Thanks for working on this and great to see more tests passing !

If this approach works in Firefox and Chromium too, it sounds good to do that change indeed.

(In reply to Cameron McCormack (:heycam) from comment #4)
> Yes, I will file a bug about that.  WPTs for the Font Loading API
> unfortunately don't have great coverage.

May I ask you to cc me to this bug?

> LayoutTests/imported/w3c/web-platform-tests/mathml/support/fonts.js:3
> +  return Promise.allSettled([...document.fonts].map(f => f.load()));

Can you please mention that document.fonts.ready is supposed to do that and why we don't use it (spec unclear, browsers inconsistent...)?

And also why we can't use Promise.all ?

[Cameron McCormack (:heycam)]

Created attachment 428464 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428466 [details]
Patch

[Cameron McCormack (:heycam)]

(In reply to Frédéric Wang (:fredw) from comment #8)
> May I ask you to cc me to this bug?

I will once I file it.  Though from my comment 5, I don't think WebKit is wrong.  But the different behavior between engines is a potential compat problem (as shown by these tests), so I'll file one to track doing something.

> > LayoutTests/imported/w3c/web-platform-tests/mathml/support/fonts.js:3
> > +  return Promise.allSettled([...document.fonts].map(f => f.load()));
> 
> Can you please mention that document.fonts.ready is supposed to do that and
> why we don't use it (spec unclear, browsers inconsistent...)?

OK.

> And also why we can't use Promise.all ?

Because I plan to land this patch before the one adding operators.woff (to make it clear which expectation changes are due to which change), and the promise will be rejected if the font file isn't found.

[Cameron McCormack (:heycam)]

Created attachment 428467 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428576 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428580 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428609 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428674 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 428679 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 428679 [details]
Patch

Finally got all the test expectations update right, at least for the EWS bots.  Fred, can you take a look at this?

[Cameron McCormack (:heycam)]

Created attachment 428717 [details]
patch produced by git

It may be more instructive to look at the patch as produced by git, which has shows file moves/renames.

[Darin Adler]

Comment on attachment 428679 [details]
Patch

I’m going to set review+ but I get the impression you want to wait to hear from Frédéric thinks.

[Cameron McCormack (:heycam)]

(In reply to Darin Adler from comment #20)
> I’m going to set review+ but I get the impression you want to wait to hear
> from Frédéric thinks.

Thanks.  I think Frédéric's input in an earlier comment and your review is sufficient.  I'll land it on Monday so I can deal with any other expectation update fallout / rebase the bug 225586 patch while I'm at my desk.

[Frédéric Wang (:fredw)]

Comment on attachment 428679 [details]
Patch

Yes, patch LGTM too

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #22)
> Comment on attachment 428679 [details]
> Patch
> 
> Yes, patch LGTM too

Well, I forgot to say that WebKit does not have automatic sync for WPT. Our current procedure is

1) Export the WPT changes on GitHub, get them approved and land them.
2) Land the patch with the WPT change in WebKit.

For (1), it's possible to use an experimental script from Youenn so that you get auto-approval if the patch was already reviewed in WebKit. But it's likely not going to work well here, so I'd suggest you send a PR manually.

[Cameron McCormack (:heycam)]

Some of the tests have changed upstream, but not yet been updated in our copy.  Is it OK to rework the changes so that the apply upstream, in the WPT PR, and land the patches here to the old copies of the tests?  Will that mess up any future import of upstream WPT changes?

[Frédéric Wang (:fredw)]

(In reply to Cameron McCormack (:heycam) from comment #24)
> Some of the tests have changed upstream, but not yet been updated in our
> copy.  Is it OK to rework the changes so that the apply upstream, in the WPT
> PR, and land the patches here to the old copies of the tests?  Will that
> mess up any future import of upstream WPT changes?

cc'ing Ryosuke who knows better the status, but my understanding is that the general rule is: it is ok to rework the changes so that they apply upstream ; but not to apply changes to our copies before they are synced again.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #25)
> (In reply to Cameron McCormack (:heycam) from comment #24)
> > Some of the tests have changed upstream, but not yet been updated in our
> > copy.  Is it OK to rework the changes so that the apply upstream, in the WPT
> > PR, and land the patches here to the old copies of the tests?  Will that
> > mess up any future import of upstream WPT changes?
> 
> cc'ing Ryosuke who knows better the status, but my understanding is that the
> general rule is: it is ok to rework the changes so that they apply upstream
> ; but not to apply changes to our copies before they are synced again.

It's okay to land WebKit changes so long as the WPT changes upstream had already been landed. No need to wait for WPT re-sync into WebKit repo to happen.

[Cameron McCormack (:heycam)]

https://github.com/web-platform-tests/wpt/pull/29011

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 428679 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Cameron McCormack (:heycam)]

Created attachment 428809 [details]
Patch

[EWS]

Committed r277577 (237804@main): <https://commits.webkit.org/237804@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428809 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78091026>

[Truitt Savell]

These two tests are failing on Big Sur across intel and Apple Silicon queues

imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001.html
  imported/w3c/web-platform-tests/mathml/relations/css-styling/padding-border-margin/padding-002.html

History;
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fmathml%2Frelations%2Fcss-styling%2Fpadding-border-margin%2Fpadding-002.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-audioworklet-interface%2Faudioworkletprocessor-process-frozen-array.https.html

I think these just need to be skipped on Big Sur overall sense there was an attempt to skip this on arm64

[Cameron McCormack (:heycam)]

That they are failing on Big Sur across the board makes me think we shouldn't be skipping, and we should just add some Big Sur specific test expectations.

[Darin Adler]

(In reply to Cameron McCormack (:heycam) from comment #33)
> That they are failing on Big Sur across the board makes me think we
> shouldn't be skipping, and we should just add some Big Sur specific test
> expectations.

Might do this in two parts:

1) Bot watchers could add the skip expectations to get us back to green.
2) Someone with more expertise in the area can create an appropriate test expectation and unskip. Might want a reviewer on that.

[Cameron McCormack (:heycam)]

I have a patch up in bug 225902 for the updated expectations for these two tests, after I tested locally on my Big Sur machine.  I had planned to just land that unreviewed, if it ended up green.

[Darin Adler]

(In reply to Cameron McCormack (:heycam) from comment #35)
> I have a patch up in bug 225902 for the updated expectations for these two
> tests, after I tested locally on my Big Sur machine.  I had planned to just
> land that unreviewed, if it ended up green.

Sounds great.

[Sam Sneddon [:gsnedders]]

(In reply to Cameron McCormack (:heycam) from comment #5)
> After reading through the spec some more, I was wrong about the ready
> promise.  If there are loads in flight, then it does indeed need to wait for
> layout to have been flushed (so that it's no longer "pending on the
> environment").  But there is no requirement to replace a settled ready
> promise with a fresh, pending one as soon as content is inserted into the
> document.  The spec isn't very strict on when loads get kicked off due to
> content in the document, it just says:
> 
> > When a user-agent needs to load a font face, it must do so by calling the load() method of the corresponding FontFace object.
> 
> So I think by the letter of the spec, the current WebKit behavior is
> allowed.  In fact, it's probably not great to be replacing the ready promise
> every time some content in the document is changed.
> 
> That does make it hard to use the ready promise in conjunction with font
> loads initiated from content (as opposed to explicit load() calls).

It sounds like it's probably worth filing a spec bug to make this more concrete, at least?