Bug 141047 – Additional emoji support

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Enrica Casucci]

Adding support for emoji variations and emoji groups.

rdar://problem/19045135

[Enrica Casucci]

Created attachment 245665 [details]
Patch

[WebKit Commit Bot]

Attachment 245665 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCascade.cpp:712:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

Created attachment 245678 [details]
Patch2

After discussing the patch in person with Dan Bernstein, I made some changes.

[Build Bot]

Comment on attachment 245678 [details]
Patch2

Attachment 245678 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5327720862973952

New failing tests:
fast/text/format-control.html

[Build Bot]

Created attachment 245689 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245678 [details]
Patch2

Attachment 245678 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6411648201916416

New failing tests:
fast/text/format-control.html

[Build Bot]

Created attachment 245690 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

The fast/text/format-control.html failure shows an existing bug being uncovered by the patch: the complex text path lets ligatures form around a zero-width joiner, whereas the fast path doesn’t allow it. Testing in TextEdit suggests that the correct behavior is to not form a ligature, so it’s a bug in the complex path.

[Enrica Casucci]

Created attachment 245914 [details]
Patch3

Updated patch after discussing with Dan.
It also fixes line breaking and cursor movement.

[WebKit Commit Bot]

Attachment 245914 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextBreakIterator.cpp:210:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/text/TextBreakIterator.cpp:232:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/text/TextBreakIterator.cpp:251:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 245914 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=245914&action=review

I think this code is probably correct, but there are no comments explaining the behavior, so I think we should do better.

> Source/WebCore/platform/graphics/FontCascade.cpp:643
> +    bool emojiGroupCandidate = false;

Normally we would not name a boolean variable "candidate" since the boolean is not itself a candidate. Maybe sawEmojiGroupCandidate is the right name for this? I’m nots sure I understand the meaning of the boolean (see below).

> Source/WebCore/platform/graphics/FontCascade.cpp:713
> +        if (c == 0x200D && emojiGroupCandidate)
> +            return Complex;

I don’t understand the logic here. Are we saying that a 0x200D immediately following a 0x1F466-0x1F469 makes the string need the Complex code path, or that a 0x200D anywhere after a 0x1F466-0x1F469 makes the string complex? The code implements the latter.

> Source/WebCore/rendering/RenderText.cpp:1441
> +inline bool isEmojiGroupCandidate(UChar32 c)

These functions should be "static inline" rather than just "inline" since we want internal linkage within this file, not external linkage. These should use the word "character" instead of the letter "c".

> Source/WebCore/rendering/RenderText.cpp:1459
>      bool sawRegionalIndicator = false;

It’s strange that sawRegionalIndicator gets set and then is left set to true no matter what intervening characters we see.

> Source/WebCore/rendering/RenderText.cpp:1460
> +    bool sawEmojiGroupCandidate = false;

I think this should be named previousCharacterWasEmojiGroupCandidate. Although I’m surprised that these code points are called “emoji group candidates”. Is that really the correct terminology?

> Source/WebCore/rendering/RenderText.cpp:1477
> +        if (sawEmojiGroupCandidate) {
> +            sawEmojiGroupCandidate = false;
> +            if (character == 0x200D)
> +                continue;
> +            U16_FWD_1_UNSAFE(text, current);
> +            break;
> +        }

I don’t understand why this is the correct thing to do. Maybe a comment is needed.

Could we please use the name zeroWidthJoiner instead of the constant 0x200D?

> Source/WebCore/rendering/RenderText.cpp:1480
> +        if (sawEmojiModifier)
> +            break;

I don’t understand why this is the correct thing to do. Maybe a comment is needed.

> LayoutTests/ChangeLog:9
> +        * editing/deleting/delete-emoji.html: Added.

Why do we need separate results for each platform? The files all look the same to me? Can’t we just put a result in this directory instead of in all four of those other directories?

> LayoutTests/editing/deleting/delete-emoji.html:8
> +Markup.description("This test verifies that emoji groups and emoji with variations are delete correctly");

"are deleted" rather than "are delete"

I’d like to see test cases covering incorrect sequences, not just correct ones.

[Enrica Casucci]

> Normally we would not name a boolean variable "candidate" since the boolean
> is not itself a candidate. Maybe sawEmojiGroupCandidate is the right name
> for this? I’m nots sure I understand the meaning of the boolean (see below).

I'll change the variable name,
> 
> > Source/WebCore/platform/graphics/FontCascade.cpp:713
> > +        if (c == 0x200D && emojiGroupCandidate)
> > +            return Complex;
> 
> I don’t understand the logic here. Are we saying that a 0x200D immediately
> following a 0x1F466-0x1F469 makes the string need the Complex code path, or
> that a 0x200D anywhere after a 0x1F466-0x1F469 makes the string complex? The
> code implements the latter.
It is immediately following. I can make this strictly accept only the immediately following.
> 
> > Source/WebCore/rendering/RenderText.cpp:1460
> > +    bool sawEmojiGroupCandidate = false;
> 
> I think this should be named previousCharacterWasEmojiGroupCandidate.
> Although I’m surprised that these code points are called “emoji group
> candidates”. Is that really the correct terminology?
> 
As far as I know yes, it is the correct terminology.
> 
> Could we please use the name zeroWidthJoiner instead of the constant 0x200D?
> 
> > Source/WebCore/rendering/RenderText.cpp:1480
> > +        if (sawEmojiModifier)
> > +            break;
> 
> I don’t understand why this is the correct thing to do. Maybe a comment is
> needed.
> 
> > LayoutTests/ChangeLog:9
> > +        * editing/deleting/delete-emoji.html: Added.
> 
> Why do we need separate results for each platform? The files all look the
> same to me? Can’t we just put a result in this directory instead of in all
> four of those other directories?
> 
> > LayoutTests/editing/deleting/delete-emoji.html:8
> > +Markup.description("This test verifies that emoji groups and emoji with variations are delete correctly");
> 
> "are deleted" rather than "are delete"
> 
> I’d like to see test cases covering incorrect sequences, not just correct
> ones.
will do.

[Enrica Casucci]

Committed revision 179567.