Bug 174030 - document.fonts.ready is resolved too quickly
Summary: document.fonts.ready is resolved too quickly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 179110
  Show dependency treegraph
 
Reported: 2017-06-30 11:05 PDT by Dima Voytenko
Modified: 2019-08-29 15:02 PDT (History)
14 users (show)

See Also:


Attachments
Patch to exhibit similar early-resolution bugs (3.33 KB, patch)
2018-03-20 06:36 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.33 MB, application/zip)
2018-03-20 07:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.73 MB, application/zip)
2018-03-20 07:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.23 MB, application/zip)
2018-03-20 08:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.00 MB, application/zip)
2018-03-20 08:15 PDT, Build Bot
no flags Details
Patch (7.64 KB, patch)
2019-08-16 05:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (72.34 KB, patch)
2019-08-16 07:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.99 KB, patch)
2019-08-19 02:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.64 MB, application/zip)
2019-08-19 03:31 PDT, Build Bot
no flags Details
Patch (9.27 KB, patch)
2019-08-19 08:13 PDT, youenn fablet
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.26 MB, application/zip)
2019-08-19 09:25 PDT, Build Bot
no flags Details
Patch (9.31 KB, patch)
2019-08-19 11:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.15 KB, patch)
2019-08-29 06:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (9.29 KB, patch)
2019-08-29 08:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2017-06-30 11:05:59 PDT
The promise `document.fonts.ready` is resolved immediately even before any of the initially used fonts have been resolved. This behavior appears to contradict the spec and other browsers.

See http://output.jsbin.com/jexixam/quiet
Comment 1 Radar WebKit Bug Importer 2017-06-30 13:17:43 PDT
<rdar://problem/33083550>
Comment 2 Frédéric Wang (:fredw) 2017-11-28 08:05:49 PST
I'm not sure, but maybe it's related to the changes in bug 158884.
Comment 3 Frédéric Wang (:fredw) 2018-03-20 06:34:03 PDT
@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?
Comment 4 Frédéric Wang (:fredw) 2018-03-20 06:36:29 PDT
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 5 Build Bot 2018-03-20 07:39:39 PDT
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
Comment 6 Build Bot 2018-03-20 07:39:40 PDT
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 7 Build Bot 2018-03-20 07:45:34 PDT
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
Comment 8 Build Bot 2018-03-20 07:45:35 PDT
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 9 Build Bot 2018-03-20 08:11:02 PDT
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
Comment 10 Build Bot 2018-03-20 08:11:04 PDT
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 11 Build Bot 2018-03-20 08:15:23 PDT
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
Comment 12 Build Bot 2018-03-20 08:15:25 PDT
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
Comment 13 Dima Voytenko 2018-03-21 08:55:47 PDT
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.
Comment 14 Philip Jägenstedt 2019-08-16 00:46:10 PDT
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.
Comment 15 Frédéric Wang (:fredw) 2019-08-16 00:53:28 PDT
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); });
  });
Comment 16 youenn fablet 2019-08-16 05:53:45 PDT
Created attachment 376494 [details]
Patch
Comment 17 youenn fablet 2019-08-16 05:54:57 PDT
(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.
Comment 18 youenn fablet 2019-08-16 06:36:16 PDT
fast/text/font-face-set-javascript.html is timing out, probably due to the change of behavior.
Comment 19 youenn fablet 2019-08-16 07:03:41 PDT
Created attachment 376497 [details]
Patch
Comment 20 youenn fablet 2019-08-19 02:10:36 PDT
Created attachment 376677 [details]
Patch
Comment 21 Build Bot 2019-08-19 03:31:39 PDT
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.
Comment 22 Build Bot 2019-08-19 03:31:41 PDT
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
Comment 23 youenn fablet 2019-08-19 08:13:52 PDT
Created attachment 376685 [details]
Patch
Comment 24 Build Bot 2019-08-19 09:24:59 PDT
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
Comment 25 Build Bot 2019-08-19 09:25:01 PDT
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
Comment 26 youenn fablet 2019-08-19 11:48:32 PDT
Created attachment 376700 [details]
Patch
Comment 27 Frédéric Wang (:fredw) 2019-08-29 01:55:20 PDT
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!
Comment 28 youenn fablet 2019-08-29 06:47:00 PDT
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.
Comment 29 youenn fablet 2019-08-29 06:54:18 PDT
Created attachment 377577 [details]
Patch for landing
Comment 30 youenn fablet 2019-08-29 08:22:34 PDT
Created attachment 377586 [details]
Patch for landing
Comment 31 WebKit Commit Bot 2019-08-29 11:26:35 PDT
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.
Comment 32 WebKit Commit Bot 2019-08-29 11:26:36 PDT
The commit-queue encountered the following flaky tests while processing attachment 377586 [details]:

The commit-queue is continuing to process your patch.
Comment 33 WebKit Commit Bot 2019-08-29 15:01:59 PDT
Comment on attachment 377586 [details]
Patch for landing

Clearing flags on attachment: 377586

Committed r249295: <https://trac.webkit.org/changeset/249295>
Comment 34 WebKit Commit Bot 2019-08-29 15:02:02 PDT
All reviewed patches have been landed.  Closing bug.