Bug 156579

Summary: [Freetype] Doesn't support coloured fonts
Product: WebKit Reporter: Bastien Nocera <bugzilla>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bugs-noreply, cgarcia, clopez, Hironori.Fujii, mcatanzaro, mmaxfield, mrobinson, Ms2ger, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 176819, 176820    
Bug Blocks:    
Attachments:
Description Flags
emoji modifiers
none
Test
none
WIP patch
none
Patch mcatanzaro: review+

Description Bastien Nocera 2016-04-14 07:19:46 PDT
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.
Comment 1 Michael Catanzaro 2016-04-14 09:24:12 PDT
Can we do this unconditionally? Presumably there would not be any performance issue?
Comment 2 Myles C. Maxfield 2016-04-14 09:37:09 PDT
(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.
Comment 3 Carlos Garcia Campos 2016-07-22 00:52:14 PDT
Bastien, could you share a screenshot of gedit showing that file with cairo patched?
Comment 4 Bastien Nocera 2016-07-22 03:49:23 PDT
Created attachment 284324 [details]
emoji modifiers

With Emoji One replacing the monospace font.
Comment 5 Carlos Garcia Campos 2016-07-22 04:56:18 PDT
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.
Comment 6 Michael Catanzaro 2016-10-11 13:22:26 PDT
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?
Comment 7 Bastien Nocera 2016-10-11 21:04:32 PDT
(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.
Comment 8 Michael Catanzaro 2016-10-12 09:34:40 PDT
TBH I think this is going to be a "patches welcome" situation. We do not have any developers who understand fonts.
Comment 9 Fujii Hironori 2017-03-14 03:26:02 PDT
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
Comment 10 Bastien Nocera 2017-03-20 08:10:25 PDT
(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.
Comment 11 Fujii Hironori 2017-04-04 19:29:59 PDT
Created attachment 306238 [details]
WIP patch

screenshot:
https://twitter.com/fujii0/status/849448580884680705
Comment 12 Carlos Garcia Campos 2017-04-05 01:09:46 PDT
(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
Comment 13 Fujii Hironori 2017-04-05 03:08:10 PDT
(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.
Comment 14 Bastien Nocera 2017-04-05 04:11:22 PDT
(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.
Comment 15 Michael Catanzaro 2017-05-24 19:00:34 PDT
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?
Comment 16 Carlos Garcia Campos 2017-08-05 03:20:36 PDT
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.
Comment 17 Carlos Garcia Campos 2017-09-08 01:46:25 PDT
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?
Comment 18 Fujii Hironori 2017-09-08 05:29:20 PDT
I think so. Unfortunately, I'm busy this month, have no time to confirm that.
Comment 19 Carlos Garcia Campos 2017-09-11 09:50:01 PDT
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).
Comment 20 Fujii Hironori 2017-09-11 18:01:50 PDT
Cool! Looks good to me. Thank you very much.
Comment 21 Carlos Garcia Campos 2017-09-12 04:21:46 PDT
Committed r221909: <http://trac.webkit.org/changeset/221909>
Comment 22 Carlos Garcia Campos 2017-09-12 07:06:13 PDT
There are some tests failing that look like actual bugs/regressions. I'll investigate them, help with them is of course welcome.
Comment 23 Michael Catanzaro 2017-09-12 10:07:53 PDT
Will you handle rebaselining the WPE bot?
Comment 24 Carlos Garcia Campos 2017-09-12 10:53:17 PDT
(In reply to Michael Catanzaro from comment #23)
> Will you handle rebaselining the WPE bot?

Carlos Lopez will do it.
Comment 25 Carlos Alberto Lopez Perez 2017-09-13 06:02:11 PDT
(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
Comment 26 Carlos Alberto Lopez Perez 2017-09-13 06:03:28 PDT
(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.
Comment 27 Michael Catanzaro 2017-09-13 06:57:24 PDT
(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.
Comment 28 Carlos Garcia Campos 2017-09-13 07:00:22 PDT
As I said, there are real regressions, I'm looking at them, so please don't roll it out.
Comment 29 Radar WebKit Bug Importer 2017-09-27 12:53:38 PDT
<rdar://problem/34694224>