Bug 158884 – [Font Loading] The callback passed to document.fonts.ready should always be called

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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.

[Alexey Proskuryakov]

*** Bug 158993 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Also reproducible with steps from bug 158993.

[Myles C. Maxfield]

Created attachment 283050 [details]
Needs tests

[Myles C. Maxfield]

Wow, haha, it looks like I uploaded the same patch as yours! Sorry about that!

[Myles C. Maxfield]

I'll update the ChangeLog to attribute both of us :)

[Myles C. Maxfield]

Created attachment 283061 [details]
Patch

[Dean Jackson]

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>

[Myles C. Maxfield]

Created attachment 283077 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 283079 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 283079 [details]
Patch for committing

Clearing flags on attachment: 283079

Committed r202945: <http://trac.webkit.org/changeset/202945>

[Alexey Proskuryakov]

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

[Frédéric Wang (:fredw)]

FYI, this new test as well as the MathML ones relying on fonts.ready are still flacky.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159565

[Ryan Haddad]

(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

[Frédéric Wang (:fredw)]

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.

[Ryan Haddad]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Committed r203002: <http://trac.webkit.org/changeset/203002>