Merge r229165 - REGRESSION(r222843): [HarfBuzz] Combining enclosed ke…

…ycap not correctly handled https://bugs.webkit.org/show_bug.cgi?id=183246 Reviewed by Michael Catanzaro. Source/WebCore: We are not correctly handling the combining enclosed keycap since we switched to use ComplexTextController. This is because fontForCombiningCharacterSequence() always returns the font of the first character, without checking if that font can render the whole sequence or not. Before 222843, the shaper did that check when creating the text runs. In this case the sequence was split and a different font was used for the text and the mark. This patch makes fontForCombiningCharacterSequence() try to find a suitable font for the whole sequence, first looking at the CSS fallbacks and finally at system ones. The result is much better than the old one, because we use the same font for both the text and the mark. If there isn't any font to render the mark, then we fallback to use the first character font, since we will end up rendering the missing glyph character, it's better to use the same font than the first character one. Test: fast/text/combining-enclosing-keycap.html * platform/graphics/cairo/FontCairoHarfbuzzNG.cpp: (WebCore::FontCascade::fontForCombiningCharacterSequence const): Check if the first charatcer font can render the whole sequence, trying with fallbacks otherwise. * platform/graphics/freetype/SimpleFontDataFreeType.cpp: (WebCore::Font::canRenderCombiningCharacterSequence const): Check if the font face has glyphs for the whole sequence not just the first character. LayoutTests: * fast/text/combining-enclosing-keycap-expected.txt: Added. * platform/gtk/fast/text/combining-enclosing-keycap.html: Added. * platform/gtk/TestExpectations:
WebKit · Mar 5, 2018 · 11d40a5 · 11d40a5
1 parent 45301a6
commit 11d40a5
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 22 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2018-03-01  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        REGRESSION(r222843): [HarfBuzz] Combining enclosed keycap not correctly handled
+        https://bugs.webkit.org/show_bug.cgi?id=183246
+
+        Reviewed by Michael Catanzaro.
+
+        * fast/text/combining-enclosing-keycap-expected.txt: Added.
+        * platform/gtk/fast/text/combining-enclosing-keycap.html: Added.
+        * platform/gtk/TestExpectations:
+
 2018-03-01  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [FreeType] Remove FontPlatformData fallbacks

diff --git a/LayoutTests/fast/text/combining-enclosing-keycap.html b/LayoutTests/fast/text/combining-enclosing-keycap.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+<span>#&#x20E3;</span>
+<span>0&#x20E3;</span>
+<span>1&#x20E3;</span>
+<span>2&#x20E3;</span>
+<span>3&#x20E3;</span>
+<span>4&#x20E3;</span>
+<span>5&#x20E3;</span>
+<span>6&#x20E3;</span>
+<span>7&#x20E3;</span>
+<span>8&#x20E3;</span>
+<span>9&#x20E3;</span>
+</body>
+</html>
diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations
@@ -3390,22 +3390,6 @@ fast/scrolling/rtl-scrollbars-alternate-body-dir-attr-does-not-update-scrollbar-
 fast/scrolling/rtl-scrollbars-alternate-iframe-body-dir-attr-does-not-update-scrollbar-placement.html [ Pass ]
 webkit.org/b/156437 fast/scrolling/rtl-scrollbars-listbox-scroll.html [ ImageOnlyFailure Pass ]
 
