imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3.html This test is a flakey failure on Mac History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fmathml%2Fpresentation-markup%2Fscripts%2Funderover-parameters-3.html Diff: --- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3-actual.txt @@ -1,10 +1,10 @@ PASS Baseline alignment PASS Heights of bases -PASS AccentBaseHeight, UnderbarExtraDescender -PASS AccentBaseHeight, UnderbarVerticalGap -PASS AccentBaseHeight, OverbarExtraAscender -PASS AccentBaseHeight, OverbarVerticalGap +FAIL AccentBaseHeight, UnderbarExtraDescender assert_approx_equals: munderover: accent over short base expected 40 +/- 2 but got 30.5 +FAIL AccentBaseHeight, UnderbarVerticalGap assert_approx_equals: munderover: accent over short base expected 40 +/- 2 but got 30.5 +FAIL AccentBaseHeight, OverbarExtraAscender assert_approx_equals: mover: accent over short base expected 40 +/- 2 but got 30.5 +FAIL AccentBaseHeight, OverbarVerticalGap assert_approx_equals: mover: nonaccent over short base expected 140 +/- 2 but got 31.5 It looks like this test began failing around r252125
<rdar://problem/57880452>
Due to a lack of builds to test there is a regression range of r252087-r252115
marked test as failing while investigating in https://trac.webkit.org/changeset/253434/webkit
Fred, do you have any idea what might be the issue?
This test involves loading web fonts with specific math parameters. Not sure why the flakiness happens only on macOS WK1. Might be related to https://github.com/web-platform-tests/wpt/pull/18937 and https://bugs.webkit.org/show_bug.cgi?id=174030
Hm... I can't reproduce this with: ./Tools/Scripts/run-webkit-tests --release -1 --iterations 100 --exit-after-n-failures 1 imported/w3c/web-platform-tests/mathml/presentation-markup/
I've tried 100 iterations on Mac mini with debug build but can't reproduce it either.
Now I have access to a Mac mini on which the flakiness reproduces.
There is a race condition between when Web fonts start loading and when load event is dispatched on window. Specifically, the failure happens when the layout which triggers font loading happens after window’s load event had been dispatched. Now I'm figuring out why that happens. In theory, we should be updating the layout at least once before load event fires.
Created attachment 388129 [details] WIP
Alright, updating the layout before deciding whether load event needs to be fired or not solves this problem. This is a bit problematic from perf perspective though because we'd end up triggering new layout whenever a sub resource finishes loading. We probably need to move it to right before document's ready state changes. I'd also have to check this behavior is mandated by the specification or not.
Do we need to force layout synchronously, or we can we just trigger a rendering update?
Hm... there is something strange here. Even if "load" event on the window were to fire before fonts load, "document.fonts" should be updating the style and trigger font loads.
Comment on attachment 388129 [details] WIP Can't land this or any variant. It would be too risky & affects too many things at least for now.
Finally I know why this is happening. The issue that when FontFaceSet’s constructor is invoked, it could immediately resolve its promise as long as the first layout has been updated. This is problematic since the first layout may happen way before the document finishes loading. Youeen added this code in https://bugs.webkit.org/show_bug.cgi?id=174030 in order to address the even more aggressive behavior we once had but this is racy & inadaquete for the purpose of satisfying the following condition in the specification: In fact, the bug which landed this fix mentions that there were previously a WebKit specific fix for this: https://bugs.webkit.org/show_bug.cgi?id=174030#c15
Created attachment 388963 [details] WIP2
Created attachment 389088 [details] Fixes the bug and add a test
Comment on attachment 389088 [details] Fixes the bug and add a test View in context: https://bugs.webkit.org/attachment.cgi?id=389088&action=review > LayoutTests/fast/css/font-face-set-ready-after-document-load-expected.txt:1 > +This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved. events* > LayoutTests/fast/css/font-face-set-ready-after-document-load.html:13 > +<p>This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved.<br> events*
Comment on attachment 389088 [details] Fixes the bug and add a test View in context: https://bugs.webkit.org/attachment.cgi?id=389088&action=review So WebKit doesn't have a way to deal with the case where onload has fired and you add an stylesheet with @font-face to the document, right? Blink and Gecko do deal with that, thought I'm not a fan of the forced layout, it seems like WebKit behavior is somewhat simpler. Though I believe such a behavior is required by the spec per https://drafts.csswg.org/css-font-loading/#ref-for-dom-fontfaceset-readypromise-slot%E2%91%A0 It doesn't seem like WebKit implements such a thing when new font loads are triggered. > Source/WebCore/css/FontFaceSet.h:122 > + bool m_isDocumentLoaded { true }; This looks like it'd deserve a comment on why it's true by default... If there's no frame() there's no layout to be done, but presumably the document does load?
(In reply to Emilio Cobos Álvarez (:emilio) from comment #19) > Comment on attachment 389088 [details] > Fixes the bug and add a test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389088&action=review > > So WebKit doesn't have a way to deal with the case where onload has fired > and you add an stylesheet with @font-face to the document, right? > > Blink and Gecko do deal with that, thought I'm not a fan of the forced > layout, it seems like WebKit behavior is somewhat simpler. > > Though I believe such a behavior is required by the spec per > https://drafts.csswg.org/css-font-loading/#ref-for-dom-fontfaceset- > readypromise-slot%E2%91%A0 > > It doesn't seem like WebKit implements such a thing when new font loads are > triggered. As far as I can tell, no. But the spec is super vague about things so it's unclear. > > Source/WebCore/css/FontFaceSet.h:122 > > + bool m_isDocumentLoaded { true }; > > This looks like it'd deserve a comment on why it's true by default... If > there's no frame() there's no layout to be done, but presumably the document > does load? I'm gonna leave it as is since I don't think I understand this code enough to provide a good / useful comment.
Created attachment 389199 [details] Patch for landing
Comment on attachment 389199 [details] Patch for landing Wait for EWS.
The commit-queue encountered the following flaky tests while processing attachment 389199 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Committed r255414: <https://trac.webkit.org/changeset/255414>
Committed r255483: <https://trac.webkit.org/changeset/255483>