Skip to content

Commit c7df725

Browse files
author
[email protected]
committed
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. 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: http://svn.webkit.org/repository/webkit/trunk@226668 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 11effde commit c7df725

23 files changed

+143
-15
lines changed

‎LayoutTests/ChangeLog

+25
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,28 @@
1+
2018-01-09 Myles C. Maxfield <[email protected]>
2+
3+
font-display:fallback can cause a visual flash (which is supposed to be impossible)
4+
https://bugs.webkit.org/show_bug.cgi?id=181374
5+
6+
Reviewed by Simon Fraser.
7+
8+
Move font-display tests into their common subfolder.
9+
10+
* fast/text/font-display/block-finish-expected.html: Renamed from LayoutTests/fast/text/loading-block-finish-expected.html.
11+
* fast/text/font-display/block-finish.html: Renamed from LayoutTests/fast/text/loading-block-finish.html.
12+
* fast/text/font-display/block-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-block-nofinish-expected.html.
13+
* fast/text/font-display/block-nofinish.html: Renamed from LayoutTests/fast/text/loading-block-nofinish.html.
14+
* fast/text/font-display/failure-finish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-finish-expected.html.
15+
* fast/text/font-display/failure-finish.html: Renamed from LayoutTests/fast/text/loading-failure-finish.html.
16+
* fast/text/font-display/failure-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish-expected.html.
17+
* fast/text/font-display/failure-nofinish.html: Renamed from LayoutTests/fast/text/loading-failure-nofinish.html.
18+
* fast/text/font-display/swap-finish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-finish-expected.html.
19+
* fast/text/font-display/swap-finish.html: Renamed from LayoutTests/fast/text/loading-swap-finish.html.
20+
* fast/text/font-display/swap-nofinish-expected.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish-expected.html.
21+
* fast/text/font-display/swap-nofinish.html: Renamed from LayoutTests/fast/text/loading-swap-nofinish.html.
22+
* fast/text/font-display/swap-flash-expected.html: Added.
23+
* fast/text/font-display/swap-flash.html: Added.
24+
* platform/win/TestExpectations:
25+
126
2018-01-09 Matt Lewis <[email protected]>
227

328
Fixed test expectaions.

‎LayoutTests/fast/text/loading-block-finish-expected.html renamed to ‎LayoutTests/fast/text/font-display/block-finish-expected.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<style>
55
@font-face {
66
font-family: "MyFont";
7-
src: url("../../resources/Ahem.ttf") format("truetype");
7+
src: url("../../../resources/Ahem.ttf") format("truetype");
88
}
99
</style>
1010
</head>

‎LayoutTests/fast/text/loading-block-finish.html renamed to ‎LayoutTests/fast/text/font-display/block-finish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/fast/text/loading-block-nofinish-expected.html renamed to ‎LayoutTests/fast/text/font-display/block-nofinish-expected.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<style>
55
@font-face {
66
font-family: "MyFont";
7-
src: url("../../resources/Ahem.ttf") format("truetype");
7+
src: url("../../../resources/Ahem.ttf") format("truetype");
88
}
99
</style>
1010
</head>

‎LayoutTests/fast/text/loading-block-nofinish.html renamed to ‎LayoutTests/fast/text/font-display/block-nofinish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/fast/text/loading-failure-finish-expected.html renamed to ‎LayoutTests/fast/text/font-display/failure-finish-expected.html

File renamed without changes.

‎LayoutTests/fast/text/loading-failure-finish.html renamed to ‎LayoutTests/fast/text/font-display/failure-finish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/fast/text/loading-failure-nofinish-expected.html renamed to ‎LayoutTests/fast/text/font-display/failure-nofinish-expected.html

File renamed without changes.

‎LayoutTests/fast/text/loading-failure-nofinish.html renamed to ‎LayoutTests/fast/text/font-display/failure-nofinish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/fast/text/loading-swap-finish-expected.html renamed to ‎LayoutTests/fast/text/font-display/swap-finish-expected.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<style>
55
@font-face {
66
font-family: "MyFont";
7-
src: url("../../resources/Ahem.ttf") format("truetype");
7+
src: url("../../../resources/Ahem.ttf") format("truetype");
88
}
99
</style>
1010
</head>

