Changeset 222090 in webkit

Timestamp:

Sep 15, 2017 8:37:02 AM (7 years ago)

Author:

Carlos Garcia Campos

Message:

[Harfbuzz] Material icons not rendered correctly when using the web font
​https://bugs.webkit.org/show_bug.cgi?id=176995

Reviewed by Michael Catanzaro.

Only a few of them are correctly rendered and some others are wrong. We only render correctly the ones that
don't have an underscore in their name (or that start with a number like 3d_rotation). In the cases where the
name before the underscore is also an icon, we render that icon instead, that's why some of them are wrong. This
is happening because the underscore is causing the HarfbuffShaper to split the text in 3 runs, one for the word
before the underscore, another one for the underscore and another for the word after the underscore. So, we
end up trying to shape the 3 runs independently and we fail when the icon doesn't exist, or when it exists but
it's not the one we are looking for. The cause of this is that the underscore has a different script (Common)
than the rest of characters (Latin) which is a condition in HarfbuffShaper to create a different run. The
unicode spec says that characters with Common script should be handled differently, but we are just ignoring
it. The spec proposes to use an heuristic based on simply inheriting the script of the previous character, which
should work in most of the cases. We could take a more conservative approach and do that only if both characters
are ASCII. We should also consider handling other cases mentioned by the spec like brackets and quotation marks,
but that belongs to a different bug/commit.

• platform/graphics/harfbuzz/HarfBuzzShaper.cpp:

(WebCore::scriptsAreCompatibleForCharacters): Helper function to check if the current and previous scripts are
compatible,
(WebCore::HarfBuzzShaper::collectHarfBuzzRuns): Use scriptsAreCompatibleForCharacters() to decided whether to
finish the current run or not. In case of Common script, inherit also the script from the previous character.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/harfbuzz/HarfBuzzShaper.cpp (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-09-15  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [Harfbuzz] Material icons not rendered correctly when using the web font
+        https://bugs.webkit.org/show_bug.cgi?id=176995
+
+        Reviewed by Michael Catanzaro.
+
+        Only a few of them are correctly rendered and some others are wrong. We only render correctly the ones that
+        don't have an underscore in their name (or that start with a number like 3d_rotation). In the cases where the
+        name before the underscore is also an icon, we render that icon instead, that's why some of them are wrong. This
+        is happening because the underscore is causing the HarfbuffShaper to split the text in 3 runs, one for the word
+        before the underscore, another one for the underscore and another for the word after the underscore. So, we
+        end up trying to shape the 3 runs independently and we fail when the icon doesn't exist, or when it exists but
+        it's not the one we are looking for. The cause of this is that the underscore has a different script (Common)
+        than the rest of characters (Latin) which is a condition in HarfbuffShaper to create a different run. The
+        unicode spec says that characters with Common script should be handled differently, but we are just ignoring
+        it. The spec proposes to use an heuristic based on simply inheriting the script of the previous character, which
+        should work in most of the cases. We could take a more conservative approach and do that only if both characters
+        are ASCII. We should also consider handling other cases mentioned by the spec like brackets and quotation marks,
+        but that belongs to a different bug/commit.
+
+        * platform/graphics/harfbuzz/HarfBuzzShaper.cpp:
+        (WebCore::scriptsAreCompatibleForCharacters): Helper function to check if the current and previous scripts are
+        compatible,
+        (WebCore::HarfBuzzShaper::collectHarfBuzzRuns): Use scriptsAreCompatibleForCharacters() to decided whether to
+        finish the current run or not. In case of Common script, inherit also the script from the previous character.
+
 2017-09-15  Carlos Garcia Campos  <cgarcia@igalia.com>
 

trunk/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp

@@ -360 +360 @@
 }
 
+static bool scriptsAreCompatibleForCharacters(UScriptCode script, UScriptCode previousScript, UChar32 character, UChar32 previousCharacter)
+{
+    if (script == previousScript)
+        return true;
+
+    if (script == USCRIPT_INHERITED || previousScript == USCRIPT_COMMON)
+        return true;
+
+    if (script == USCRIPT_COMMON) {
+        // §5.1 Handling Characters with the Common Script Property.
+        // Programs must resolve any of the special Script property values, such as Common,
+        // based on the context of the surrounding characters. A simple heuristic uses the
+        // script of the preceding character, which works well in many cases.
+        // http://www.unicode.org/reports/tr24/#Common.
+        //
+        // FIXME: cover all other cases mentioned in the spec (ie. brackets or quotation marks).
+        // https://bugs.webkit.org/show_bug.cgi?id=177003.
+        //
+        // We use a slightly more conservative heuristic than the one proposed in the spec,
+        // using the script of the previous character only if both are ASCII.
+        if (isASCII(character) && isASCII(previousCharacter))
+            return true;
+    }
+
+    return uscript_hasScript(character, previousScript);
+}
+
 bool HarfBuzzShaper::collectHarfBuzzRuns()
 {
@@ -382 +409 @@
             currentFontData = &m_font->primaryFont();
         UScriptCode currentScript = nextScript;
+        UChar32 previousCharacter = character;
 
         for (iterator.advance(clusterLength); iterator.consume(character, clusterLength); iterator.advance(clusterLength)) {
@@ -411 +439 @@
             if (U_FAILURE(errorCode))
                 return false;
-            if ((nextFontData != currentFontData) || ((currentScript != nextScript) && (nextScript != USCRIPT_INHERITED) && (!uscript_hasScript(character, currentScript))))
+
+            if (nextFontData != currentFontData)
                 break;
-            if (nextScript == USCRIPT_INHERITED)
+
+            if (!scriptsAreCompatibleForCharacters(nextScript, currentScript, character, previousCharacter))
+                break;
+
+            if (nextScript == USCRIPT_INHERITED || nextScript == USCRIPT_COMMON)
                 nextScript = currentScript;
+
             currentCharacterPosition = iterator.characters();
+            previousCharacter = character;
         }
         unsigned numCharactersOfCurrentRun = iterator.currentIndex() - startIndexOfCurrentRun;