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 2c098bb..19dbc5a 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.
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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 48d04ce..e6909c6 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 7d4ef66..4a8aa18 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 9a8b54b..70f7c56 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 d35ea7c..d557772 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 7f1ba1c..17da8ff 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 f8adfc6..8936f0c 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 38ef2f8..477cc1f 100644 (file)
@@ -394,6 +394,7 @@ static NSSet *allowedFontFamilySet()
         @"New Peninim MT",
         @"Optima",
         @"Osaka",
+        @"Palatino",
         @"Papyrus",
         @"PCMyungjo",
         @"PilGi",
index e6c7311..37ab44b 100644 (file)
@@ -235,6 +235,7 @@ static NSSet *allowedFontFamilySet()
         @"New Peninim MT",
         @"Optima",
         @"Osaka",
+        @"Palatino",
         @"Papyrus",
         @"PCMyungjo",
         @"PilGi",