1371386 - Firefox does not always prefer `font.name-list.emoji` when showing emojis

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P3)

Description

[Thomas Wisniewski]

Attachment: testcase.html

Given the attached test-case (reduced from webcompat.com issues #6001 and #6915), Chrome and Safari render both lines with all of the glyphs taken from the Apple Color Emoji font. Firefox prefers the glyphs from Times New Roman instead.

On my MacBook, I can get Firefox to prefer Apple Color Emoji when there is no CSS font-family specified by the web page, by changing the about:config value font.name-list.serif.x-western font-list to have Apple Color Emoji as its first value.

However, if CSS specifies the font (even sans-serif or not including Times New Roman) then Firefox will still prefer the Times New Roman glyphs.

By contrast, Chrome and Safari seem to always prefer Apple Color Emoji glyphs, even the CSS specifies the font-family to be Times or Times New Roman as per the test-case.

Of course I would suggest that if we change to match the behavior of Chrome/Safari in either case, that we don't limit this glyph-preference to just Apple Color Emoji, but whatever emoji font Firefox preferes on the system (be it Apple Color Emoji to EmojiOne).

Comment 1

[Thomas Wisniewski]

Jonathan, is there something specific I should CC on this bug?

Comment 2

[Jonathan Kew [:jfkthame]]

So the main issue here, I think, is that our font selection process doesn't recognize the presence of VS16 (U+FE0F) to request "emoji-style" presentation and use that to force a search for a font with color emoji glyphs, it just runs the standard font matching algorithm and takes the first match that supports the given codepoint.

If we did font matching on a per-cluster basis (as per bug 543200) instead of largely per-codepoint, that would tend to help, but we probably also need special handling for the emoji presentation sequences. The font-fallback code does some of this, but that doesn't help if the default font for the element already supports the base character in question (such as the old smiley-face symbol, or the digits).

cc'ing m_kato, who I'm guessing might be interested in this.

Comment 3

[Jonathon Kereliuk]

Any update on the progress of this? Also is this behavior spec-able at all?
On Edge as well the emojis are rendered but they are different looking emojis.

Comment 4

[Jonathon Kereliuk]

Hey Makato, is there an update on the progress of this?

Comment 5

[Nathan Froyd [:froydnj]]

Adding a ni? for good measure for comment 4.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Jonathon Kereliuk from comment #4)
> Hey Makato, is there an update on the progress of this?

I have WIP, but no time to review & land for 60 cycle.  So I will work this for 61 cycle.

Comment 7

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3afe70e2303d94b917429012782a376c3a65869

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Use emoji font for keycap

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8981084 [details] [diff] [review]
Use emoji font for keycap

number character (0-9) is possible emoji presentation.  Gecko doesn't select emoji font for these characters with variation selector-16 because it doesn't check whether selected font is color/emoji font.   Normally, number character is included in normal font, so it usually select first font even if next is VS16.

So if next character is VS16, we should try color font if there is in font list.

Android 4.3 doesn't have emoji font, so it should skip reftest.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8981084 [details] [diff] [review]
Use emoji font for keycap

Review of attachment 8981084 [details] [diff] [review]:
-----------------------------------------------------------------

Although the original example was about keycaps, this change could affect font selection for any character with a possible emoji presentation; as such, the commit message should reflect that rather than being so specific. Something more like "Make font selection try to find a color font for characters where emoji presentation is requested, even if this means ignoring fonts in the specified font-family list" would be clearer, I think.

Clearing review? for now, pending consideration of the question re gfxFontGroup::FindFontForChar behavior.

::: gfx/thebes/gfxFontEntry.h
@@ +485,5 @@
>      bool             mGrFaceInitialized : 1;
>      bool             mCheckedForColorGlyph : 1;
>      bool             mCheckedForVariationAxes : 1;
> +    bool             mCheckedForSomeColorTables : 1;
> +    bool             mHasSomeColorTables : 1;

I believe adding flags here will increase the size of gfxFontEntry by 8 bytes (assuming a 64-bit build), because of alignment (note the comment at the top of the group of bit flags); that seems a bit unfortunate, but if we need the flags I guess we'll have to accept it.

I wonder, though, if we can re-engineer something so as to eliminate or merge a couple of the already-existing flags, so as to stay within a total of 24 bits? AFAICT, the mFixedPitch flag is only relevant on macOS, so we could move it to the platform subclass; if we could save another flag as well, we'd avoid the size increase here.

::: gfx/thebes/gfxTextRun.cpp
@@ +2952,5 @@
> +        if (aNextCh == kVariationSelector16 && isPossibleEmojiPresentation) {
> +            gfxFontEntry *fe = ff.FontEntry();
> +            if (!fe->HasSomeColorTables(font)) {
> +                // No color font. Try next
> +                continue;

If I'm following the logic correctly, I think this will be a problem for the case where aCh has a possible emoji presentation, but is not actually supported by any available color font. In that case, we should still be using the first font from the list that supports aCh by itself; but this code will have caused us to ignore it, and we'll end up with whatever fallback happens to pick.

So for example on Android,

    <div style="font-family: foo, bar, serif">1&#xfe0f;</div>

will look for a color glyph for "1", but not finding one, it will have ignored the "foo" and "bar" fonts that were specified, and probably end up using Roboto.

Am I reasoning correctly here? If so, I think this is an issue we need to handle better: if we don't actually find a color glyph, we should end up with the same font as we'd have used in the absence of the VS16 selector.

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Comment on attachment 8981084 [details] [diff] [review]
> Use emoji font for keycap
> 
> Review of attachment 8981084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Although the original example was about keycaps, this change could affect
> font selection for any character with a possible emoji presentation; as
> such, the commit message should reflect that rather than being so specific.
> Something more like "Make font selection try to find a color font for
> characters where emoji presentation is requested, even if this means
> ignoring fonts in the specified font-family list" would be clearer, I think.
> 
> Clearing review? for now, pending consideration of the question re
> gfxFontGroup::FindFontForChar behavior.
> 
> ::: gfx/thebes/gfxFontEntry.h
> @@ +485,5 @@
> >      bool             mGrFaceInitialized : 1;
> >      bool             mCheckedForColorGlyph : 1;
> >      bool             mCheckedForVariationAxes : 1;
> > +    bool             mCheckedForSomeColorTables : 1;
> > +    bool             mHasSomeColorTables : 1;
> 
> I believe adding flags here will increase the size of gfxFontEntry by 8
> bytes (assuming a 64-bit build), because of alignment (note the comment at
> the top of the group of bit flags); that seems a bit unfortunate, but if we
> need the flags I guess we'll have to accept it.

Hmm, it might be unnecessary to use bool flag for checked or not.  Because I only check whether font has tables for color font.

> I wonder, though, if we can re-engineer something so as to eliminate or
> merge a couple of the already-existing flags, so as to stay within a total
> of 24 bits? AFAICT, the mFixedPitch flag is only relevant on macOS, so we
> could move it to the platform subclass; if we could save another flag as
> well, we'd avoid the size increase here.
> 
> ::: gfx/thebes/gfxTextRun.cpp
> @@ +2952,5 @@
> > +        if (aNextCh == kVariationSelector16 && isPossibleEmojiPresentation) {
> > +            gfxFontEntry *fe = ff.FontEntry();
> > +            if (!fe->HasSomeColorTables(font)) {
> > +                // No color font. Try next
> > +                continue;
> 
> If I'm following the logic correctly, I think this will be a problem for the
> case where aCh has a possible emoji presentation, but is not actually
> supported by any available color font. In that case, we should still be
> using the first font from the list that supports aCh by itself; but this
> code will have caused us to ignore it, and we'll end up with whatever
> fallback happens to pick.
> 
> So for example on Android,
> 
>     <div style="font-family: foo, bar, serif">1&#xfe0f;</div>
> 
> will look for a color glyph for "1", but not finding one, it will have
> ignored the "foo" and "bar" fonts that were specified, and probably end up
> using Roboto.
> 
> Am I reasoning correctly here? If so, I think this is an issue we need to
> handle better: if we don't actually find a color glyph, we should end up
> with the same font as we'd have used in the absence of the VS16 selector.

Android 4.3 case should select serif, but if using macOS, we should select Apple's color font that is in preference because that is emoji presentation.

After looking for preferences font, if there is no font for color, I should select first selected font that is monochrome.  I will try it.

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Bug 1371386 - Make font selection try to find a color font for characters where emoji presentation is requested

Comment 13

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 14

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: testcase for macOS with DejaVu Sans installed

Here's a testcase I've just been using to compare our behavior (without / with the patch here) to Safari's on macOS for various font-family settings. This testcase assumes the DejaVu Sans font (which has b/w versions of some of the U+1Fxxx characters whose default rendering should be emoji style) is installed.

I'll also attach screenshots showing the rendering I get.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: rendering of testcase in Safari

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: rendering of testcase in Nightly

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: rendering of testcase in Nightly with patch D27271

Comment 19

[Jonathan Kew [:jfkthame]]

Regarding the above testcase: I think Safari's default rendering (the initial column) is correct, with U+2601 defaulting to text-style and U+1F603 defaulting to emoji-style, and the variation selectors providing the ability to override this.

When DejaVu Sans is listed as the first font in font-family, I think Safari is wrong to ignore the U+FE0F variation selector and render the text-style glyphs in all cases; on the rows with explicit U+FE0F it should search for a color-emoji font, either later in the font-family list or via a fallback search.

Interestingly, when Apple Color Emoji is listed as the first font, Safari correctly (IMO) ignores it when U+FE0E is present, and falls back to a text-style rendering from a different font (either DejaVu Sans, if listed in font-family, or a default symbol font from the system if it supports the character).

The last screenshot shows that while some of the cases are improved compared to current Nightly, not all of them are handled correctly yet.

Comment 24

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: testcase on Windows

Changing the title as this also affects Windows.

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: Testcase for Windows using Segoe UI Symbol & Emoji fonts

This is a version of attachment 9073332 [details] that is better suited to testing on Windows, using font names available there.

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: Rendering of Windows testcase in current Nightly

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1371386 - Take account of requirements for emoji-style or text-style presentation during font selection & fallback. r=m_kato

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: Rendering of Windows testcase with patched build

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: Testcase rendering on macOS (with DejaVu Sans installed) using patched build

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cad1ef724ee
Take account of requirements for emoji-style or text-style presentation during font selection & fallback. r=m_kato

Comment 32

[Alexandru Michis [:malexandru]]

Backed out changeset 0cad1ef724ee (Bug 1371386) for causing failures in emoji-fallback-3.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/9a20d7d354d7bcdf66201735b3fa7d6d4e5b8b13

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&fromchange=0cad1ef724eea3d362a65dac7b4092a0c2d120e7&tochange=9a20d7d354d7bcdf66201735b3fa7d6d4e5b8b13&selectedTaskRun=bxy8e4k9TDq3ZebWu-nLUQ.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313858316&repo=autoland&lineNumber=8892

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8cda577ac2f
Take account of requirements for emoji-style or text-style presentation during font selection & fallback. r=m_kato

Comment 34

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f8cda577ac2f