Summary: | [Font Loading] The callback passed to document.fonts.ready should always be called | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | alex, ap, bfulgham, commit-queue, jonlee, mmaxfield, ryanhaddad | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=174030 https://bugs.webkit.org/show_bug.cgi?id=225728 |
||||||||||||||||
Bug Depends on: | 136502, 153348, 159565 | ||||||||||||||||
Bug Blocks: | 153346, 133567, 133845, 135390, 153742, 153918, 155638, 155639, 155714 | ||||||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2016-06-17 13:38:33 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). 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. *** Bug 158993 has been marked as a duplicate of this bug. *** Also reproducible with steps from bug 158993. Created attachment 283050 [details]
Needs tests
Wow, haha, it looks like I uploaded the same patch as yours! Sorry about that! I'll update the ChangeLog to attribute both of us :) Created attachment 283061 [details]
Patch
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>
Created attachment 283077 [details]
Patch for committing
Created attachment 283079 [details]
Patch for committing
Comment on attachment 283079 [details] Patch for committing Clearing flags on attachment: 283079 Committed r202945: <http://trac.webkit.org/changeset/202945> > [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.
FYI, this new test as well as the MathML ones relying on fonts.ready are still flacky. Re-opened since this is blocked by bug 159565 (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 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. (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> (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. Committed r203002: <http://trac.webkit.org/changeset/203002> |