[Harfbuzz] Material icons not rendered correctly when using the web font

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

You can see it here:

https://material.io/icons/

Only a few of them are correctly rendered and some others are wrong. We only render correctly the ones that doesn't have an underscore in its 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 one 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 character 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.

[Carlos Garcia Campos]

Created attachment 320891 [details]
Patch

[Michael Catanzaro]

Comment on attachment 320891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320891&action=review

> Source/WebCore/ChangeLog:20
> +        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.

r=me if you file a new bug report.

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:379
> +        if (isASCII(character) && isASCII(previousCharacter))

Why the isASCII check? Did this fix something? Surely that's not going to work for e.g. curly quotation marks.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 320891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320891&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        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.
> 
> r=me if you file a new bug report.

Sure, I will.

> > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:379
> > +        if (isASCII(character) && isASCII(previousCharacter))
> 
> Why the isASCII check? Did this fix something? Surely that's not going to
> work for e.g. curly quotation marks.

Just tried to be conservative here. Why do you think curly quotation marks are going to be broken?

[Michael Catanzaro]

I mean if this heuristic is extended to more characters besides underscores, like quotation marks, as you indicated needs to be done in the FIXME, then obviously it's not going to work for curly quotes as you have an ASCII check. I don't understand what the ASCII check is for. Why is it relevant that both characters be ASCII?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> I mean if this heuristic is extended to more characters besides underscores,
> like quotation marks, as you indicated needs to be done in the FIXME, then
> obviously it's not going to work for curly quotes as you have an ASCII
> check. I don't understand what the ASCII check is for. Why is it relevant
> that both characters be ASCII?

Because if both are ascii it's safer to use the same run, in all other cases we are using a different run, like current code does. Note also, that this check only happens when the next character script is different than the current one and it's Common.

[Sergio Villar Senin]

The fix is awesome but we should try hard to have test cases for this, otherwise we're going to eventually break it.

[Carlos Garcia Campos]

(In reply to Sergio Villar Senin from comment #6)
> The fix is awesome but we should try hard to have test cases for this,
> otherwise we're going to eventually break it.

I could add the material design icons to wkgtk fonts test repo, and add a test case. I think the license is apache2, I don't know if we can add it to WebKit to make a cross-platform test instead. A good ref test could be to use the font names in the test and the unicode code directly in the reference, since the unicode code always worked, the problem was only with icon names.

<i class="material-icons">3d_rotation</i> <- doesn't work
<i class="material-icons">&#xE84D;</i> <- works

[Carlos Garcia Campos]

Committed r222090: <http://trac.webkit.org/changeset/222090>

[Radar WebKit Bug Importer]

<rdar://problem/34693390>