REGRESSION(r216944): Font loads can cause Chinese characters to draw as .notdef
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 22:09:11 +0000 (22:09 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 22:09:11 +0000 (22:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173962
<rdar://problem/32925318>

Reviewed by Simon Fraser.

Source/WebCore:

Previously, there was no signalling between our font loading code
which determined whether or not a font should be invisible (because
its in the middle of loading) and our system fallback code which
created fonts when we fall off the end of the fallback list. Because
of this, we were doing two things wrong:

1. When we started downloading a font, we would try to use a fallback
font. However, if the fallback font didn't suppor the character we're
trying to render, we would just bail and draw .notdef
2. Even if we continued down the fallback list, and fell of the end,
we wouldn't realize that the system fallback font should also be drawn
as invisible.

This patch solves these two problems by:
1. Performing a search to find the best (local) fallback font with
which to fall systemFallbackFontForCharacter(). This way, if you say
"font-family: 'RemoteFont', 'Helvetica'" we will use Helvetica as
the lookup to ask the system to search for.
2. Give the Font class an accessor which can create a duplicate, but
invisible font. Give FontCascadeFonts::glyphDataForVariant() the
correct tracking to know when to use this invisible duplicate.

Tests: fast/text/font-loading-system-fallback.html
       http/tests/webfont/font-loading-system-fallback-visibility.html

* platform/graphics/Font.cpp:
(WebCore::Font::invisibleFont):
* platform/graphics/Font.h:
* platform/graphics/FontCascadeFonts.cpp:
(WebCore::findBestFallbackFont):
(WebCore::FontCascadeFonts::glyphDataForSystemFallback):
(WebCore::FontCascadeFonts::glyphDataForVariant):
* platform/graphics/FontCascadeFonts.h:

LayoutTests:

* fast/text/font-loading-system-fallback-expected.html: Added.
* fast/text/font-loading-system-fallback.html: Added.
* http/tests/webfont/font-loading-system-fallback-visibility-expected.html: Added.
* http/tests/webfont/font-loading-system-fallback-visibility.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/text/font-loading-system-fallback-expected.html [new file with mode: 0644]
LayoutTests/fast/text/font-loading-system-fallback.html [new file with mode: 0644]
LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility-expected.html [new file with mode: 0644]
LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/Font.h
Source/WebCore/platform/graphics/FontCascadeFonts.cpp
Source/WebCore/platform/graphics/FontCascadeFonts.h

index 8c526a2..fea1cc0 100644 (file)
@@ -1,3 +1,16 @@
+2017-07-06  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(r216944): Font loads can cause Chinese characters to draw as .notdef
+        https://bugs.webkit.org/show_bug.cgi?id=173962
+        <rdar://problem/32925318>
+
+        Reviewed by Simon Fraser.
+
+        * fast/text/font-loading-system-fallback-expected.html: Added.
+        * fast/text/font-loading-system-fallback.html: Added.
+        * http/tests/webfont/font-loading-system-fallback-visibility-expected.html: Added.
+        * http/tests/webfont/font-loading-system-fallback-visibility.html: Added.
+
 2017-07-06  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r219193.
diff --git a/LayoutTests/fast/text/font-loading-system-fallback-expected.html b/LayoutTests/fast/text/font-loading-system-fallback-expected.html
new file mode 100644 (file)
index 0000000..5bbefde
--- /dev/null
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+This test makes sure that system fallback fonts (occuring when falling off the end of the fallback list) during loading are chosen from the best local font possible. The test passes if the Japanese character below is drawn in a font which matches with Helvetica's design.
+<div style="font: 48px 'Helvetica';">&#x306E;<div style="display: inline-block; width: 1px; height: 200px;"></div></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/text/font-loading-system-fallback.html b/LayoutTests/fast/text/font-loading-system-fallback.html
new file mode 100644 (file)
index 0000000..45917a9
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.internals) {
+    internals.settings.setWebFontsAlwaysFallBack(true);
+    internals.clearMemoryCache();
+    internals.invalidateFontCache();
+}
+</script>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: url("../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+</head>
+<body>
+This test makes sure that system fallback fonts (occuring when falling off the end of the fallback list) during loading are chosen from the best local font possible. The test passes if the Japanese character below is drawn in a font which matches with Helvetica's design.
+<div style="font: 48px 'WebFont', 'Helvetica';">&#x306E;<div style="display: inline-block; width: 1px; height: 200px;"></div></div>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility-expected.html b/LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility-expected.html
new file mode 100644 (file)
index 0000000..0d91ae1
--- /dev/null
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.internals) {
+    internals.clearMemoryCache();
+    internals.invalidateFontCache();
+}
+</script>
+</head>
+<body>
+This test makes sure that system fallback fonts (occuring when falling off the end of the fallback list) during loading are invisible when the loading font is invisible. The test passes if this text is the only text on the page.
+</body>
+</html>
diff --git a/LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility.html b/LayoutTests/http/tests/webfont/font-loading-system-fallback-visibility.html
new file mode 100644 (file)
index 0000000..66000cf
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.internals) {
+    internals.clearMemoryCache();
+    internals.invalidateFontCache();
+}
+</script>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: url("../resources/Ahem.woff");
+}
+
+@font-face {
+    font-family: "WebFont2";
+    src: url("slow-ahem-loading.cgi");
+}
+</style>
+</head>
+<body>
+This test makes sure that system fallback fonts (occuring when falling off the end of the fallback list) during loading are invisible when the loading font is invisible. The test passes if this text is the only text on the page.
+<div style="font: 48px 'WebFont', 'WebFont2', 'Helvetica';">&#x306E;</div>
+</body>
+</html>
index b8d1ca9..e7843c3 100644 (file)
@@ -1,3 +1,45 @@
+2017-07-06  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(r216944): Font loads can cause Chinese characters to draw as .notdef
+        https://bugs.webkit.org/show_bug.cgi?id=173962
+        <rdar://problem/32925318>
+
+        Reviewed by Simon Fraser.
+
+        Previously, there was no signalling between our font loading code
+        which determined whether or not a font should be invisible (because
+        its in the middle of loading) and our system fallback code which
+        created fonts when we fall off the end of the fallback list. Because
+        of this, we were doing two things wrong:
+
+        1. When we started downloading a font, we would try to use a fallback
+        font. However, if the fallback font didn't suppor the character we're
+        trying to render, we would just bail and draw .notdef
+        2. Even if we continued down the fallback list, and fell of the end,
+        we wouldn't realize that the system fallback font should also be drawn
+        as invisible.
+
+        This patch solves these two problems by:
+        1. Performing a search to find the best (local) fallback font with
+        which to fall systemFallbackFontForCharacter(). This way, if you say
+        "font-family: 'RemoteFont', 'Helvetica'" we will use Helvetica as
+        the lookup to ask the system to search for.
+        2. Give the Font class an accessor which can create a duplicate, but
+        invisible font. Give FontCascadeFonts::glyphDataForVariant() the
+        correct tracking to know when to use this invisible duplicate.
+
+        Tests: fast/text/font-loading-system-fallback.html
+               http/tests/webfont/font-loading-system-fallback-visibility.html
+
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::invisibleFont):
+        * platform/graphics/Font.h:
+        * platform/graphics/FontCascadeFonts.cpp:
+        (WebCore::findBestFallbackFont):
+        (WebCore::FontCascadeFonts::glyphDataForSystemFallback):
+        (WebCore::FontCascadeFonts::glyphDataForVariant):
+        * platform/graphics/FontCascadeFonts.h:
+
 2017-07-06  Chris Dumez  <cdumez@apple.com>
 
         FileMonitor should not be ref counted
