font-display:fallback can cause a visual flash (which is supposed to be impossible)
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 00:44:49 +0000 (00:44 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 00:44:49 +0000 (00:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181374

Reviewed by Simon Fraser.

Source/WebCore:

A FontCascade represents an entire font-family fallback list, but sometimes we need to pull out a single
representative font from the list to calculate things like line height. Previously, if the first item in
the font-family list was in the middle of being downloaded, this representative font was hardcoded to be
Times. However, when actually laying out and drawing the glyphs, we have logic to skip the interstitial
Times if there are any installed fonts present in the font-family list (so you wouldn't ever actually
see Times). This means that line height (among other things) was being calculated as if Times was used,
but in reality, some other font from the font-family list was being used.

Alone, this isn't a huge problem, but font-display:fallback makes a font transition between "timed out"
and "failed," and when the font hits the failed state, the representative font skips over the cancelled
item and hits the next item in the fallback list. This means that line heights will change, which causes
a visual flash, even when font-display:fallback is specified.

The solution is simply to educate the logic which identifies this representative font so that it
understands what to do for currently-loading fonts.

Tests: fast/text/font-display/swap-flash.html

* platform/graphics/FontCascadeFonts.h:
(WebCore::FontCascadeFonts::primaryFont):
* rendering/line/BreakingContext.h:
(WebCore::textWidth):

Tools:

The test requires Palatino.

* DumpRenderTree/mac/DumpRenderTree.mm:
(allowedFontFamilySet):
* WebKitTestRunner/mac/TestControllerMac.mm:
(WTR::allowedFontFamilySet):

LayoutTests:

Move font-display tests into their common subfolder.

* fast/text/font-display/block-finish-expected.html: Renamed from LayoutTests/fast/text/loading-block-finish-expected.html.
* fast/text/font-display/block-finish.html: Renamed from LayoutTests/fast/text/loading-block-finish.html.
* fast/text/font-display/block-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-block-nofinish-expected.html.
* fast/text/font-display/block-nofinish.html: Renamed from LayoutTests/fast/text/loading-block-nofinish.html.
* fast/text/font-display/failure-finish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-finish-expected.html.
* fast/text/font-display/failure-finish.html: Renamed from LayoutTests/fast/text/loading-failure-finish.html.
* fast/text/font-display/failure-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish-expected.html.
* fast/text/font-display/failure-nofinish.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish.html.
* fast/text/font-display/swap-finish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-finish-expected.html.
* fast/text/font-display/swap-finish.html: Renamed from LayoutTests/fast/text/loading-swap-finish.html.
* fast/text/font-display/swap-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish-expected.html.
* fast/text/font-display/swap-nofinish.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish.html.
* fast/text/font-display/swap-flash-expected.html: Added.
* fast/text/font-display/swap-flash.html: Added.
* platform/win/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226668 268f45cc-cd09-0410-ab3c-d52691b4dbfc

23 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/text/font-display/block-finish-expected.html [moved from LayoutTests/fast/text/loading-block-finish-expected.html with 84% similarity]
LayoutTests/fast/text/font-display/block-finish.html [moved from LayoutTests/fast/text/loading-block-finish.html with 88% similarity]
LayoutTests/fast/text/font-display/block-nofinish-expected.html [moved from LayoutTests/fast/text/loading-block-nofinish-expected.html with 81% similarity]
LayoutTests/fast/text/font-display/block-nofinish.html [moved from LayoutTests/fast/text/loading-block-nofinish.html with 88% similarity]
LayoutTests/fast/text/font-display/failure-finish-expected.html [moved from LayoutTests/fast/text/loading-failure-finish-expected.html with 100% similarity]
LayoutTests/fast/text/font-display/failure-finish.html [moved from LayoutTests/fast/text/loading-failure-finish.html with 88% similarity]
LayoutTests/fast/text/font-display/failure-nofinish-expected.html [moved from LayoutTests/fast/text/loading-failure-nofinish-expected.html with 100% similarity]
LayoutTests/fast/text/font-display/failure-nofinish.html [moved from LayoutTests/fast/text/loading-failure-nofinish.html with 88% similarity]
LayoutTests/fast/text/font-display/swap-finish-expected.html [moved from LayoutTests/fast/text/loading-swap-finish-expected.html with 84% similarity]
LayoutTests/fast/text/font-display/swap-finish.html [moved from LayoutTests/fast/text/loading-swap-finish.html with 88% similarity]
LayoutTests/fast/text/font-display/swap-flash-expected.html [new file with mode: 0644]
LayoutTests/fast/text/font-display/swap-flash.html [new file with mode: 0644]
LayoutTests/fast/text/font-display/swap-nofinish-expected.html [moved from LayoutTests/fast/text/loading-swap-nofinish-expected.html with 100% similarity]
LayoutTests/fast/text/font-display/swap-nofinish.html [moved from LayoutTests/fast/text/loading-swap-nofinish.html with 90% similarity]
LayoutTests/platform/win/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/FontCascadeFonts.h
Source/WebCore/platform/graphics/FontRanges.h
Source/WebCore/rendering/line/BreakingContext.h
Tools/ChangeLog
Tools/DumpRenderTree/mac/DumpRenderTree.mm
Tools/WebKitTestRunner/mac/TestControllerMac.mm

index 2c098bba4fdfc02254cbe5135d1270fac245444f..19dbc5a93cedde055191a31bff4ce2f640d130ba 100644 (file)
@@ -1,3 +1,28 @@
+2018-01-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        font-display:fallback can cause a visual flash (which is supposed to be impossible)
+        https://bugs.webkit.org/show_bug.cgi?id=181374
+
+        Reviewed by Simon Fraser.
+
+        Move font-display tests into their common subfolder.
+
+        * fast/text/font-display/block-finish-expected.html: Renamed from LayoutTests/fast/text/loading-block-finish-expected.html.
+        * fast/text/font-display/block-finish.html: Renamed from LayoutTests/fast/text/loading-block-finish.html.
+        * fast/text/font-display/block-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-block-nofinish-expected.html.
+        * fast/text/font-display/block-nofinish.html: Renamed from LayoutTests/fast/text/loading-block-nofinish.html.
+        * fast/text/font-display/failure-finish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-finish-expected.html.
+        * fast/text/font-display/failure-finish.html: Renamed from LayoutTests/fast/text/loading-failure-finish.html.
+        * fast/text/font-display/failure-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish-expected.html.
+        * fast/text/font-display/failure-nofinish.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish.html.
+        * fast/text/font-display/swap-finish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-finish-expected.html.
+        * fast/text/font-display/swap-finish.html: Renamed from LayoutTests/fast/text/loading-swap-finish.html.
+        * fast/text/font-display/swap-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish-expected.html.
+        * fast/text/font-display/swap-nofinish.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish.html.
+        * fast/text/font-display/swap-flash-expected.html: Added.
+        * fast/text/font-display/swap-flash.html: Added.
+        * platform/win/TestExpectations:
+
 2018-01-09  Matt Lewis  <jlewis3@apple.com>
 
         Fixed test expectaions.
similarity index 84%
rename from LayoutTests/fast/text/loading-block-finish-expected.html
rename to LayoutTests/fast/text/font-display/block-finish-expected.html
index a2efaa4c56c201cec9e4ef6d8088a53a14e42ae2..3117695bf2e53351a7b5fd939175c97b0741957e 100644 (file)
@@ -4,7 +4,7 @@
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 88%
rename from LayoutTests/fast/text/loading-block-finish.html
rename to LayoutTests/fast/text/font-display/block-finish.html
index c7f40c0161e298f19ccd5ea6f015ec73807cc1e2..62df99c16c336b6f0197b2202de07aa4f2578bc2 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 81%
rename from LayoutTests/fast/text/loading-block-nofinish-expected.html
rename to LayoutTests/fast/text/font-display/block-nofinish-expected.html
index 8d8673c512aea540807e1e64bdbce653ef04836c..670feea6ceb606c3023fbb102793a8b6c37f16dc 100644 (file)
@@ -4,7 +4,7 @@
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 88%
rename from LayoutTests/fast/text/loading-block-nofinish.html
rename to LayoutTests/fast/text/font-display/block-nofinish.html
index bf605e5498a755b5a5005d71f1114cedd3a51f4d..29cc9bad5a0ebaa612fc7ec1273701854522fd14 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 88%
rename from LayoutTests/fast/text/loading-failure-finish.html
rename to LayoutTests/fast/text/font-display/failure-finish.html
index 894b14924429ab2090060ce837c259610e687412..74b6b1784fe96d6beb1e7a8bbe2f04b3ca7789fe 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 88%
rename from LayoutTests/fast/text/loading-failure-nofinish.html
rename to LayoutTests/fast/text/font-display/failure-nofinish.html
index eff6b25bfabb2a1c1d1236843bdd169cbf1dc27f..b626b4bb28f3645bfe33c04cf103df4dc18581b7 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 84%
rename from LayoutTests/fast/text/loading-swap-finish-expected.html
rename to LayoutTests/fast/text/font-display/swap-finish-expected.html
index 3f2dbc75ee789ce789c3951726b84d6c36d2d866..03b5ac30ee473a42e9831fdeda5bcac7b5f09ea5 100644 (file)
@@ -4,7 +4,7 @@
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
similarity index 88%
rename from LayoutTests/fast/text/loading-swap-finish.html
rename to LayoutTests/fast/text/font-display/swap-finish.html
index 365ac0b72e172e9738c9572ffdc328dc62aa1288..c2e9f92008ecb89ad4035a7701a7835dda29808c 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
diff --git a/LayoutTests/fast/text/font-display/swap-flash-expected.html b/LayoutTests/fast/text/font-display/swap-flash-expected.html
new file mode 100644 (file)
index 0000000..149f3b3
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script>
+if (window.internals) {
+    internals.settings.setFontLoadTimingOverride("Failure");
+    internals.settings.setShouldIgnoreFontLoadCompletions(false);
+}
+</script>
+<style>
+@font-face {
+    font-family: "MyFont";
+    src: url("../../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+</head>
+<body>
+This test makes sure that text rendered in the "swap" period is identical to text rendered in the "failure" period. The test passes if the vertical position of the text below is identital (to the subpixel) between this file and the expected file.
+<div style="font-family: MyFont, Palatino; font-size: 24px; height: 30px; line-height: 28px;"><span style="font-family: MyFont, Palatino; font-size: 24px; line-height: 30px;">Descriptors</span></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/text/font-display/swap-flash.html b/LayoutTests/fast/text/font-display/swap-flash.html
new file mode 100644 (file)
index 0000000..2b4dab8
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script>
+if (window.internals) {
+    internals.settings.setFontLoadTimingOverride("Swap");
+    internals.settings.setShouldIgnoreFontLoadCompletions(true);
+}
+</script>
+<style>
+@font-face {
+    font-family: "MyFont";
+    src: url("../../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+</head>
+<body>
+This test makes sure that text rendered in the "swap" period is identical to text rendered in the "failure" period. The test passes if the vertical position of the text below is identital (to the subpixel) between this file and the expected file.
+<div style="font-family: MyFont, Palatino; font-size: 24px; height: 30px; line-height: 28px;"><span style="font-family: MyFont, Palatino; font-size: 24px; line-height: 30px;">Descriptors</span></div>
+</body>
+</html>
similarity index 90%
rename from LayoutTests/fast/text/loading-swap-nofinish.html
rename to LayoutTests/fast/text/font-display/swap-nofinish.html
index 51e7f9c947aa5b9047d4f0c9502f408bc8bf77a8..a435e35ee5177a6fef8576beb322696b83984794 100644 (file)
@@ -10,7 +10,7 @@ if (window.internals) {
 <style>
 @font-face {
     font-family: "MyFont";
-    src: url("../../resources/Ahem.ttf") format("truetype");
+    src: url("../../../resources/Ahem.ttf") format("truetype");
 }
 </style>
 </head>
index 48d04ceed7653bea285e3edde92fcc8d2e841f35..e6909c6cac0e25b44fec3aa3a44f080df214b424 100644 (file)
@@ -3789,10 +3789,10 @@ webkit.org/b/177626 fast/dom/HTMLLinkElement/preconnect-support.html  [ Skip ]
 webkit.org/b/177626 http/tests/preconnect/link-rel-preconnect-http.html [ Skip ]
 webkit.org/b/177626 http/tests/preconnect/link-rel-preconnect-https.html [ Skip ]
 
-webkit.org/b/177964 fast/text/loading-block-nofinish.html [ Pass ImageOnlyFailure ]
-webkit.org/b/177964 fast/text/loading-failure-finish.html [ Pass ImageOnlyFailure ]
-webkit.org/b/177964 fast/text/loading-failure-nofinish.html [ Pass ImageOnlyFailure ]
-webkit.org/b/177964 fast/text/loading-swap-nofinish.html [ Pass ImageOnlyFailure ]
+webkit.org/b/177964 fast/text/font-display/block-nofinish.html [ Pass ImageOnlyFailure ]
+webkit.org/b/177964 fast/text/font-display/failure-finish.html [ Pass ImageOnlyFailure ]
+webkit.org/b/177964 fast/text/font-display/failure-nofinish.html [ Pass ImageOnlyFailure ]
+webkit.org/b/177964 fast/text/font-display/swap-nofinish.html [ Pass ImageOnlyFailure ]
 
 # xhtml tests are failing on some of the EWS bots.
 webkit.org/b/178230 fast/xmlhttprequest/xmlhttprequest-get.xhtml [ Failure ]
index 7d4ef66d5a1787f77d846924e67b34bdb611fbbc..4a8aa189bff7675d6c6450891ccf563e0a23d603 100644 (file)
@@ -1,3 +1,33 @@
+2018-01-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        font-display:fallback can cause a visual flash (which is supposed to be impossible)
+        https://bugs.webkit.org/show_bug.cgi?id=181374
+
+        Reviewed by Simon Fraser.
+
+        A FontCascade represents an entire font-family fallback list, but sometimes we need to pull out a single
+        representative font from the list to calculate things like line height. Previously, if the first item in
+        the font-family list was in the middle of being downloaded, this representative font was hardcoded to be
+        Times. However, when actually laying out and drawing the glyphs, we have logic to skip the interstitial
+        Times if there are any installed fonts present in the font-family list (so you wouldn't ever actually
+        see Times). This means that line height (among other things) was being calculated as if Times was used,
+        but in reality, some other font from the font-family list was being used.
+
+        Alone, this isn't a huge problem, but font-display:fallback makes a font transition between "timed out"
+        and "failed," and when the font hits the failed state, the representative font skips over the cancelled
+        item and hits the next item in the fallback list. This means that line heights will change, which causes
+        a visual flash, even when font-display:fallback is specified.
+
+        The solution is simply to educate the logic which identifies this representative font so that it
+        understands what to do for currently-loading fonts.
+
+        Tests: fast/text/font-display/swap-flash.html
+
+        * platform/graphics/FontCascadeFonts.h:
+        (WebCore::FontCascadeFonts::primaryFont):
+        * rendering/line/BreakingContext.h:
+        (WebCore::textWidth):
+
 2018-01-04  Filip Pizlo  <fpizlo@apple.com>
 
         CodeBlocks should be in IsoSubspaces
index 9a8b54b6e39edd6c05bbba5bb8866c55dafcd9bf..70f7c56104256ae197c4d5000ed820cfaf98d5e7 100644 (file)
@@ -126,9 +126,21 @@ inline const Font& FontCascadeFonts::primaryFont(const FontCascadeDescription& d
     ASSERT(isMainThread());
     if (!m_cachedPrimaryFont) {
         auto& primaryRanges = realizeFallbackRangesAt(description, 0);
-        m_cachedPrimaryFont = primaryRanges.fontForCharacter(' ');
+        m_cachedPrimaryFont = primaryRanges.glyphDataForCharacter(' ', ExternalResourceDownloadPolicy::Allow).font;
         if (!m_cachedPrimaryFont)
             m_cachedPrimaryFont = &primaryRanges.fontForFirstRange();
+        else if (m_cachedPrimaryFont->isInterstitial()) {
+            for (unsigned index = 1; ; ++index) {
+                auto& localRanges = realizeFallbackRangesAt(description, index);
+                if (localRanges.isNull())
+                    break;
+                auto* font = localRanges.glyphDataForCharacter(' ', ExternalResourceDownloadPolicy::Forbid).font;
+                if (font && !font->isInterstitial()) {
+                    m_cachedPrimaryFont = font;
+                    break;
+                }
+            }
+        }
     }
     return *m_cachedPrimaryFont;
 }
index d35ea7c4c16af9c4ff80927d3237289988b52f56..d5577728690c07756214866aca3992de96dfe9b3 100644 (file)
@@ -83,7 +83,7 @@ public:
     unsigned size() const { return m_ranges.size(); }
     const Range& rangeAt(unsigned i) const { return m_ranges[i]; }
 
-    GlyphData glyphDataForCharacter(UChar32, ExternalResourceDownloadPolicy) const;
+    WEBCORE_EXPORT GlyphData glyphDataForCharacter(UChar32, ExternalResourceDownloadPolicy) const;
     WEBCORE_EXPORT const Font* fontForCharacter(UChar32) const;
     WEBCORE_EXPORT const Font& fontForFirstRange() const;
     bool isLoading() const;
index 7f1ba1c4e4a5c260ed68b440f3edd84792b9ad1b..17da8ffb349e14232d971ab546c5753c47a71ed2 100644 (file)
@@ -621,6 +621,7 @@ ALWAYS_INLINE float textWidth(RenderText& text, unsigned from, unsigned len, con
     const RenderStyle& style = text.style();
 
     GlyphOverflow glyphOverflow;
+    // FIXME: This is not the right level of abstraction for isFixedPitch. Font fallback may make it such that the fixed pitch font is never actually used!
     if (isFixedPitch || (!from && len == text.text().length()) || style.hasTextCombine())
         return text.width(from, len, font, xPos, &fallbackFonts, &glyphOverflow);
 
index f8adfc699e8b1bb19c64177d797acb01acf268d2..8936f0c835d99f6e8c02c55c20d89436336d6a80 100644 (file)
@@ -1,3 +1,17 @@
+2018-01-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        font-display:fallback can cause a visual flash (which is supposed to be impossible)
+        https://bugs.webkit.org/show_bug.cgi?id=181374
+
+        Reviewed by Simon Fraser.
+
+        The test requires Palatino.
+
+        * DumpRenderTree/mac/DumpRenderTree.mm:
+        (allowedFontFamilySet):
+        * WebKitTestRunner/mac/TestControllerMac.mm:
+        (WTR::allowedFontFamilySet):
+
 2018-01-09  Saam Barati  <sbarati@apple.com>
 
         Give some slack in display-profiler-outputs computation of the terminal window's number of columns
index 38ef2f848b083d5fa1d9c282313b8a263c232602..477cc1f30ee57fc680e9d5cdcf3f3ddb9bcb1994 100644 (file)
@@ -394,6 +394,7 @@ static NSSet *allowedFontFamilySet()
         @"New Peninim MT",
         @"Optima",
         @"Osaka",
+        @"Palatino",
         @"Papyrus",
         @"PCMyungjo",
         @"PilGi",
index e6c73110f5ab14bdf9346aeba97aefeca0b4ff58..37ab44ba5111a9b1b07ddcf1280dcab4917852cc 100644 (file)
@@ -235,6 +235,7 @@ static NSSet *allowedFontFamilySet()
         @"New Peninim MT",
         @"Optima",
         @"Osaka",
+        @"Palatino",
         @"Papyrus",
         @"PCMyungjo",
         @"PilGi",