Closed Bug 1032671 Opened 5 years ago Closed 2 years ago

Add font.name-list.* for Emoji

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 5 obsolete files)

58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
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).
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.
Assignee: nobody → m_kato
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.
Depends on: 1201533
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.
Attachment #8666566 - Flags: review?(jfkthame)
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
Attachment #8666567 - Flags: review?(jfkthame)
Comment on attachment 8666568 [details] [diff] [review]
Part 3. set font.name.emoji for each platform

set default preference for emoji
Attachment #8666568 - Flags: review?(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.
Attachment #8666566 - Flags: review?(jfkthame) → review+
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.
Attachment #8666567 - Flags: review?(jfkthame) → review+
Attachment #8666568 - Flags: review?(jfkthame) → review+
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.
Attachment #8666567 - Attachment is obsolete: true
Attachment #8667791 - Flags: review?(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/
Attachment #8667791 - Flags: review?(jfkthame) → review?(jdaggett)
Duplicate of this bug: 1066933
When I discuss with John-san, he suggest font.emoji instead of font.(name|name-list).emoji.
Attachment #8667791 - Flags: review?(jdaggett)
Blocks: 1239380
Blocks: 1295029
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.
Attachment #8819184 - Flags: review?(jfkthame) → review+
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)
Attachment #8819185 - Flags: review?(jfkthame) → review+
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.
Attachment #8819186 - Flags: review?(jfkthame) → review+
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.
Attachment #8819187 - Flags: review?(jfkthame) → review+
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?)
Attachment #8819188 - Flags: review?(jfkthame) → review+
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.
: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?
Flags: needinfo?(m_kato)
Blocks: 1321685
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.
Flags: needinfo?(m_kato)
Attachment #8819189 - Attachment is obsolete: true
Attachment #8819189 - Flags: review?(jfkthame)
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.
Attachment #8819186 - Flags: review+ → review?(jfkthame)
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
Attachment #8819188 - Flags: review+ → review?(jfkthame)
Attachment #8666566 - Attachment is obsolete: true
Attachment #8666568 - Attachment is obsolete: true
Attachment #8667791 - Attachment is obsolete: true
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.
Attachment #8819186 - Flags: review?(jfkthame) → review+
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
Attachment #8819188 - Flags: review?(jfkthame) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
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
Duplicate of this bug: 1295029
Duplicate of this bug: 1321586
You need to log in before you can comment on or make changes to this bug.