index ebbc90e..71da0e0 100644 (file)
@@ -261,47 +261,59 @@ GlyphData Font::glyphDataForCharacter(UChar32 character) const
     return page->glyphDataForCharacter(character);
 }
 
-const Font& Font::verticalRightOrientationFont() const
+auto Font::ensureDerivedFontData() const -> DerivedFonts&
 {
     if (!m_derivedFontData)
         m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->verticalRightOrientation) {
+    return *m_derivedFontData;
+}
+
+const Font& Font::verticalRightOrientationFont() const
+{
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.verticalRightOrientationFont) {
         auto verticalRightPlatformData = FontPlatformData::cloneWithOrientation(m_platformData, Horizontal);
-        m_derivedFontData->verticalRightOrientation = create(verticalRightPlatformData, origin(), Interstitial::No, Visibility::Visible, OrientationFallback::Yes);
+        derivedFontData.verticalRightOrientationFont = create(verticalRightPlatformData, origin(), Interstitial::No, Visibility::Visible, OrientationFallback::Yes);
     }
-    ASSERT(m_derivedFontData->verticalRightOrientation != this);
-    return *m_derivedFontData->verticalRightOrientation;
+    ASSERT(derivedFontData.verticalRightOrientationFont != this);
+    return *derivedFontData.verticalRightOrientationFont;
 }
 
 const Font& Font::uprightOrientationFont() const
 {
-    if (!m_derivedFontData)
-        m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->uprightOrientation)
-        m_derivedFontData->uprightOrientation = create(m_platformData, origin(), Interstitial::No, Visibility::Visible, OrientationFallback::Yes);
-    ASSERT(m_derivedFontData->uprightOrientation != this);
-    return *m_derivedFontData->uprightOrientation;
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.uprightOrientationFont)
+        derivedFontData.uprightOrientationFont = create(m_platformData, origin(), Interstitial::No, Visibility::Visible, OrientationFallback::Yes);
+    ASSERT(derivedFontData.uprightOrientationFont != this);
+    return *derivedFontData.uprightOrientationFont;
+}
+
+const Font& Font::invisibleFont() const
+{
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.invisibleFont)
+        derivedFontData.invisibleFont = create(m_platformData, origin(), Interstitial::Yes, Visibility::Invisible);
+    ASSERT(derivedFontData.invisibleFont != this);
+    return *derivedFontData.invisibleFont;
 }
 
 const Font* Font::smallCapsFont(const FontDescription& fontDescription) const
 {
-    if (!m_derivedFontData)
-        m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->smallCaps)
-        m_derivedFontData->smallCaps = createScaledFont(fontDescription, smallCapsFontSizeMultiplier);
-    ASSERT(m_derivedFontData->smallCaps != this);
-    return m_derivedFontData->smallCaps.get();
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.smallCapsFont)
+        derivedFontData.smallCapsFont = createScaledFont(fontDescription, smallCapsFontSizeMultiplier);
+    ASSERT(derivedFontData.smallCapsFont != this);
+    return derivedFontData.smallCapsFont.get();
 }
 
 const Font& Font::noSynthesizableFeaturesFont() const
 {
 #if PLATFORM(COCOA)
-    if (!m_derivedFontData)
-        m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->noSynthesizableFeatures)
-        m_derivedFontData->noSynthesizableFeatures = createFontWithoutSynthesizableFeatures();
-    ASSERT(m_derivedFontData->noSynthesizableFeatures != this);
-    return *m_derivedFontData->noSynthesizableFeatures;
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.noSynthesizableFeaturesFont)
+        derivedFontData.noSynthesizableFeaturesFont = createFontWithoutSynthesizableFeatures();
+    ASSERT(derivedFontData.noSynthesizableFeaturesFont != this);
+    return *derivedFontData.noSynthesizableFeaturesFont;
 #else
     return *this;
 #endif
