| Summary: | document.fonts.ready is resolved too quickly | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dima Voytenko <dvoytenko> | ||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | andreas, commit-queue, dbarton, dino, ews-watchlist, fred.wang, graouts, koivisto, mmaxfield, philip, rbuis, rniwa, thorton, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
| Version: | Safari Technology Preview | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=158884 https://bugs.webkit.org/show_bug.cgi?id=225728 |
||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||
| Bug Blocks: | 179110, 205168 | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Dima Voytenko
2017-06-30 11:05:59 PDT
I'm not sure, but maybe it's related to the changes in bug 158884. @Dima: Can you elaborate how your test case is/was working? I see that with the release version of Safari, the "FONTS READY" message appeared just after the "START" message. However, with Epiphany and Safari Technology Preview it appears just after the "readystatechange" and before the "window.onload" one. That seems a different from Gecko/Chromium (where it appears at the really end) but is better. So I guess the bug you initially reported is FIXED? Created attachment 336117 [details] Patch to exhibit similar early-resolution bugs For WPT tests using Web fonts, the issue was worked around in https://github.com/w3c/web-platform-tests/pull/10025 ; The attached patch demonstrates the issue when document.fonts.ready is only called after window.onload (contrary to the initial test case): * frac-parameters-1.html uses CSS @font-face ; document.fonts.ready resolves immediately after window.onload and hence the test fails unless you delay a bit the check with e.g; requestAnimationFrame or setTimeout(..., 0) * In frac-parameters-2.html, the patch uses the JS API to add the font after window.onload, but still document.fonts.ready is resolved immediately. * In root-parameters-1.html, the same is done but the font load is triggered explicitly. This allows to delay the resolution of document.fonts.ready and the tests pass. I think WebKit does not really follow closely the spec but at the same time the spec is not always clear either. More analysis and conformance tests would be needed. Comment on attachment 336117 [details] Patch to exhibit similar early-resolution bugs Attachment 336117 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7038441 New failing tests: imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-2.html Created attachment 336118 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 336117 [details] Patch to exhibit similar early-resolution bugs Attachment 336117 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7038445 New failing tests: imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-2.html Created attachment 336119 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 336117 [details] Patch to exhibit similar early-resolution bugs Attachment 336117 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7038470 New failing tests: imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-2.html Created attachment 336123 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 336117 [details] Patch to exhibit similar early-resolution bugs Attachment 336117 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7038468 New failing tests: imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/fractions/frac-parameters-2.html Created attachment 336124 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
It appears that my repro page (http://output.jsbin.com/jexixam/quiet) works fine now on Safari Preview. But it also looks like some web platform tests are still failing. I've run into this as well in web-platform-tests, and have created a test that distills what many tests do and that won't work in Safari with web fonts: https://github.com/web-platform-tests/wpt/pull/18489 I'm not sure if this is really the same bug as in http://output.jsbin.com/jexixam/quiet, but it does fit the same description: document.fonts.ready is resolved too quickly This would cause thousands of tests to fail in Safari if we can't find a workaround. FWIW, MathML WPT tests requiring web fonts rely on the following workaround:
window.addEventListener("load", function() {
// Delay the check to workaround WebKit's bug https://webkit.org/b/174030.
requestAnimationFrame(() => { document.fonts.ready.then(runTests); });
});
Created attachment 376494 [details]
Patch
(In reply to Philip Jägenstedt from comment #14) > I've run into this as well in web-platform-tests, and have created a test > that distills what many tests do and that won't work in Safari with web > fonts: > https://github.com/web-platform-tests/wpt/pull/18489 Do you plan landing this PR? Looking at https://drafts.csswg.org/css-font-loading/#font-face-set-ready, the spec is not very precise in how to implement ready promise resolution. fast/text/font-face-set-javascript.html is timing out, probably due to the change of behavior. Created attachment 376497 [details]
Patch
Created attachment 376677 [details]
Patch
Comment on attachment 376677 [details] Patch Attachment 376677 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12939994 Number of test failures exceeded the failure limit. Created attachment 376680 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 376685 [details]
Patch
Comment on attachment 376685 [details] Patch Attachment 376685 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12941183 New failing tests: imported/w3c/web-platform-tests/mathml/presentation-markup/radicals/root-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-2.html imported/w3c/web-platform-tests/mathml/presentation-markup/tables/table-axis-height.html imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-4.html imported/w3c/web-platform-tests/mathml/relations/css-styling/displaystyle-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/operators/mo-axis-height-1.html imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/subsup-parameters-2.html Created attachment 376689 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 376700 [details]
Patch
Comment on attachment 376700 [details]
Patch
The name FontFaceSet::firstLayoutDone() was a bit confusing since it is actually a callback after the first layout, while FrameLoaderStateMachine::firstLayoutDone() has the same name but is actually a getter to check whether the first layout is done.
Not sure I have a better suggestion though. Maybe rename one of them e.g. FrameLoaderStateMachine::isFirstLayoutDone() or FontFaceSet::didFirstLayout()?
In any case, LGTM thanks for working on this!
Thanks for the review. (In reply to Frédéric Wang (:fredw) from comment #27) > Comment on attachment 376700 [details] > Patch > > The name FontFaceSet::firstLayoutDone() was a bit confusing since it is > actually a callback after the first layout, while > FrameLoaderStateMachine::firstLayoutDone() has the same name but is actually > a getter to check whether the first layout is done. > > Not sure I have a better suggestion though. Maybe rename one of them e.g. > FrameLoaderStateMachine::isFirstLayoutDone() or > FontFaceSet::didFirstLayout()? didFirstLayout seems good to me. Created attachment 377577 [details]
Patch for landing
Created attachment 377586 [details]
Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 377586 [details]: imported/w3c/web-platform-tests/xhr/event-error-order.sub.html bug 192363 (authors: cdumez@apple.com and youennf@gmail.com) The commit-queue is continuing to process your patch. The commit-queue encountered the following flaky tests while processing attachment 377586 [details]:
The commit-queue is continuing to process your patch.
Comment on attachment 377586 [details] Patch for landing Clearing flags on attachment: 377586 Committed r249295: <https://trac.webkit.org/changeset/249295> All reviewed patches have been landed. Closing bug. Comment on attachment 377586 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=377586&action=review > Source/WebCore/css/FontFaceSet.cpp:71 > + m_isFirstLayoutDone = document.frame()->loader().stateMachine().firstLayoutDone(); This fix is not complete. The specification says that we must wait until the document is no longer loading: https://drafts.csswg.org/css-font-loading/#fontfaceset-pending-on-the-environment The first layout may be done whenever the parser yields & we decide to paint so it's not a sufficient signal to determine that we're done loading a document. Fixing the race condition in https://bugs.webkit.org/show_bug.cgi?id=205168. This bug doesn't seem to be fixed. When the page is loaded with emptied caches the promise will still resolve too early. The requestAnimationFrame() workaround is still necessary for the first time a page is loaded. (In reply to Andreas from comment #37) > This bug doesn't seem to be fixed. > When the page is loaded with emptied caches the promise will still resolve > too early. > The requestAnimationFrame() workaround is still necessary for the first time > a page is loaded. Interesting. Could you file a new bug with a reduced test case? (In reply to Ryosuke Niwa from comment #38) > (In reply to Andreas from comment #37) > > This bug doesn't seem to be fixed. > > When the page is loaded with emptied caches the promise will still resolve > > too early. > > The requestAnimationFrame() workaround is still necessary for the first time > > a page is loaded. > > Interesting. Could you file a new bug with a reduced test case? Please cc me, Youenn, and Myles on the new bug if you file one. Thanks! |