Summary: | [Freetype] Doesn't support coloured fonts | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bastien Nocera <bugzilla> | ||||||||||
Component: | Text | Assignee: | 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
Bastien Nocera
2016-04-14 07:19:46 PDT
Can we do this unconditionally? Presumably there would not be any performance issue? (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. Bastien, could you share a screenshot of gedit showing that file with cairo patched? Created attachment 284324 [details]
emoji modifiers
With Emoji One replacing the monospace font.
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.
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? (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. TBH I think this is going to be a "patches welcome" situation. We do not have any developers who understand fonts. 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 (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. Created attachment 306238 [details] WIP patch screenshot: https://twitter.com/fujii0/status/849448580884680705 (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 (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. (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 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? 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. 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? I think so. Unfortunately, I'm busy this month, have no time to confirm that. 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).
Cool! Looks good to me. Thank you very much. Committed r221909: <http://trac.webkit.org/changeset/221909> There are some tests failing that look like actual bugs/regressions. I'll investigate them, help with them is of course welcome. Will you handle rebaselining the WPE bot? (In reply to Michael Catanzaro from comment #23) > Will you handle rebaselining the WPE bot? Carlos Lopez will do it. (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 (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. (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. As I said, there are real regressions, I'm looking at them, so please don't roll it out. |