@@ -309,24 +321,22 @@ const Font& Font::noSynthesizableFeaturesFont() const
 
 const Font* Font::emphasisMarkFont(const FontDescription& fontDescription) const
 {
-    if (!m_derivedFontData)
-        m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->emphasisMark)
-        m_derivedFontData->emphasisMark = createScaledFont(fontDescription, emphasisMarkFontSizeMultiplier);
-    ASSERT(m_derivedFontData->emphasisMark != this);
-    return m_derivedFontData->emphasisMark.get();
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.emphasisMarkFont)
+        derivedFontData.emphasisMarkFont = createScaledFont(fontDescription, emphasisMarkFontSizeMultiplier);
+    ASSERT(derivedFontData.emphasisMarkFont != this);
+    return derivedFontData.emphasisMarkFont.get();
 }
 
 const Font& Font::brokenIdeographFont() const
 {
-    if (!m_derivedFontData)
-        m_derivedFontData = std::make_unique<DerivedFonts>();
-    if (!m_derivedFontData->brokenIdeograph) {
-        m_derivedFontData->brokenIdeograph = create(m_platformData, origin(), Interstitial::No);
-        m_derivedFontData->brokenIdeograph->m_isBrokenIdeographFallback = true;
+    DerivedFonts& derivedFontData = ensureDerivedFontData();
+    if (!derivedFontData.brokenIdeographFont) {
+        derivedFontData.brokenIdeographFont = create(m_platformData, origin(), Interstitial::No);
+        derivedFontData.brokenIdeographFont->m_isBrokenIdeographFallback = true;
     }
-    ASSERT(m_derivedFontData->brokenIdeograph != this);
-    return *m_derivedFontData->brokenIdeograph;
+    ASSERT(derivedFontData.brokenIdeographFont != this);
+    return *derivedFontData.brokenIdeographFont;
 }
 
 #ifndef NDEBUG
index 32ea821..a2baa79 100644 (file)
@@ -134,6 +134,7 @@ public:
 
     const Font& verticalRightOrientationFont() const;
     const Font& uprightOrientationFont() const;
+    const Font& invisibleFont() const;
 
     bool hasVerticalGlyphs() const { return m_hasVerticalGlyphs; }
     bool isTextOrientationFallback() const { return m_isTextOrientationFallback; }
@@ -237,6 +238,9 @@ private:
     RefPtr<Font> platformCreateScaledFont(const FontDescription&, float scaleFactor) const;
 
     void removeFromSystemFallbackCache();
+    
+    struct DerivedFonts;
+    DerivedFonts& ensureDerivedFontData() const;
 
 #if PLATFORM(WIN)
     void initGDIFont();
@@ -274,12 +278,13 @@ private:
 #endif
     public:
 
-        RefPtr<Font> smallCaps;
-        RefPtr<Font> noSynthesizableFeatures;
-        RefPtr<Font> emphasisMark;
-        RefPtr<Font> brokenIdeograph;
-        RefPtr<Font> verticalRightOrientation;
-        RefPtr<Font> uprightOrientation;
+        RefPtr<Font> smallCapsFont;
+        RefPtr<Font> noSynthesizableFeaturesFont;
+        RefPtr<Font> emphasisMarkFont;
+        RefPtr<Font> brokenIdeographFont;
+        RefPtr<Font> verticalRightOrientationFont;
+        RefPtr<Font> uprightOrientationFont;
+        RefPtr<Font> invisibleFont;
     };
 
     mutable std::unique_ptr<DerivedFonts> m_derivedFontData;
