1032671 - Add font.name-list.* for Emoji

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Makoto Kato [:m_kato]]

Many Emoji characters are into non-BMP area.  But user cannot set specific font for it by preference.  font.name-list.*.x-unicode may work, but this is applied for all other area of greater than U+FFFF.

Maybe, we need add prefs (font.name-list.*.nonbmp?) for emoji only if required.  Android 4.4 by NTT Docomo has some emoji fonts that are Noto Color Emoji and thier designed font　（DcmColorUniEmoji).

Comment 1

[Jonathan Kew [:jfkthame]]

I don't like the *.nonbmp name, as there are lots of non-bmp characters that are not emoji: math, lots of ancient scripts, CJK extensions, etc.

Maybe just font.name-list.emoji? The serif/sans-serif/monospace settings don't really seem relevant here.

Comment 2

[Makoto Kato [:m_kato]]

Emoji will be defined by UTR#51.
http://www.unicode.org/reports/tr51/tr51-2.html#Presentation_Style

Comment 3

[Makoto Kato [:m_kato]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

Note that I've just filed bug 1201533 (and bug 1201518) to remove the East Asian Width data from nsCharProps2, as it's not actually used for anything. If we do that, there'll be a 3-bit field available there, which should be enough space for the UTR51 data you want to add.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 1. Detect emoji range using UTR51

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Part 2. Implement font.name.emoji for emoji font prefs

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Part 3. set font.name.emoji for each platform

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8666566 [details] [diff] [review]
Part 1. Detect emoji range using UTR51

Detect emoji presentation range using UTR#51 data file.

But this implementation has limitation for country flags and keymap.  I cannot find good way to detect both.  But I think that all country flags range should be default emoji presentation and keymap is text presentation.

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8666567 [details] [diff] [review]
Part 2. Implement font.name.emoji for emoji font prefs

implement font.name.emoji and font.name-list.emoji

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8666568 [details] [diff] [review]
Part 3. set font.name.emoji for each platform

set default preference for emoji

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8666566 [details] [diff] [review]
Part 1. Detect emoji range using UTR51

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

::: intl/unicharutil/tools/genUnicodePropertyData.pl
@@ +672,5 @@
> +    } elsif (m/([0-9A-F]{4,6})\s+([0-9A-F]{4,6})\s*;\s*([^ ]+)/) {
> +        # if not keycaps and country flags, output warning
> +        my $first = hex "0x$1";
> +        my $second = hex "0x$2";
> +        warn "unknow Emoji area $1 $2" unless ($second == 0x20e3 ||

s/unknow/unknown/

@@ +783,5 @@
>  }
>  $type = q/
>  struct nsCharProps2 {
>    unsigned char mScriptCode:8;
> +  unsigned char mEmojiPresentation:2;

nit: please add a comment noting that there is currently one unused bit in this struct.

Or perhaps better, add an unnamed 1-bit field to make the padding explicit:

  unsigned char : 1; // currently 1 unused bit

Put it right before or after mEmojiPresentation, to keep mCategory in the low 5 bits of a byte (probably -- the packing is implementation-dependent, I believe, but we can at least try to arrange things somewhat optimally); that way we (probably) won't need a shift operation when reading that field.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8666567 [details] [diff] [review]
Part 2. Implement font.name.emoji for emoji font prefs

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

::: gfx/thebes/gfxTextRun.cpp
@@ +3154,5 @@
> +
> +    EmojiPresentation emoji = GetEmojiPresentation(aCh);
> +    if ((emoji != EmojiPresentation::TextOnly &&
> +         (aNextCh == 0xfe0f ||
> +          (emoji == EmojiPresentation::EmojiDefault && aNextCh != 0xfe0e)))) {

nit: please do something like

  const uint32_t kVariationSelector15 = 0xFE0E; // text presentation
  const uint32_t kVariationSelector16 = 0xFE0F; // emoji presentation

and then use the named constants here, for clarity.

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Part 2. Implement font.name.emoji for emoji font prefs v2

After landed bug 1182361, previous fix (part 2) is conflict.

font.name.emoji has no lang attribute, I add add new member to gfxPlatformFontList for cache.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8667791 [details] [diff] [review]
Part 2. Implement font.name.emoji for emoji font prefs v2

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

Bouncing this review to John, as he's been working on this stuff lately so he's better able to check whether this is the right way to handle it.

A couple of small nits in the meantime:

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +871,5 @@
>  
>      nsIAtom* langGroup = GetLangGroupForPrefLang(aPrefLang);
>      NS_ASSERTION(langGroup, "null lang group for pref lang");
>  
> +    gfxPlatformFontList::GetFontFamiiesFromGenericFamilies(genericFamilies,

No need for the explicit "gfxPlatformFontList::" here, surely?

@@ +888,5 @@
> +    gfxFontUtils::AppendPrefsFontList(prefFontName.get(), genericFamilies);
> +    nsAutoCString prefFontListName("font.name-list.emoji");
> +    gfxFontUtils::AppendPrefsFontList(prefFontListName.get(), genericFamilies);
> +
> +    gfxPlatformFontList::GetFontFamiiesFromGenericFamilies(genericFamilies,

Or here.

::: gfx/thebes/gfxPlatformFontList.h
@@ +335,5 @@
> +    void
> +    ResolveEmojiFontNames(nsTArray<nsRefPtr<gfxFontFamily>>* aGenericFamilies);
> +
> +    void
> +    GetFontFamiiesFromGenericFamilies(

s/Famiies/Families/

Comment 16

[Makoto Kato [:m_kato]]

When I discuss with John-san, he suggest font.emoji instead of font.(name|name-list).emoji.

Comment 17

[Makoto Kato [:m_kato]]

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

Comment 18

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 1. Add emoji presentation API for UTR#51.

From ICU 57, ICU supports emoji presentation as draft API.  If ICU isn't supported, use our unicode table by ICD.

Review commit: https://reviewboard.mozilla.org/r/99042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99042/

Comment 19

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 2. Use font.name-list.emoji preference for emoji presenration.

Review commit: https://reviewboard.mozilla.org/r/99044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99044/

Comment 20

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 5. Add reftest for emoji.

When I discuss with John-san before he leaves Mozilla, he suggests that preference name as font.emoji instead of font.name.emoji and font.name-list.emoji.

Jonathan, Should I keep font.name.emoji like previous fix?

Review commit: https://reviewboard.mozilla.org/r/99046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99046/

Comment 21

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 3. Set font.name-list.emoji for some platforms.

- Segoe UI Emoji for Windows 8+
- EmojiOne Mozilla for Windows 7
- Apple Color Emoji for OSX
- EmojiOne Mozilla for GTK(Linux).  fontconfig doesn't support emoji as family name.
- Noto Color Emoji for Android

Review commit: https://reviewboard.mozilla.org/r/99048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99048/

Comment 22

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 4. Font fallback should detect emoji range for color emoji font.

Review commit: https://reviewboard.mozilla.org/r/99050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99050/

Comment 23

[Makoto Kato [:m_kato]]

Attachment: Bug 1032671 - Part 6. Add reftest for emoji.

I set 'Arial' as dummy to resolve line-height on OSX refrest.

Review commit: https://reviewboard.mozilla.org/r/99052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99052/

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819184 [details]
Bug 1032671 - Part 1. Add emoji presentation API for UTR#51.

https://reviewboard.mozilla.org/r/99042/#review99810

::: intl/unicharutil/tools/genUnicodePropertyData.pl:861
(Diff revision 1)
>             "CharProp2", $type, "nsCharProps2", 9, 7, \&sprintCharProps2_short, 16, 1, 1);
>  
>  sub sprintCharProps2_full
>  {
>    my $usv = shift;
> -  return sprintf("{%d,%d,%d,%d,%d,%d,%d,%d,%d,%d},",
> +  return sprintf("{%d,%d,%d,%d,%d,%d,%d,%d,%d,%d, %d},",

Insignificant nit: the rest of this record is output without spaces in the generated data file; would be nice to be consistent throughout.

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819185 [details]
Bug 1032671 - Part 2. Use font.name-list.emoji preference for emoji presenration.

https://reviewboard.mozilla.org/r/99044/#review99812

(Regenerate after fixing the inconsistency in printing the charProps2 record)

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819186 [details]
Bug 1032671 - Part 5. Add reftest for emoji.

https://reviewboard.mozilla.org/r/99046/#review99814

Looks good, though I'd prefer us to go with the more explicit pref name (see below).

::: gfx/thebes/gfxPlatformFontList.h:375
(Diff revision 1)
>  
> +    void
> +    ResolveEmojiFontNames(nsTArray<RefPtr<gfxFontFamily>>* aGenericFamilies);
> +
> +    void
> +    GetFontFamiiesFromGenericFamilies(

Typo (missing "l") in the method name, please fix here and throughout the patch.

::: gfx/thebes/gfxPlatformFontList.cpp:872
(Diff revision 1)
> +    nsTArray<RefPtr<gfxFontFamily>>* aGenericFamilies)
> +{
> +    // emoji preference has no lang name
> +    AutoTArray<nsString,4> genericFamilies;
> +
> +    nsAutoCString prefFontListName("font.emoji");

Contrary to what John said, I think this should be "font.name-list.emoji", for better consistency with the rest of the font prefs. It will accept a list of font names, just like other font.name-list.* prefs.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819187 [details]
Bug 1032671 - Part 3. Set font.name-list.emoji for some platforms.

https://reviewboard.mozilla.org/r/99048/#review99818

LGTM; we'll need to keep the pref name in sync with update to the previous patch, of course.

Comment 28

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819188 [details]
Bug 1032671 - Part 4. Font fallback should detect emoji range for color emoji font.

https://reviewboard.mozilla.org/r/99050/#review99822

::: gfx/thebes/gfxAndroidPlatform.cpp:176
(Diff revision 1)
>      static const char kNotoColorEmoji[] = "Noto Color Emoji";
>  
> -    if (aNextCh == 0xfe0fu) {
> +    EmojiPresentation emoji = GetEmojiPresentation(aCh);
> +    if (emoji != EmojiPresentation::TextOnly) {
> +        if (aNextCh == 0xfe0fu ||
> +           (aNextCh != 0xfe0eu && emoji == EmojiPresentation::EmojiDefault)) {

Although I realize the old code used hex constants here (which is probably my fault), I think it'd be better to use `const uint32_t kVariationSelector16 = ...` and so on, here and in the similar code on other platforms. Or maybe there's a common header we could usefully put these into, so we don't have to repeat the numbers in so many places? (Maybe define them in nsUnicodeProperties.h?)

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819189 [details]
Bug 1032671 - Part 6. Add reftest for emoji.

https://reviewboard.mozilla.org/r/99052/#review99824

Could we add tests that involve the VS15/VS16 controls as well? This doesn't seem to cover all aspects of the patch yet.

Comment 30

[Jonathan Kew [:jfkthame]]

:m_kato, it would be great if we could get this finished/landed. I'm not sure how much the patches may have bit-rotted, but aside from that it was just waiting for some additional tests. Any chance you'll be able to get back to it sometime soon?

Comment 31

[Makoto Kato [:m_kato]]

Yes, since we require ICU, so I don't need a parser for new UTR51 table.  So I will rebase patch and send a review again for some part.

Comment 32

[Makoto Kato [:m_kato]]

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

Comment 33

[Makoto Kato [:m_kato]]

Comment on attachment 8819184 [details]
Bug 1032671 - Part 1. Add emoji presentation API for UTR#51.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99042/diff/1-2/

Comment 34

[Makoto Kato [:m_kato]]

Comment on attachment 8819185 [details]
Bug 1032671 - Part 2. Use font.name-list.emoji preference for emoji presenration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99044/diff/1-2/

Comment 35

[Makoto Kato [:m_kato]]

Comment on attachment 8819187 [details]
Bug 1032671 - Part 3. Set font.name-list.emoji for some platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99048/diff/1-2/

Comment 36

[Makoto Kato [:m_kato]]

Comment on attachment 8819188 [details]
Bug 1032671 - Part 4. Font fallback should detect emoji range for color emoji font.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99050/diff/1-2/

Comment 37

[Makoto Kato [:m_kato]]

Comment on attachment 8819186 [details]
Bug 1032671 - Part 5. Add reftest for emoji.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99046/diff/1-2/

Comment 38

[Makoto Kato [:m_kato]]

Comment on attachment 8819186 [details]
Bug 1032671 - Part 5. Add reftest for emoji.

add variation selector test.

Windows 7 and Linux don't have system emoji font, so unicode support is different for Emoji One vs OS's font.   So emoji-fallback-2 isn't successful.

Comment 39

[Makoto Kato [:m_kato]]

Comment on attachment 8819188 [details]
Bug 1032671 - Part 4. Font fallback should detect emoji range for color emoji font.

move kVariationSelector15/16 to UnicodeProperties.h

Comment 40

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819186 [details]
Bug 1032671 - Part 5. Add reftest for emoji.

https://reviewboard.mozilla.org/r/99046/#review210376

LGTM. We may need to update these in future, as they're dependent on the emoji fonts shipped by various platforms, but in this case I think we just have to deal with that if/when it arises.

Comment 41

[Jonathan Kew [:jfkthame]]

Comment on attachment 8819188 [details]
Bug 1032671 - Part 4. Font fallback should detect emoji range for color emoji font.

https://reviewboard.mozilla.org/r/99050/#review210378

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5b43d990d70
Part 1. Add emoji presentation API for UTR#51. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/69219385100a
Part 2. Use font.name-list.emoji preference for emoji presenration. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/da7ddc04b431
Part 3. Set font.name-list.emoji for some platforms. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/9e104764efa1
Part 4. Font fallback should detect emoji range for color emoji font. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f16b41dbddee
Part 5. Add reftest for emoji. r=jfkthame

Comment 43

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e5b43d990d70
https://hg.mozilla.org/mozilla-central/rev/69219385100a
https://hg.mozilla.org/mozilla-central/rev/da7ddc04b431
https://hg.mozilla.org/mozilla-central/rev/9e104764efa1
https://hg.mozilla.org/mozilla-central/rev/f16b41dbddee