Bug 156579 – [Freetype] Doesn't support coloured fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Bastien Nocera]

This font:
https://github.com/Ranks/emojione/blob/master/assets/fonts/emojione-android.ttf
has coloured glyphs.

Freetype supports this type of glyph when passed the FT_LOAD_COLOR load flag, as done in this cairo branch:
https://github.com/matthiasclasen/cairo/tree/matthiasc/color-emoji-4

This cairo patch is enough to make GTK+ render the font correctly, but as WebKitGTK+ uses freetype directly, it doesn't work in Epiphany.

Example page:
https://git.gnome.org/browse/pango/tree/pango-view/EMOJI-MODIFIERS.txt

This works when shown in GTK+/gedit after forcing the font.

[Michael Catanzaro]

Can we do this unconditionally? Presumably there would not be any performance issue?

[Myles C. Maxfield]

(In reply to comment #1)
> Can we do this unconditionally? Presumably there would not be any
> performance issue?

THe Mac and iOS ports will show SBIX (colored) glyphs unconditionally.

[Carlos Garcia Campos]

Bastien, could you share a screenshot of gedit showing that file with cairo patched?

[Bastien Nocera]

Created attachment 284324 [details]
emoji modifiers

With Emoji One replacing the monospace font.

[Carlos Garcia Campos]

Created attachment 284325 [details]
Test

I have converted the EMOJI-MODIFIERS.txt into a simple HTML test. You need to download the font and save it in the same dir than the test file. It works fine with chromium, WebKitGTK+ and firefox show more or less the same thing.

[Michael Catanzaro]

So what exactly needs done here? We just need to pass FT_LOAD_COLOR... where?

Carlos says the test already works fine, but it requires some font so it doesn't display properly for me. Which font?

[Bastien Nocera]

(In reply to comment #6)
> So what exactly needs done here? We just need to pass FT_LOAD_COLOR... where?
> 
> Carlos says the test already works fine, but it requires some font so it
> doesn't display properly for me. Which font?

The goal is for the test to work after installing the modified cairo and emoji font at:
https://copr.fedorainfracloud.org/coprs/hadess/emoji/

The font is "emojione-android.ttf" as shipped in the package above, but the test didn't work for me either.

[Michael Catanzaro]

TBH I think this is going to be a "patches welcome" situation. We do not have any developers who understand fonts.

[Fujii Hironori]

I tried Behdad's cairo color emoji patch with GTK port MiniBrowser (trunk@213790).
Matthiasclasen'patch doesn't work for me.

Behdad's cairo color emoji patch
https://github.com/behdad/cairo/commits/color-emoji

jhbuild.modules patch
https://gist.github.com/fujii/25df67525f6f822c187c47a47c9ab12a

screenshot
https://twitter.com/fujii0/status/841595202041073664

[Bastien Nocera]

(In reply to comment #9)
> I tried Behdad's cairo color emoji patch with GTK port MiniBrowser
> (trunk@213790).

For a value of "works":
- fitzpatrick modifiers don't work
- ligatures don't work
- it uses and requires a copy of the font in the same place as the web page, instead of using the system's font.

> Matthiasclasen'patch doesn't work for me.

I'll investigate that.

[Fujii Hironori]

Created attachment 306238 [details]
WIP patch

screenshot:
https://twitter.com/fujii0/status/849448580884680705

[Carlos Garcia Campos]

(In reply to Fujii Hironori from comment #11)
> Created attachment 306238 [details]
> WIP patch

Awesome!

> screenshot:
> https://twitter.com/fujii0/status/849448580884680705

So, this is with newer freetype, icu and harfbuzz, but without the cairo patch?

And this one? https://twitter.com/fujii0/status/849197583503249409

[Fujii Hironori]

(In reply to Carlos Garcia Campos from comment #12)
> So, this is with newer freetype, icu and harfbuzz, but without the cairo
> patch?

Yes. Without cairo color glyph patch, FreeType automatically grayscaled embedded PNG glyphs.

Matthias's patch was rejected by upstream Cairo.
https://lists.cairographics.org/archives/cairo/2016-April/027358.html

> And this one? https://twitter.com/fujii0/status/849197583503249409

ICU 56.1 + Behdad's cairo.
Old ICU 56 seems to have a wrong width problem.

[Bastien Nocera]

(In reply to Fujii Hironori from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > So, this is with newer freetype, icu and harfbuzz, but without the cairo
> > patch?
> 
> Yes. Without cairo color glyph patch, FreeType automatically grayscaled
> embedded PNG glyphs.
> 
> Matthias's patch was rejected by upstream Cairo.
> https://lists.cairographics.org/archives/cairo/2016-April/027358.html

It wasn't rejected, it needs work.

> > And this one? https://twitter.com/fujii0/status/849197583503249409
> 
> ICU 56.1 + Behdad's cairo.
> Old ICU 56 seems to have a wrong width problem.

Good to know about the ICU requirement. I'll still need to track down the regression between Matthias' version and Behdad's. But Behdad's version will never be merged, it is very incomplete, which is what Matthias' version is supposed to add.

[Michael Catanzaro]

Comment on attachment 306238 [details]
WIP patch

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

> Source/WebCore/platform/graphics/FontCascade.h:295
> +#if PLATFORM(COCOA) || PLATFORM(GTK)

Let's try to avoid PLATFORM(GTK) conditionals... I guess this could be || USE(FREETYPE) instead, right? It should surely work for WPE, and probably also WinCairo?

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:75
> +        if (OS2Table && freeTypeFace->face_flags & FT_FACE_FLAG_SCALABLE) {

Is this fixing a different issue?

[Carlos Garcia Campos]

Cairo patch landed in master. We still need to bump freetype in jhbuild, which requires a huge rebaseline, so let's try to do it right after branching.

[Carlos Garcia Campos]

Fujii, I think we have everything now in jhbuild. Do you think we also need the cairo snapshot? I think it's only needed to render the emojis colored, since most of the emoji tests are ref-tests, we don't probably need the cairo snapshots for the tests, right?

[Fujii Hironori]

I think so. Unfortunately, I'm busy this month, have no time to confirm that.

[Carlos Garcia Campos]

Created attachment 320433 [details]
Patch

Fujii, does this look good to you? I've added the EmojiOne font to the fonts repo and updated the gtk and wpe jhbuild modulesets. This also improves the font rendering in general (better kerning in some cases) so it requires another large rebaseline (more than 2000, I don't remember exatly the number, but less than 2500 I think).

[Fujii Hironori]

Cool! Looks good to me. Thank you very much.

[Carlos Garcia Campos]

Committed r221909: <http://trac.webkit.org/changeset/221909>

[Carlos Garcia Campos]

There are some tests failing that look like actual bugs/regressions. I'll investigate them, help with them is of course welcome.

[Michael Catanzaro]

Will you handle rebaselining the WPE bot?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #23)
> Will you handle rebaselining the WPE bot?

Carlos Lopez will do it.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #24)
> (In reply to Michael Catanzaro from comment #23)
> > Will you handle rebaselining the WPE bot?
> 
> Carlos Lopez will do it.

I have done that on bug 176819.

The second rebaseline r221955: <http://trac.webkit.org/changeset/221955> odes some rebaselines with whitespace changes in several tests.

Is this something we should expect as outcome of the fix in this bug? See below  examples of those diffs


--- /home/clopez/webkit/wpe/layout-test-results/fast/tokenizer/script_extra_close-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/fast/tokenizer/script_extra_close-actual.txt
@@ -1 +1 @@
-TEST... PASSED. This text should show up.
+TEST...	PASSED. This text should show up.
--- /home/clopez/webkit/wpe/layout-test-results/fast/events/mouseover-mouseout-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/fast/events/mouseover-mouseout-actual.txt
@@ -2,7 +2,7 @@
 
 Move the mouse pointer from left to right:
 
-1               1               2345
+1               1              2345
 Log	Expected results
 mouseover on t1_1
 mouseout on t1_1

--- /home/clopez/webkit/wpe/layout-test-results/animations/width-using-ems-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/animations/width-using-ems-actual.txt
@@ -1,3 +1,3 @@
-This test performs an animation of the width property using 'em' units. It tests whether or not we are properly getting the default font-size.Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text.
+This test performs an animation of the width property using 'em' units. It tests whether or not we are properly getting the default font-size. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text.
 PASS - "width" property for "box" element at 0.333s saw something close to: 133
 

--- /home/clopez/webkit/wpe/layout-test-results/fast/events/window-events-bubble2-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/fast/events/window-events-bubble2-actual.txt
@@ -1 +1 @@
-Tests that preventDefault() will still allow window events to bubble. Clicking here should fire window.onclick. This will match Firefox behavior.Window.onClick fired. Test Passed.
+Tests that preventDefault() will still allow window events to bubble. Clicking here should fire window.onclick. This will match Firefox behavior. Window.onClick fired. Test Passed.
(END)


--- /home/clopez/webkit/wpe/layout-test-results/fast/events/window-events-bubble-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/fast/events/window-events-bubble-actual.txt
@@ -1 +1 @@
-Test that stopPropagation() will not allow window events to bubble. Clicking on this should not fire window.onclick. This matches our old behavior and Firefox behavior.stopPropagation called. Test Passed.
+Test that stopPropagation() will not allow window events to bubble. Clicking on this should not fire window.onclick. This matches our old behavior and Firefox behavior. stopPropagation called. Test Passed.



--- /home/clopez/webkit/wpe/layout-test-results/animations/simultaneous-start-transform-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/animations/simultaneous-start-transform-actual.txt
@@ -1,4 +1,4 @@
-This test performs an animation of the transform property. It animates over 10 seconds. It takes 3 snapshots and expects each result to be within a specified range.PASS - "webkitTransform" property for "box1" element at 2s saw something close to: 0.309017,0.951057
+This test performs an animation of the transform property. It animates over 10 seconds. It takes 3 snapshots and expects each result to be within a specified range. PASS - "webkitTransform" property for "box1" element at 2s saw something close to: 0.309017,0.951057
 PASS - "webkitTransform" property for "box2" element at 2s saw something close to: 0.309017,0.951057
 PASS - "webkitTransform" property for "box1" and "box2" elements at 2s are close enough to each other
 PASS - "webkitTransform" property for "box1" element at 5s saw something close to: -1,0

--- /home/clopez/webkit/wpe/layout-test-results/animations/lineheight-animation-expected.txt
+++ /home/clopez/webkit/wpe/layout-test-results/animations/lineheight-animation-actual.txt
@@ -1,3 +1,3 @@
-This test performs an animation of the line-height property. It tests whether or not we are properly getting the font-size.Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text.
+This test performs an animation of the line-height property. It tests whether or not we are properly getting the font-size. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text. Here is some text.
 PASS - "lineHeight" property for "box" element at 0.1s saw something close to: 10

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #25)

> 
> The second rebaseline r221955: <http://trac.webkit.org/changeset/221955>
> odes some rebaselines with whitespace changes in several tests.

^ The second rebaseline r221955: <http://trac.webkit.org/changeset/221955> includes some rebaselines with whitespace changes in several tests.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #25)
> /home/clopez/webkit/wpe/layout-test-results/animations/width-using-ems-
> expected.txt
> +++
> /home/clopez/webkit/wpe/layout-test-results/animations/width-using-ems-
> actual.txt
> @@ -1,3 +1,3 @@
> -This test performs an animation of the width property using 'em' units. It
> tests whether or not we are properly getting the default font-size.Here is
> some text. Here is some text. Here is some text. Here is some text. Here is
> some text. Here is some text.
> +This test performs an animation of the width property using 'em' units. It
> tests whether or not we are properly getting the default font-size. Here is
> some text. Here is some text. Here is some text. Here is some text. Here is
> some text. Here is some text.
>  PASS - "width" property for "box" element at 0.333s saw something close to:
> 133

Looking at the test, I would say the change is unexpected and should be rolled out. That will be quite a pain.

[Carlos Garcia Campos]

As I said, there are real regressions, I'm looking at them, so please don't roll it out.

[Radar WebKit Bug Importer]

<rdar://problem/34694224>