index 5ba3ac3..c5768d7 100644 (file)
@@ -323,30 +323,49 @@ static GlyphData glyphDataForNonCJKCharacterWithGlyphOrientation(UChar32 charact
     return data;
 }
 
-GlyphData FontCascadeFonts::glyphDataForSystemFallback(UChar32 c, const FontCascadeDescription& description, FontVariant variant)
+static const Font* findBestFallbackFont(FontCascadeFonts& fontCascadeFonts, const FontCascadeDescription& description, UChar32 character)
 {
-    // System fallback is character-dependent.
-    auto& primaryRanges = realizeFallbackRangesAt(description, 0);
-    auto* originalFont = primaryRanges.fontForCharacter(c);
-    if (!originalFont)
-        originalFont = &primaryRanges.fontForFirstRange();
+    for (unsigned fallbackIndex = 0; ; ++fallbackIndex) {
+        auto& fontRanges = fontCascadeFonts.realizeFallbackRangesAt(description, fallbackIndex);
+        if (fontRanges.isNull())
+            break;
+        auto* currentFont = fontRanges.glyphDataForCharacter(character, ExternalResourceDownloadPolicy::Forbid).font;
+        if (!currentFont)
+            currentFont = &fontRanges.fontForFirstRange();
+
+        if (!currentFont->isInterstitial())
+            return currentFont;
+    }
+
+    return nullptr;
+}
 
