With Fedora 27, GTK now renders color emojis, which took me by surprise as I now see them rendered properly in Evolution's messages listview widget... but they don't get rendered correctly in the webkit view of the messages themselves. Opening http://www.fileformat.info/info/emoji/browsertest.htm with Epiphany also makes me suspect that color emojis are not implemented in WebkitGtk... am I correct in presuming it's a webkitgtk issue and that once it is done there it will automagically work in applications like Evolution?
You don't get color emoji? Did you try turning your computer upside-down? That is the first thing you should check before reporting a bug. OK, enough trolling... yes, I enjoyed that.... (In reply to Jeff Fortin from comment #0) > Opening http://www.fileformat.info/info/emoji/browsertest.htm with Epiphany > also makes me suspect that color emojis are not implemented in WebkitGtk... > am I correct in presuming it's a webkitgtk issue and that once it is done > there it will automagically work in applications like Evolution? In theory, we already support color emoji since bug #156579. I don't know if that got backported to 2.18 or not, so you might need 2.20. But I've never seen it work. Your test page does not work in Epiphany Tech Preview. Maybe GNOME's flatpak runtime is pinned to an older version of some dependency, though it uses git master for most dependencies... but it also doesn't work in Fedora 27, which should have the latest versions of all relevant dependencies. I wonder what is wrong. Maybe Carlos Garcia would know.
Note: many emoji will look like crap due to bug #177040, but they should at least be colorful!
jhbuild patch to bump Cairo 1.15.10 https://gist.github.com/fujii/05ae2fb4f2e3d92bcef3b44647f8c876 ./Tools/jhbuild/jhbuild-wrapper --gtk uninstall cairo ./Tools/jhbuild/jhbuild-wrapper --gtk buildone cairo Screenshot. https://photos.app.goo.gl/2Lqm0Lll6BToh2bo2 Something wrong with the font selections. I need to download the test case (Bug Comment 5) to show color emoji. I don't know how GTK selects fonts. I might need to configure my Linux box.
You need an emoji font installed. In this case the problem is not that we don't support colored emojis. The ones we are showing are not taken from the emoji font, but from other font that contains the character. So, the problem here is that we are not looking for the character in the emoji font for whatever reason.
Font fallbacks are indeed working fine, here the emojione font is selected, but we end up rendering tiny icons instead. This is because for some reason the matrix we are getting from font config contains a scale, which we don't expect. We only get the font matrix to appply rotations in case of oblique fonts, but then we always apply the scale for the computed pixel font size. Ignoring the fc matrix scale fixes the issue, see this screenshot: https://people.igalia.com/cgarcia/mb-emojis.png I'll submit a patch.
Created attachment 334693 [details] Patch I haven't had time to run the tests with the patch, I'll do it tomorrow.
Comment on attachment 334693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334693&action=review > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:73 > + bool italic = fontDescription.italic(); > + if (!FcPatternAddInteger(pattern, FC_SLANT, italic ? FC_SLANT_ITALIC : FC_SLANT_ROMAN)) Since you're moving this code, might as well take the opportunity to get rid of the unnecessary local variable and do fontDescription.italic() ? FC_SLANT_ITALIC : FC_SLANT_ROMAN > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:98 > + if (!configurePatternForFontDescription(pattern.get(), fontDescription)) > + return nullptr; Nit: to make this more readable, add one blank line above and below. It's unrelated to forcing scalable fonts and unrelated to the FcConfigSubstitute/cairo_ft_font_options_substitute/FcDefaultSubstitute dance. > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:112 > - return FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult); > + return adoptRef(FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult)); Good catch. > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:126 > + // Try first a direct match. > + FcResult fontConfigResult; > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) { > + FontPlatformData alternateFontData(resultPattern.get(), description); > + return fontForPlatformData(alternateFontData); > + } I'm pretty sure this is wrong. I think Fontconfig will always return a pattern here, whatever works best. Now you'll never consider the fallback case. In short: I suspect this disables CSS font fallback. E.g. if you have, say: font: Cantarell; Liberation Sans; sans I suspect that, with this change, fontconfig will always return some best-effort result for Cantarell, whatever its closest match is, without ever considering the fallbacks Liberation Sans or sans. If I'm wrong, please explain why. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:310 > - cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx, > - -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0); > + > + cairo_matrix_init(&fontMatrix, 1, -fontConfigMatrix.yx, -fontConfigMatrix.xy, 1, 0, 0); Is ignoring the scale really the right thing to do? This seems quite doubtful....
> You don't get color emoji? Did you try turning your computer upside-down? I did, but all I got was this 🙃 (and it was still in black & white) This time I don't think it's my computer overheating ;)
(In reply to Michael Catanzaro from comment #7) > Comment on attachment 334693 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334693&action=review > > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:73 > > + bool italic = fontDescription.italic(); > > + if (!FcPatternAddInteger(pattern, FC_SLANT, italic ? FC_SLANT_ITALIC : FC_SLANT_ROMAN)) > > Since you're moving this code, might as well take the opportunity to get rid > of the unnecessary local variable and do fontDescription.italic() ? > FC_SLANT_ITALIC : FC_SLANT_ROMAN Sure. > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:98 > > + if (!configurePatternForFontDescription(pattern.get(), fontDescription)) > > + return nullptr; > > Nit: to make this more readable, add one blank line above and below. It's > unrelated to forcing scalable fonts and unrelated to the > FcConfigSubstitute/cairo_ft_font_options_substitute/FcDefaultSubstitute > dance. Ok. > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:112 > > - return FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult); > > + return adoptRef(FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult)); > > Good catch. > > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:126 > > + // Try first a direct match. > > + FcResult fontConfigResult; > > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) { > > + FontPlatformData alternateFontData(resultPattern.get(), description); > > + return fontForPlatformData(alternateFontData); > > + } > > I'm pretty sure this is wrong. I think Fontconfig will always return a > pattern here, whatever works best. Now you'll never consider the fallback > case. In short: I suspect this disables CSS font fallback. E.g. if you have, > say: > > font: Cantarell; Liberation Sans; sans > > I suspect that, with this change, fontconfig will always return some > best-effort result for Cantarell, whatever its closest match is, without > ever considering the fallbacks Liberation Sans or sans. > > If I'm wrong, please explain why. Yes, but only in part :-) It seems you are right that direct match always returns a pattern, so the fallbacks code is never used. But this has nothing to do with CSS fallbacks, those are handled by WebCore, not by fonconfig. The CSS fallbacks are used when we fail to find a font for the given family, then trying with the next fallbacks. In all those cases FontCache::createFontPlatformData() is called with the font family. When we have a match for the font family, but we fail to find a glyph, then we try the system fallbacks, ignoring the font family simply looking for a font that contains glyphs for the given characters. In this case is when we call FontCache::systemFallbackForCharacters(). So, it seems to me that in the case of system fallbacks the direct match and the fallbacks match are doing pretty much the same thing, so we can simply get rid of the fontconfig fallbacks. I've made several tests cases with a few debug output to see how this works (with this patch applied): - test-glyph-in-css-fallbacks.html <span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">①</span> DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: draw glyphs with DejaVuSans Cantarell doesn't have a glyph for the character, and libration either, so we end up using DejaVuSans. Note that systemFallbackForCharacters() is not called at all, because there's a font in the CSS fallbacks that contains the glyph. - test-main-font-not-found-glyph-in-css-fallbacks.html <span style="font-family: TheGreatFooFont, Cantarell, sans; font-size: 128px">≈</span> DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: No matchedFontFamily for TheGreatFooFont - TheGreatFooFont DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf DBG: draw glyphs with Cantarell-Regular The first font doesn't exist, but Cantarell has a glyph to render the character, so Cantarell-Regular is used. No systemFallbackForCharacters() in this case either. - test-glyph-not-in-css-fallbacks-but-in-system.html <span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">🍔</span> DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: systemFallbackForCharacters: d83c df54 DBG: Direct fallback found for d83c df54 at /home/cgarcia/.local/share/fonts/emojione-android.ttf DBG: draw glyphs with EmojiOne It's an emoji that is not present in any of the CSS fallbacks, but it's present in EmojiOne, installed in my system. systemFallbackForCharacters() is called in this case and the direct match find the right font. Doing the fallbacks before the direct match in this case provides the exactly same result. - test-glyph-not-in-css-fallbacks-nor-in-system.html <span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">🏱</span> DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: systemFallbackForCharacters: d83c dff1 DBG: Direct fallback found for d83c dff1 at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf DBG: draw glyphs with Cantarell-Regular There isn't any font in the system with the glyph required to render the character, so the system fallback returned is DejaVuSans, but since it doesn't contain the glyph either, the main font is used to render the square that indicates there's no glyph. In this case doing the fallbacks before the direct match provides the exactly same result. So, I think we can get rid of the fontconfig fallbacks entirely. They were implemented in 2008 (see r36309) and only the fallbacks were used in FontCache::getFontDataForCharacters(). Then, the direct match after the fallbacks was added in 2010 (see r70688), including a layout test (platform/gtk/fonts/custom-font-missing-glyphs.html) that still passes with this patch. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:310 > > - cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx, > > - -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0); > > + > > + cairo_matrix_init(&fontMatrix, 1, -fontConfigMatrix.yx, -fontConfigMatrix.xy, 1, 0, 0); > > Is ignoring the scale really the right thing to do? This seems quite > doubtful.... We don't expect to get a scale change from the fontconfig matrix, that's what is causing this bug. See the emoji browser test linked in description, we are actually rendering the icons using emojione, but they are tiny, because the scale is broken. See the comments in the code: // FontConfig may return a list of transformation matrices with the pattern, for instance, // for fonts that are oblique. We use that to initialize the cairo font matrix. We don't expect a scale change, only in rotation for oblique fonts. // The matrix from FontConfig does not include the scale. Scaling a font with width zero size leads // to a failed cairo_scaled_font_t instantiations. Instead we scale the font to a very tiny // size and just abort rendering later on. Even clearer in this comment.
Note that in some cases we still don't get colored emojis, this is because the glyph is found in another font before emojione, for example U+231A the watch is found in several fonts in my system. We only get the colored emoji one when Emoji One is used explicitly in the font family.
Created attachment 334735 [details] Updated patch Addressed review comments. I've left the fallbacks changes out in the end, because it's kind of unrelated to this patch. I'll handle that in a separate bug. This patch doesn't introduce any regression (nor progression) in layout tests.
Comment on attachment 334735 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=334735&action=review I still don't understand why ignoring the scale is correct, but since this doesn't break anything, let's do it. How hard is it to add a test for this? You could just have an HTML page that displays a single character and save the expected result as an image, right? Then we could at least see that it is colored. > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:132 > FcResult fontConfigResult; > - RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult)); > - if (!resultPattern) > - return nullptr; > - FontPlatformData alternateFontData(resultPattern.get(), description); > - return fontForPlatformData(alternateFontData); > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) { > + FontPlatformData alternateFontData(resultPattern.get(), description); > + return fontForPlatformData(alternateFontData); > + } I don't think this change is unrelated. It used to be live code, and now you've turned it into dead code, right? But very subtle dead code that most developers would not notice. So I would address bug #183210 in this same patch. Up to you, as long as you follow up promptly so that we don't forget about this.
(In reply to Michael Catanzaro from comment #12) > Comment on attachment 334735 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334735&action=review > > I still don't understand why ignoring the scale is correct, but since this > doesn't break anything, let's do it. > > How hard is it to add a test for this? You could just have an HTML page that > displays a single character and save the expected result as an image, right? > Then we could at least see that it is colored. > > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:132 > > FcResult fontConfigResult; > > - RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult)); > > - if (!resultPattern) > > - return nullptr; > > - FontPlatformData alternateFontData(resultPattern.get(), description); > > - return fontForPlatformData(alternateFontData); > > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) { > > + FontPlatformData alternateFontData(resultPattern.get(), description); > > + return fontForPlatformData(alternateFontData); > > + } > > I don't think this change is unrelated. It used to be live code, and now > you've turned it into dead code, right? But very subtle dead code that most > developers would not notice. So I would address bug #183210 in this same > patch. Up to you, as long as you follow up promptly so that we don't forget > about this. Why was it live code? I haven't changed anything here in behavior, it's just a cleanup.
Comment on attachment 334735 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=334735&action=review >>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:132 >>> + } >> >> I don't think this change is unrelated. It used to be live code, and now you've turned it into dead code, right? But very subtle dead code that most developers would not notice. So I would address bug #183210 in this same patch. Up to you, as long as you follow up promptly so that we don't forget about this. > > Why was it live code? I haven't changed anything here in behavior, it's just a cleanup. Oops, I didn't look closely enough... I see this is different from your original patch. Yes, moving the declarations into the conditional tests is good. OK then!
(In reply to Michael Catanzaro from comment #12) > Comment on attachment 334735 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334735&action=review > > I still don't understand why ignoring the scale is correct, but since this > doesn't break anything, let's do it. > > How hard is it to add a test for this? You could just have an HTML page that > displays a single character and save the expected result as an image, right? > Then we could at least see that it is colored. Again, the problem is not that emojis aren't colored, they are. The problem is that we are applying the wrong matrix and the scale makes the icons tiny, almost invisible. This only happens when the font is used as a system fallback, so a possible test would be to use Emoji One explicitly in the reference and the default font in the test for the same character. Both should render the same emoji at the same scale. I wrote this test indeed, but it didn't work because emojis are not positioned at the same place in both cases. This is because of the font line, used to position the characters in a line. In the case of using Emoji One explicitly the font line is Emoji One, so the metrics used for the line are the Emoji One. But then Emoji One is used as a fallback DejaVu or Liberation are the font line and the metrics used are not the Emoji One. At first I thought it was a WebKit bug, but then I noticed that Firefox and Chromium do exactly the same thing. I'll try to make a non-ref test, but I'm not sure if the render tree is different. > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:132 > > FcResult fontConfigResult; > > - RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult)); > > - if (!resultPattern) > > - return nullptr; > > - FontPlatformData alternateFontData(resultPattern.get(), description); > > - return fontForPlatformData(alternateFontData); > > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) { > > + FontPlatformData alternateFontData(resultPattern.get(), description); > > + return fontForPlatformData(alternateFontData); > > + } > > I don't think this change is unrelated. It used to be live code, and now > you've turned it into dead code, right? But very subtle dead code that most > developers would not notice. So I would address bug #183210 in this same > patch. Up to you, as long as you follow up promptly so that we don't forget > about this.
Committed r229128: <https://trac.webkit.org/changeset/229128>
I would use a non-ref test, because a reftest would be difficult. And I would use an image for the expected results rather than the render tree. That way it will be obvious enough if the emoji is no longer there.
(In reply to Michael Catanzaro from comment #17) > I would use a non-ref test, because a reftest would be difficult. And I > would use an image for the expected results rather than the render tree. > That way it will be obvious enough if the emoji is no longer there. We don't have pixels tests enabled. And the thing is that this is not reproducible in the test env because of our custom fontconfig configuration.
(In reply to Carlos Garcia Campos from comment #18) > (In reply to Michael Catanzaro from comment #17) > > I would use a non-ref test, because a reftest would be difficult. And I > > would use an image for the expected results rather than the render tree. > > That way it will be obvious enough if the emoji is no longer there. > > We don't have pixels tests enabled. Ah, yes, good point... the pixel tests are basically useless, then. > And the thing is that this is not > reproducible in the test env because of our custom fontconfig configuration. That's not a problem, because we control the configuration. It's kind of expected that it will have to be updated when adding new font tests. But I think the only sane way to do this would be as a pixel test, and there's no value in adding pixel tests, so I wouldn't worry about it.