‎LayoutTests/fast/text/loading-swap-finish.html renamed to ‎LayoutTests/fast/text/font-display/swap-finish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/fast/text/font-display/swap-flash-expected.html

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8">
5+
<script>
6+
if (window.internals) {
7+
internals.settings.setFontLoadTimingOverride("Failure");
8+
internals.settings.setShouldIgnoreFontLoadCompletions(false);
9+
}
10+
</script>
11+
<style>
12+
@font-face {
13+
font-family: "MyFont";
14+
src: url("../../../resources/Ahem.ttf") format("truetype");
15+
}
16+
</style>
17+
</head>
18+
<body>
19+
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.
20+
<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>
21+
</body>
22+
</html>

‎LayoutTests/fast/text/font-display/swap-flash.html

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8">
5+
<script>
6+
if (window.internals) {
7+
internals.settings.setFontLoadTimingOverride("Swap");
8+
internals.settings.setShouldIgnoreFontLoadCompletions(true);
9+
}
10+
</script>
11+
<style>
12+
@font-face {
13+
font-family: "MyFont";
14+
src: url("../../../resources/Ahem.ttf") format("truetype");
15+
}
16+
</style>
17+
</head>
18+
<body>
19+
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.
20+
<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>
21+
</body>
22+
</html>

‎LayoutTests/fast/text/loading-swap-nofinish-expected.html renamed to ‎LayoutTests/fast/text/font-display/swap-nofinish-expected.html

File renamed without changes.

‎LayoutTests/fast/text/loading-swap-nofinish.html renamed to ‎LayoutTests/fast/text/font-display/swap-nofinish.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<style>
1111
@font-face {
1212
font-family: "MyFont";
13-
src: url("../../resources/Ahem.ttf") format("truetype");
13+
src: url("../../../resources/Ahem.ttf") format("truetype");
1414
}
1515
</style>
1616
</head>

‎LayoutTests/platform/win/TestExpectations

+4-4
Original file line numberDiff line numberDiff line change
@@ -3789,10 +3789,10 @@ webkit.org/b/177626 fast/dom/HTMLLinkElement/preconnect-support.html [ Skip ]
37893789
webkit.org/b/177626 http/tests/preconnect/link-rel-preconnect-http.html [ Skip ]
37903790
webkit.org/b/177626 http/tests/preconnect/link-rel-preconnect-https.html [ Skip ]
37913791

3792-
webkit.org/b/177964 fast/text/loading-block-nofinish.html [ Pass ImageOnlyFailure ]
3793-
webkit.org/b/177964 fast/text/loading-failure-finish.html [ Pass ImageOnlyFailure ]
3794-
webkit.org/b/177964 fast/text/loading-failure-nofinish.html [ Pass ImageOnlyFailure ]
3795-
webkit.org/b/177964 fast/text/loading-swap-nofinish.html [ Pass ImageOnlyFailure ]
3792+
webkit.org/b/177964 fast/text/font-display/block-nofinish.html [ Pass ImageOnlyFailure ]
3793+
webkit.org/b/177964 fast/text/font-display/failure-finish.html [ Pass ImageOnlyFailure ]
3794+
webkit.org/b/177964 fast/text/font-display/failure-nofinish.html [ Pass ImageOnlyFailure ]
3795+
webkit.org/b/177964 fast/text/font-display/swap-nofinish.html [ Pass ImageOnlyFailure ]
37963796

37973797
# xhtml tests are failing on some of the EWS bots.
37983798
webkit.org/b/178230 fast/xmlhttprequest/xmlhttprequest-get.xhtml [ Failure ]

‎Source/WebCore/ChangeLog

+30
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,33 @@
1+
2018-01-09 Myles C. Maxfield <[email protected]>
2+
3+
font-display:fallback can cause a visual flash (which is supposed to be impossible)
4+
https://bugs.webkit.org/show_bug.cgi?id=181374
5+
6+
Reviewed by Simon Fraser.
7+
8+
A FontCascade represents an entire font-family fallback list, but sometimes we need to pull out a single
9+
representative font from the list to calculate things like line height. Previously, if the first item in
10+
the font-family list was in the middle of being downloaded, this representative font was hardcoded to be
11+
Times. However, when actually laying out and drawing the glyphs, we have logic to skip the interstitial
12+
Times if there are any installed fonts present in the font-family list (so you wouldn't ever actually
13+
see Times). This means that line height (among other things) was being calculated as if Times was used,
14+
but in reality, some other font from the font-family list was being used.
15+
16+
Alone, this isn't a huge problem, but font-display:fallback makes a font transition between "timed out"
17+
and "failed," and when the font hits the failed state, the representative font skips over the cancelled
18+
item and hits the next item in the fallback list. This means that line heights will change, which causes
19+
a visual flash, even when font-display:fallback is specified.
20+
21+
The solution is simply to educate the logic which identifies this representative font so that it
22+
understands what to do for currently-loading fonts.
23+
24+
Tests: fast/text/font-display/swap-flash.html
25+
26+
* platform/graphics/FontCascadeFonts.h:
27+
(WebCore::FontCascadeFonts::primaryFont):
28+
* rendering/line/BreakingContext.h:
29+
(WebCore::textWidth):
30+
131
2018-01-04 Filip Pizlo <[email protected]>
232

