Bug 158884 - [Font Loading] The callback passed to document.fonts.ready should always be called
Summary: [Font Loading] The callback passed to document.fonts.ready should always be c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
: 158993 (view as bug list)
Depends on: 136502 153348 159565
Blocks: 153346 133567 133845 135390 153742 153918 155638 155639 155714
  Show dependency treegraph
 
Reported: 2016-06-17 13:38 PDT by Frédéric Wang (:fredw)
Modified: 2016-07-08 12:41 PDT (History)
7 users (show)

See Also:


Attachments
testcase (632 bytes, text/html)
2016-06-17 13:38 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (844 bytes, patch)
2016-06-18 03:21 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Needs tests (1.76 KB, patch)
2016-07-07 14:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.90 KB, patch)
2016-07-07 15:08 PDT, Myles C. Maxfield
dino: review+
Details | Formatted Diff | Diff
Patch for committing (4.70 KB, patch)
2016-07-07 16:03 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (4.69 KB, patch)
2016-07-07 16:05 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-06-17 13:38:33 PDT
Created attachment 281583 [details]
testcase

Per the CSS Font Loading working draft (https://drafts.csswg.org/css-font-loading/#font-face-set-ready):

"This is similar to listening for a loadingdone event to fire, but the callbacks passed to the ready promise will always get called, even when no font loads occur because the fonts in question are already loaded."

It seems that contrary to Chrome or Firefox, WebKit does not always call that callback, as shown by the simple attached testcase.

----

More context: we are extending the support for OpenType MATH table during the MathML refactoring and we are importing tests from http://tests.mathml-association.org/ that use custom web fonts to check all these OpenType features. These tests wait for the page load (so that the font loading requests will have started or even be done) and possibly wait again for the web font loading to complete as follows:

window.addEventListener("load", function() {
   document.fonts.ready.then(runTests);
});

This works in Gecko but not in WebKit. I tried calling document.fonts.ready before (e.g. at "DOMContentLoaded" or immediately) which may give better result in WebKit although it still does not always work and sometimes it's causing random failures on Gecko too. I suspect that the problem is the following: If the fonts loading request has not started then Gecko will call runTests immediately (per the spec) and so too early while if the fonts loading request is complete then WebKit will never call runTests (per the present bug) ; so one has to call document.fonts.ready.then exactly while the web fonts are loading (which is not easy since these web fonts are loaded via standard CSS rules rather than with Javascript).

I'm making this bug blocking some other MathML bugs, although for now it is still possible to tweak the tests with some setTimeOut or similar hacks.
Comment 1 Frédéric Wang (:fredw) 2016-06-18 03:21:28 PDT
Created attachment 281615 [details]
Patch

This is a simple patch to set m_isReady to false iff no fonts are loading. That obviously fixes the trivial test case and that seems to give better result on http://tests.mathml-association.org/ when I apply it on the MathML refactoring branch (although there are still some random failures). But I'm not actually sure this patch is quite correct and I didn't find any tests for document.fonts.ready in LayoutTests...

I'd like to highlight this additional comment from the font loading draft: "The ready promise is only fulfilled *after layout operations complete* and no additional font loads are necessary." (emphasis is mine).
Comment 2 Frédéric Wang (:fredw) 2016-06-18 03:29:36 PDT
All but the SuperscriptShiftUpCramped test (bug 156401) should pass in WebKit trunk build:

http://tests.mathml-association.org/mathml/presentation-markup/scripts/subsup-parameters-1.html

At the moment it times out because of this bug. With the patch, it does not time out but test success only seems to happen sometimes when I reload the page.
Comment 3 Alexey Proskuryakov 2016-06-21 11:59:11 PDT
*** Bug 158993 has been marked as a duplicate of this bug. ***
Comment 4 Alexey Proskuryakov 2016-06-21 11:59:53 PDT
Also reproducible with steps from bug 158993.
Comment 5 Myles C. Maxfield 2016-07-07 14:32:34 PDT
Created attachment 283050 [details]
Needs tests
Comment 6 Myles C. Maxfield 2016-07-07 14:33:27 PDT
Wow, haha, it looks like I uploaded the same patch as yours! Sorry about that!
Comment 7 Myles C. Maxfield 2016-07-07 14:33:44 PDT
I'll update the ChangeLog to attribute both of us :)
Comment 8 Myles C. Maxfield 2016-07-07 15:08:41 PDT
Created attachment 283061 [details]
Patch
Comment 9 Dean Jackson 2016-07-07 15:12:54 PDT
Comment on attachment 283061 [details]
Patch

[08:10:33]  <dino>	myles: i didn't comment on the coding style violations in the layout test
[08:10:38]  <dino>	bad indent
[08:10:50]  <dino>	single line conditionals
[08:11:20]  <dino>	no need for meta
[08:11:31]  <dino>	no need for body
[08:11:35]  <dino>	or head
[08:11:45]  <dino>	could be just <script src> <script> <script src>
Comment 10 Myles C. Maxfield 2016-07-07 16:03:51 PDT
Created attachment 283077 [details]
Patch for committing
Comment 11 Myles C. Maxfield 2016-07-07 16:05:04 PDT
Created attachment 283079 [details]
Patch for committing
Comment 12 WebKit Commit Bot 2016-07-07 16:34:48 PDT
Comment on attachment 283079 [details]
Patch for committing

Clearing flags on attachment: 283079

Committed r202945: <http://trac.webkit.org/changeset/202945>
Comment 13 Alexey Proskuryakov 2016-07-07 17:20:44 PDT
> [08:10:33]  <dino>	myles: i didn't comment on the coding style violations in the layout test

I agree that having some weirdness in layout tests is good. It is quite common that tests discover issues that weren't the reason why they were written.
Comment 14 Frédéric Wang (:fredw) 2016-07-08 02:14:39 PDT
FYI, this new test as well as the MathML ones relying on fonts.ready are still flacky.
Comment 15 WebKit Commit Bot 2016-07-08 09:36:07 PDT
Re-opened since this is blocked by bug 159565
Comment 16 Ryan Haddad 2016-07-08 09:44:31 PDT
(In reply to comment #15)
> Re-opened since this is blocked by bug 159565

Rolled out in https://trac.webkit.org/r202983. The test for this change fails on every platform:

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r202981%20(7478)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Ffont-face-set-ready-fire.html

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/text/font-face-set-ready-fire-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/text/font-face-set-ready-fire-actual.txt
@@ -4,7 +4,7 @@
 
 
 PASS document.fonts.status is "loaded"
-PASS document.fonts.status is "loading"
+FAIL document.fonts.status should be loading. Was loaded.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 17 Frédéric Wang (:fredw) 2016-07-08 10:35:05 PDT
Note that many MathML tests I landed today may require the C++ change in this patch to detect when required web fonts are loaded. If they become flaky because of the rollout, please just change the TestExpectations. Thanks.
Comment 18 Ryan Haddad 2016-07-08 10:44:04 PDT
(In reply to comment #17)
> Note that many MathML tests I landed today may require the C++ change in
> this patch to detect when required web fonts are loaded. If they become
> flaky because of the rollout, please just change the TestExpectations.
> Thanks.

Yep, sure enough, 5 did start timing out. Updated expectations in <https://trac.webkit.org/r202988>
Comment 19 Myles C. Maxfield 2016-07-08 11:38:36 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Re-opened since this is blocked by bug 159565
> 
> Rolled out in https://trac.webkit.org/r202983. The test for this change
> fails on every platform:
> 
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r202981%20(7478)/results.html
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Ftext%2Ffont-face-set-ready-fire.html
> 
> ---
> /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/
> fast/text/font-face-set-ready-fire-expected.txt
> +++
> /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/
> fast/text/font-face-set-ready-fire-actual.txt
> @@ -4,7 +4,7 @@
>  
>  
>  PASS document.fonts.status is "loaded"
> -PASS document.fonts.status is "loading"
> +FAIL document.fonts.status should be loading. Was loaded.
>  PASS successfullyParsed is true
>  
>  TEST COMPLETE

Yeah, this is just because I didn't clear the cache at the beginning of the test. I thought it might be necessary; I should have made sure. Updating and rolling back in now.
Comment 20 Myles C. Maxfield 2016-07-08 12:41:03 PDT
Committed r203002: <http://trac.webkit.org/changeset/203002>