-    auto systemFallbackFont = originalFont->systemFallbackFontForCharacter(c, description, m_isForPlatformFont);
+GlyphData FontCascadeFonts::glyphDataForSystemFallback(UChar32 character, const FontCascadeDescription& description, FontVariant variant, bool systemFallbackShouldBeInvisible)
+{
+    const Font* font = findBestFallbackFont(*this, description, character);
+
+    if (!font)
+        font = &realizeFallbackRangesAt(description, 0).fontForFirstRange();
+
+    auto systemFallbackFont = font->systemFallbackFontForCharacter(character, description, m_isForPlatformFont);
     if (!systemFallbackFont)
         return GlyphData();
 
-    if (systemFallbackFont->platformData().orientation() == Vertical && !systemFallbackFont->hasVerticalGlyphs() && FontCascade::isCJKIdeographOrSymbol(c))
+    if (systemFallbackShouldBeInvisible)
+        systemFallbackFont = const_cast<Font*>(&systemFallbackFont->invisibleFont());
+
+    if (systemFallbackFont->platformData().orientation() == Vertical && !systemFallbackFont->hasVerticalGlyphs() && FontCascade::isCJKIdeographOrSymbol(character))
         variant = BrokenIdeographVariant;
 
     GlyphData fallbackGlyphData;
     if (variant == NormalVariant)
-        fallbackGlyphData = systemFallbackFont->glyphDataForCharacter(c);
+        fallbackGlyphData = systemFallbackFont->glyphDataForCharacter(character);
     else
-        fallbackGlyphData = systemFallbackFont->variantFont(description, variant)->glyphDataForCharacter(c);
+        fallbackGlyphData = systemFallbackFont->variantFont(description, variant)->glyphDataForCharacter(character);
 
     if (fallbackGlyphData.font && fallbackGlyphData.font->platformData().orientation() == Vertical && !fallbackGlyphData.font->isTextOrientationFallback()) {
-        if (variant == NormalVariant && !FontCascade::isCJKIdeographOrSymbol(c))
-            fallbackGlyphData = glyphDataForNonCJKCharacterWithGlyphOrientation(c, description.nonCJKGlyphOrientation(), fallbackGlyphData);
+        if (variant == NormalVariant && !FontCascade::isCJKIdeographOrSymbol(character))
+            fallbackGlyphData = glyphDataForNonCJKCharacterWithGlyphOrientation(character, description.nonCJKGlyphOrientation(), fallbackGlyphData);
     }
 
     // Keep the system fallback fonts we use alive.
@@ -356,8 +375,15 @@ GlyphData FontCascadeFonts::glyphDataForSystemFallback(UChar32 c, const FontCasc
     return fallbackGlyphData;
 }
 
+enum class SystemFallbackVisibility {
+    Immaterial,
+    Visible,
+    Invisible
+};
+
 GlyphData FontCascadeFonts::glyphDataForVariant(UChar32 character, const FontCascadeDescription& description, FontVariant variant, unsigned fallbackIndex)
 {
+    SystemFallbackVisibility systemFallbackVisibility = SystemFallbackVisibility::Immaterial;
     ExternalResourceDownloadPolicy policy = ExternalResourceDownloadPolicy::Allow;
     GlyphData loadingResult;
     for (; ; ++fallbackIndex) {
@@ -371,7 +397,9 @@ GlyphData FontCascadeFonts::glyphDataForVariant(UChar32 character, const FontCas
 
         if (data.font->isInterstitial()) {
             policy = ExternalResourceDownloadPolicy::Forbid;
-            if (!loadingResult.font)
+            if (systemFallbackVisibility == SystemFallbackVisibility::Immaterial)
+                systemFallbackVisibility = data.font->visibility() == Font::Visibility::Visible ? SystemFallbackVisibility::Visible : SystemFallbackVisibility::Invisible;
+            if (!loadingResult.font && data.glyph)
                 loadingResult = data;
             continue;
         }
@@ -399,7 +427,7 @@ GlyphData FontCascadeFonts::glyphDataForVariant(UChar32 character, const FontCas
 
     if (loadingResult.font)
         return loadingResult;
-    return glyphDataForSystemFallback(character, description, variant);
+    return glyphDataForSystemFallback(character, description, variant, systemFallbackVisibility == SystemFallbackVisibility::Invisible);
 }
 
 static RefPtr<GlyphPage> glyphPageFromFontRanges(unsigned pageNumber, const FontRanges& fontRanges)
index 92e06a6..9a8b54b 100644 (file)
@@ -76,7 +76,7 @@ private:
     FontCascadeFonts(RefPtr<FontSelector>&&);
     FontCascadeFonts(const FontPlatformData&);
 
-    GlyphData glyphDataForSystemFallback(UChar32, const FontCascadeDescription&, FontVariant);
+    GlyphData glyphDataForSystemFallback(UChar32, const FontCascadeDescription&, FontVariant, bool systemFallbackShouldBeInvisible);
     GlyphData glyphDataForVariant(UChar32, const FontCascadeDescription&, FontVariant, unsigned fallbackIndex = 0);
 
     Vector<FontRanges, 1> m_realizedFallbackRanges;