Bug 225728

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

Created attachment 428452 [details] Patch

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

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

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).

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.

Created attachment 428462 [details] Patch

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 ?

Created attachment 428464 [details] Patch

Created attachment 428466 [details] Patch

(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.

Created attachment 428467 [details] Patch

Created attachment 428576 [details] Patch

Created attachment 428580 [details] Patch

Created attachment 428609 [details] Patch

Created attachment 428674 [details] Patch

Created attachment 428679 [details] Patch

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?

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.

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.

(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.

Comment on attachment 428679 [details] Patch Yes, patch LGTM too

(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.

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?

(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.

(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.

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

Created attachment 428809 [details] Patch

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].

<rdar://problem/78091026>

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

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.

(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.

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.

(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.

(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?