333
CodeBlocks should be in IsoSubspaces

‎Source/WebCore/platform/graphics/FontCascadeFonts.h

+13-1
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,21 @@ inline const Font& FontCascadeFonts::primaryFont(const FontCascadeDescription& d
126126
ASSERT(isMainThread());
127127
if (!m_cachedPrimaryFont) {
128128
auto& primaryRanges = realizeFallbackRangesAt(description, 0);
129-
m_cachedPrimaryFont = primaryRanges.fontForCharacter(' ');
129+
m_cachedPrimaryFont = primaryRanges.glyphDataForCharacter(' ', ExternalResourceDownloadPolicy::Allow).font;
130130
if (!m_cachedPrimaryFont)
131131
m_cachedPrimaryFont = &primaryRanges.fontForFirstRange();
132+
else if (m_cachedPrimaryFont->isInterstitial()) {
133+
for (unsigned index = 1; ; ++index) {
134+
auto& localRanges = realizeFallbackRangesAt(description, index);
135+
if (localRanges.isNull())
136+
break;
137+
auto* font = localRanges.glyphDataForCharacter(' ', ExternalResourceDownloadPolicy::Forbid).font;
138+
if (font && !font->isInterstitial()) {
139+
m_cachedPrimaryFont = font;
140+
break;
141+
}
142+
}
143+
}
132144
}
133145
return *m_cachedPrimaryFont;
134146
}

‎Source/WebCore/platform/graphics/FontRanges.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ class FontRanges {
8383
unsigned size() const { return m_ranges.size(); }
8484
const Range& rangeAt(unsigned i) const { return m_ranges[i]; }
8585

86-
GlyphData glyphDataForCharacter(UChar32, ExternalResourceDownloadPolicy) const;
86+
WEBCORE_EXPORT GlyphData glyphDataForCharacter(UChar32, ExternalResourceDownloadPolicy) const;
8787
WEBCORE_EXPORT const Font* fontForCharacter(UChar32) const;
8888
WEBCORE_EXPORT const Font& fontForFirstRange() const;
8989
bool isLoading() const;

‎Source/WebCore/rendering/line/BreakingContext.h

+1
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,7 @@ ALWAYS_INLINE float textWidth(RenderText& text, unsigned from, unsigned len, con
621621
const RenderStyle& style = text.style();
622622

623623
GlyphOverflow glyphOverflow;
624+
// 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!
624625
if (isFixedPitch || (!from && len == text.text().length()) || style.hasTextCombine())
625626
return text.width(from, len, font, xPos, &fallbackFonts, &glyphOverflow);
626627

‎Tools/ChangeLog

+14
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2018-01-09 Myles C. Maxfield <[email protected]>
2+
3+
font-display:fallback can cause a visual flash (which is supposed to be impossible)
4+
https://bugs.webkit.org/show_bug.cgi?id=181374
5+
6+
Reviewed by Simon Fraser.
7+
8+
The test requires Palatino.
9+
10+
* DumpRenderTree/mac/DumpRenderTree.mm:
11+
(allowedFontFamilySet):
12+
* WebKitTestRunner/mac/TestControllerMac.mm:
13+
(WTR::allowedFontFamilySet):
14+
115
2018-01-09 Saam Barati <[email protected]>
216

317
Give some slack in display-profiler-outputs computation of the terminal window's number of columns

‎Tools/DumpRenderTree/mac/DumpRenderTree.mm

+1
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,7 @@ static bool shouldIgnoreWebCoreNodeLeaks(const string& URLString)
394394
@"New Peninim MT",
395395
@"Optima",
396396
@"Osaka",
397+
@"Palatino",
397398
@"Papyrus",
398399
@"PCMyungjo",
399400
@"PilGi",

‎Tools/WebKitTestRunner/mac/TestControllerMac.mm

+1
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ static PlatformWindow wtr_NSApplication_keyWindow(id self, SEL _cmd)
235235
@"New Peninim MT",
236236
@"Optima",
237237
@"Osaka",
238+
@"Palatino",
238239
@"Papyrus",
239240
@"PCMyungjo",
240241
@"PilGi",

0 commit comments

Comments
 (0)