Attachment 369732 Details for Bug 197838 – Patch

[patch] Patch

wcore-ft-emoji-style.diff (text/plain), 9.82 KB, created by Carlos Garcia Campos on 2019-05-13 08:12:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Carlos Garcia Campos

Created: 2019-05-13 08:12:59 PDT

Size: 9.82 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5bc103ec247..6b173577fad 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2019-05-13  Carlos Garcia Campos  <cgarcia@igalia.com>
>+
>+        [FreeType] Some character sequences with a variation selector are not rendered
>+        https://bugs.webkit.org/show_bug.cgi?id=197838
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We get the invalid glyph instead. See http://mts.io/2015/04/21/unicode-symbol-render-text-emoji/. In the table at
>+        the end the Emoji and Text columns are not correctly rendered. It happens also when copying an emoji from
>+        GtkEmojiChooser and pasting in WebKit text field, because GTK appends U+FE0F to all emojis to force the emoji
>+        style. We need to take into account the variaiton selector when checking if a font can render a combining
>+        sequence, using FT_Face_GetCharVariantIndex to get the right glyph in case of variation character present.
>+
>+        * platform/graphics/Font.cpp:
>+        (WebCore::Font::platformSupportsCodePoint const): Add optional variation parameter.
>+        (WebCore::Font::canRenderCombiningCharacterSequence const): Take into account variation selector characters
>+        * platform/graphics/Font.h:
>+        * platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:
>+        (WebCore::FontCascade::fontForCombiningCharacterSequence const): Check variation selectors 0xFE0E and 0xFE0F to
>+        decide whether to use the emoji or text style.
>+        * platform/graphics/cocoa/FontCocoa.mm:
>+        (WebCore::Font::platformSupportsCodePoint const): Return false when a variation character is passed so that
>+        characters are checked individually.
>+        * platform/graphics/freetype/SimpleFontDataFreeType.cpp:
>+        (WebCore::Font::platformSupportsCodePoint const): Use FT_Face_GetCharVariantIndex when a variation character is
>+        passed.
>+        * platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:
>+        (WebCore::harfBuzzFontFunctions): Do not return true when FT_Face_GetCharVariantIndex returns 0.
>+
> 2019-05-08  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         REGRESSION(r239915): [FreeType] White space skipped when rendering plain text with noto CJK font
>diff --git a/Source/WebCore/platform/graphics/Font.cpp b/Source/WebCore/platform/graphics/Font.cpp
>index cc94d6cdcff..a743e9cc031 100644
>--- a/Source/WebCore/platform/graphics/Font.cpp
>+++ b/Source/WebCore/platform/graphics/Font.cpp
>@@ -33,6 +33,7 @@
> #if PLATFORM(COCOA)
> #include <pal/spi/cocoa/CoreTextSPI.h>
> #endif
>+#include "CharacterProperties.h"
> #include "FontCache.h"
> #include "FontCascade.h"
> #include "OpenTypeMathData.h"
>@@ -641,9 +642,9 @@ bool Font::variantCapsSupportsCharacterForSynthesis(FontVariantCaps fontVariantC
>     }
> }
> 
>-bool Font::platformSupportsCodePoint(UChar32 character) const
>+bool Font::platformSupportsCodePoint(UChar32 character, Optional<UChar32> variation) const
> {
>-    return glyphForCharacter(character);
>+    return variation ? false : glyphForCharacter(character);
> }
> #endif
> 
>@@ -671,7 +672,23 @@ bool Font::canRenderCombiningCharacterSequence(const UChar* characters, size_t l
> {
>     ASSERT(isMainThread());
> 
>-    for (UChar32 codePoint : StringView(characters, length).codePoints()) {
>+    auto codePoints = StringView(characters, length).codePoints();
>+    auto it = codePoints.begin();
>+    auto end = codePoints.end();
>+    while (it != end) {
>+        auto codePoint = *it;
>+        ++it;
>+
>+        if (it != end && isVariationSelector(*it)) {
>+            if (!platformSupportsCodePoint(codePoint, *it)) {
>+                // Try the characters individually.
>+                if (!supportsCodePoint(codePoint) || !supportsCodePoint(*it))
>+                    return false;
>+            }
>+            ++it;
>+            continue;
>+        }
>+
>         if (!supportsCodePoint(codePoint))
>             return false;
>     }
>diff --git a/Source/WebCore/platform/graphics/Font.h b/Source/WebCore/platform/graphics/Font.h
>index 11df1e6b9f6..3ca978862d9 100644
>--- a/Source/WebCore/platform/graphics/Font.h
>+++ b/Source/WebCore/platform/graphics/Font.h
>@@ -175,7 +175,7 @@ public:
>     GlyphData glyphDataForCharacter(UChar32) const;
>     Glyph glyphForCharacter(UChar32) const;
>     bool supportsCodePoint(UChar32) const;
>-    bool platformSupportsCodePoint(UChar32) const;
>+    bool platformSupportsCodePoint(UChar32, Optional<UChar32> variation = WTF::nullopt) const;
> 
>     RefPtr<Font> systemFallbackFontForCharacter(UChar32, const FontDescription&, IsForPlatformFont) const;
> 
>diff --git a/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp b/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp
>index 0704635384c..a0882f7c2cf 100644
>--- a/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp
>+++ b/Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp
>@@ -115,11 +115,18 @@ const Font* FontCascade::fontForCombiningCharacterSequence(const UChar* original
>         return nullptr;
> 
>     bool isEmoji = characterSequenceIsEmoji(iterator, character, clusterLength);
>+    bool preferColoredFont = isEmoji;
>+    // U+FE0E forces text style.
>+    // U+FE0F forces emoji style.
>+    if (characters[length - 1] == 0xFE0E)
>+        preferColoredFont = false;
>+    else if (characters[length - 1] == 0xFE0F)
>+        preferColoredFont = true;
> 
>     const Font* baseFont = glyphDataForCharacter(character, false, NormalVariant).font;
>     if (baseFont
>         && (clusterLength == length || baseFont->canRenderCombiningCharacterSequence(characters, length))
>-        && (!isEmoji || baseFont->platformData().isColorBitmapFont()))
>+        && (!preferColoredFont || baseFont->platformData().isColorBitmapFont()))
>         return baseFont;
> 
>     for (unsigned i = 0; !fallbackRangesAt(i).isNull(); ++i) {
>@@ -127,12 +134,16 @@ const Font* FontCascade::fontForCombiningCharacterSequence(const UChar* original
>         if (!fallbackFont || fallbackFont == baseFont)
>             continue;
> 
>-        if (fallbackFont->canRenderCombiningCharacterSequence(characters, length) && (!isEmoji || fallbackFont->platformData().isColorBitmapFont()))
>+        if (fallbackFont->canRenderCombiningCharacterSequence(characters, length) && (!preferColoredFont || fallbackFont->platformData().isColorBitmapFont()))
>             return fallbackFont;
>     }
> 
>-    if (auto systemFallback = FontCache::singleton().systemFallbackForCharacters(m_fontDescription, baseFont, IsForPlatformFont::No, isEmoji ? FontCache::PreferColoredFont::Yes : FontCache::PreferColoredFont::No, characters, length)) {
>-        if (systemFallback->canRenderCombiningCharacterSequence(characters, length) && (!isEmoji || systemFallback->platformData().isColorBitmapFont()))
>+    if (auto systemFallback = FontCache::singleton().systemFallbackForCharacters(m_fontDescription, baseFont, IsForPlatformFont::No, preferColoredFont ? FontCache::PreferColoredFont::Yes : FontCache::PreferColoredFont::No, characters, length)) {
>+        if (systemFallback->canRenderCombiningCharacterSequence(characters, length) && (!preferColoredFont || systemFallback->platformData().isColorBitmapFont()))
>+            return systemFallback.get();
>+
>+        // In case of emoji, if fallback font is colored try again without the variation selector character.
>+        if (isEmoji && characters[length - 1] == 0xFE0F && systemFallback->platformData().isColorBitmapFont() && systemFallback->canRenderCombiningCharacterSequence(characters, length - 1))
>             return systemFallback.get();
>     }
> 
>diff --git a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>index 89c25715306..7007af46ad5 100644
>--- a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>+++ b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>@@ -613,8 +613,11 @@ Path Font::platformPathForGlyph(Glyph glyph) const
>     return adoptCF(CGPathCreateMutableCopy(result.get()));
> }
> 
>-bool Font::platformSupportsCodePoint(UChar32 character) const
>+bool Font::platformSupportsCodePoint(UChar32 character, Optional<UChar32> variation) const
> {
>+    if (variation)
>+        return false;
>+
>     UniChar codeUnits[2];
>     CGGlyph glyphs[2];
>     CFIndex count = 0;
>diff --git a/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp b/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp
>index c91542936f2..cd082ca5057 100644
>--- a/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp
>+++ b/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp
>@@ -201,11 +201,11 @@ bool Font::variantCapsSupportsCharacterForSynthesis(FontVariantCaps fontVariantC
>     }
> }
> 
>-bool Font::platformSupportsCodePoint(UChar32 character) const
>+bool Font::platformSupportsCodePoint(UChar32 character, Optional<UChar32> variation) const
> {
>     CairoFtFaceLocker cairoFtFaceLocker(m_platformData.scaledFont());
>     if (FT_Face face = cairoFtFaceLocker.ftFace())
>-        return !!FcFreeTypeCharIndex(face, character);
>+        return variation ? !!FT_Face_GetCharVariantIndex(face, character, variation.value()) : !!FcFreeTypeCharIndex(face, character);
> 
>     return false;
> }
>diff --git a/Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp b/Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp
>index 360ffa1976a..69c8c90f05f 100644
>--- a/Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp
>+++ b/Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp
>@@ -78,7 +78,7 @@ static hb_font_funcs_t* harfBuzzFontFunctions()
>             CairoFtFaceLocker cairoFtFaceLocker(scaledFont);
>             if (FT_Face ftFace = cairoFtFaceLocker.ftFace()) {
>                 *glyph = FT_Face_GetCharVariantIndex(ftFace, unicode, variation);
>-                return true;
>+                return !!*glyph;
>             }
>             return false;
>             }, nullptr, nullptr);

Flags:

mcatanzaro: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 197838: 369732 | 369824