-# We don't support emoji genders, but half the reftests pass anyway.
-fast/text/emoji-gender-2-3.html [ Pass ]
-fast/text/emoji-gender-2-4.html [ Pass ]
-fast/text/emoji-gender-2-5.html [ Pass ]
-fast/text/emoji-gender-2-6.html [ Pass ]
-fast/text/emoji-gender-2-7.html [ Pass ]
-fast/text/emoji-gender-2-8.html [ Pass ]
-fast/text/emoji-gender-2-9.html [ Pass ]
-fast/text/emoji-gender-fe0f-3.html [ Pass ]
-fast/text/emoji-gender-fe0f-4.html [ Pass ]
-fast/text/emoji-gender-fe0f-5.html [ Pass ]
-fast/text/emoji-gender-fe0f-6.html [ Pass ]
-fast/text/emoji-gender-fe0f-7.html [ Pass ]
-fast/text/emoji-gender-fe0f-8.html [ Pass ]
-fast/text/emoji-gender-fe0f-9.html [ Pass ]
-
 transitions/svg-text-shadow-transition.html [ Pass ]
 
 webkit.org/b/155132 http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html [ Pass ]

diff --git a/LayoutTests/platform/gtk/fast/text/combining-enclosing-keycap-expected.txt b/LayoutTests/platform/gtk/fast/text/combining-enclosing-keycap-expected.txt
@@ -0,0 +1,59 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x34
+  RenderBlock {HTML} at (0,0) size 800x34
+    RenderBody {BODY} at (8,8) size 784x18
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (0,0) size 10x17
+          text run at (0,0) width 10: "#\x{20E3}"
+      RenderText {#text} at (10,0) size 4x17
+        text run at (10,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (14,0) size 10x17
+          text run at (14,0) width 10: "0\x{20E3}"
+      RenderText {#text} at (24,0) size 4x17
+        text run at (24,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (28,0) size 10x17
+          text run at (28,0) width 10: "1\x{20E3}"
+      RenderText {#text} at (38,0) size 4x17
+        text run at (38,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (42,0) size 10x17
+          text run at (42,0) width 10: "2\x{20E3}"
+      RenderText {#text} at (52,0) size 4x17
+        text run at (52,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (56,0) size 10x17
+          text run at (56,0) width 10: "3\x{20E3}"
+      RenderText {#text} at (66,0) size 4x17
+        text run at (66,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (70,0) size 10x17
+          text run at (70,0) width 10: "4\x{20E3}"
+      RenderText {#text} at (80,0) size 4x17
+        text run at (80,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (84,0) size 10x17
+          text run at (84,0) width 10: "5\x{20E3}"
+      RenderText {#text} at (94,0) size 4x17
+        text run at (94,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (98,0) size 10x17
+          text run at (98,0) width 10: "6\x{20E3}"
+      RenderText {#text} at (108,0) size 4x17
+        text run at (108,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (112,0) size 10x17
+          text run at (112,0) width 10: "7\x{20E3}"
+      RenderText {#text} at (122,0) size 4x17
+        text run at (122,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (126,0) size 10x17
+          text run at (126,0) width 10: "8\x{20E3}"
+      RenderText {#text} at (136,0) size 4x17
+        text run at (136,0) width 4: " "
+      RenderInline {SPAN} at (0,0) size 10x17
+        RenderText {#text} at (140,0) size 10x17
+          text run at (140,0) width 10: "9\x{20E3}"
+      RenderText {#text} at (0,0) size 0x0
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,29 @@
+2018-03-01  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        REGRESSION(r222843): [HarfBuzz] Combining enclosed keycap not correctly handled
+        https://bugs.webkit.org/show_bug.cgi?id=183246
+
+        Reviewed by Michael Catanzaro.
+
+        We are not correctly handling the combining enclosed keycap since we switched to use
+        ComplexTextController. This is because fontForCombiningCharacterSequence() always returns the font of the first
+        character, without checking if that font can render the whole sequence or not. Before 222843, the shaper did
+        that check when creating the text runs. In this case the sequence was split and a different font was used for the
+        text and the mark. This patch makes fontForCombiningCharacterSequence() try to find a suitable font for the
+        whole sequence, first looking at the CSS fallbacks and finally at system ones. The result is much better than
+        the old one, because we use the same font for both the text and the mark. If there isn't any font to render the
+        mark, then we fallback to use the first character font, since we will end up rendering the missing glyph
+        character, it's better to use the same font than the first character one.
+
+        Test: fast/text/combining-enclosing-keycap.html
+
+        * platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:
+        (WebCore::FontCascade::fontForCombiningCharacterSequence const): Check if the first charatcer font can render
+        the whole sequence, trying with fallbacks otherwise.
+        * platform/graphics/freetype/SimpleFontDataFreeType.cpp:
+        (WebCore::Font::canRenderCombiningCharacterSequence const): Check if the font face has glyphs for the whole
+        sequence not just the first character.
+
 2018-03-01  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [FreeType] Remove FontPlatformData fallbacks

diff --git a/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp b/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp
@@ -29,6 +29,7 @@
 
 #if USE(CAIRO)
 
+#include "FontCache.h"
 #include "SurrogatePairAwareTextIterator.h"
 #include <unicode/normlzr.h>
 
@@ -58,7 +59,25 @@ const Font* FontCascade::fontForCombiningCharacterSequence(const UChar* characte
     if (!iterator.consume(character, clusterLength))
         return nullptr;
 
-    return glyphDataForCharacter(character, false, NormalVariant).font;
+    const Font* baseFont = glyphDataForCharacter(character, false, NormalVariant).font;
+    if (baseFont && (static_cast<int32_t>(clusterLength) == normalizedLength || baseFont->canRenderCombiningCharacterSequence(characters, length)))
+        return baseFont;
+
+    for (unsigned i = 0; !fallbackRangesAt(i).isNull(); ++i) {
+        const Font* fallbackFont = fallbackRangesAt(i).fontForCharacter(character);
+        if (!fallbackFont || fallbackFont == baseFont)
+            continue;
+
+        if (fallbackFont->canRenderCombiningCharacterSequence(characters, length))
+            return fallbackFont;
+    }
+
+    if (auto systemFallback = FontCache::singleton().systemFallbackForCharacters(m_fontDescription, baseFont, false, characters, length)) {
+        if (systemFallback->canRenderCombiningCharacterSequence(characters, length))
+            return systemFallback.get();
+    }
+
+    return baseFont;
 }
 
 } // namespace WebCore

diff --git a/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp b/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp
@@ -42,6 +42,7 @@
 #include "GlyphBuffer.h"
 #include "OpenTypeTypes.h"
 #include "RefPtrCairo.h"
+#include "SurrogatePairAwareTextIterator.h"
 #include "UTF16UChar32Iterator.h"
 #include <cairo-ft.h>
 #include <cairo.h>
@@ -199,19 +200,24 @@ bool Font::canRenderCombiningCharacterSequence(const UChar* characters, size_t l
     UErrorCode error = U_ZERO_ERROR;
     Vector<UChar, 4> normalizedCharacters(length);
     int32_t normalizedLength = unorm_normalize(characters, length, UNORM_NFC, UNORM_UNICODE_3_2, &normalizedCharacters[0], length, &error);
-    // Can't render if we have an error or no composition occurred.
-    if (U_FAILURE(error) || (static_cast<size_t>(normalizedLength) == length))
+    if (U_FAILURE(error))
         return false;
 
     CairoFtFaceLocker cairoFtFaceLocker(m_platformData.scaledFont());
     FT_Face face = cairoFtFaceLocker.ftFace();
     if (!face)
         return false;
 
-    if (FcFreeTypeCharIndex(face, normalizedCharacters[0]))
-        addResult.iterator->value = true;
+    UChar32 character;
+    unsigned clusterLength = 0;
+    SurrogatePairAwareTextIterator iterator(normalizedCharacters.data(), 0, normalizedLength, normalizedLength);
+    for (iterator.advance(clusterLength); iterator.consume(character, clusterLength); iterator.advance(clusterLength)) {
+        if (!FcFreeTypeCharIndex(face, character))
+            return false;
+    }
 
-    return addResult.iterator->value;
+    addResult.iterator->value = true;
+    return true;
 }
 #endif