Bug 141661 - Emoji sequences do not render properly.
Summary: Emoji sequences do not render properly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified OS X 10.10
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-16 13:00 PST by Enrica Casucci
Modified: 2015-02-16 17:05 PST (History)
9 users (show)

See Also:


Attachments
Patch (10.94 KB, patch)
2015-02-16 13:04 PST, Enrica Casucci
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (602.56 KB, application/zip)
2015-02-16 13:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (757.49 KB, application/zip)
2015-02-16 14:04 PST, Build Bot
no flags Details
Patch2 (16.63 KB, patch)
2015-02-16 15:56 PST, Enrica Casucci
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2015-02-16 13:00:48 PST
Some of the emoji groups and emoji with variations do not render correctly on OS X.

rdar://problem/19820463
Comment 1 Enrica Casucci 2015-02-16 13:04:53 PST
Created attachment 246670 [details]
Patch
Comment 2 mitz 2015-02-16 13:13:49 PST
Comment on attachment 246670 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        * dom/Text.h: Moved from RenderText.cpp to make the functions
> +        available to other parts of the code.

Code in the platform directory depend on anything outside the platform directory, so clearly this not the right place for these definitions. They also have nothing to do with the DOM text node. Perhaps you meant for this to move somewhere in platform/text?
Comment 3 mitz 2015-02-16 13:14:35 PST
(In reply to comment #2)
> Code in the platform directory depend on anything […]

I meant “should not depend”.
Comment 4 Enrica Casucci 2015-02-16 13:53:10 PST
Comment on attachment 246670 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        available to other parts of the code.
> 
> Code in the platform directory depend on anything outside the platform directory, so clearly this not the right place for these definitions. They also have nothing to do with the DOM text node. Perhaps you meant for this to move somewhere in platform/text?

You're right, I'll find another place for it.
Comment 5 Build Bot 2015-02-16 13:58:45 PST
Comment on attachment 246670 [details]
Patch

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

New failing tests:
fast/text/emoji.html
Comment 6 Build Bot 2015-02-16 13:58:47 PST
Created attachment 246678 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-02-16 14:04:41 PST
Comment on attachment 246670 [details]
Patch

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

New failing tests:
fast/text/emoji.html
Comment 8 Build Bot 2015-02-16 14:04:46 PST
Created attachment 246680 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Sam Weinig 2015-02-16 14:44:10 PST
Comment on attachment 246670 [details]
Patch

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

> Source/WebCore/dom/Text.h:89
> +inline bool isEmojiGroupCandidate(UChar32 character)
> +{
> +    return (character >= 0x1F466 && character <= 0x1F469) || character == 0x2764 || character == 0x1F48B;
> +}
> +
> +inline bool isEmojiModifier(UChar32 character)
> +{
> +    return character >= 0x1F3FB && character <= 0x1F3FF;
> +}
> +
> +inline bool isVariationSelector(UChar32 character)
> +{
> +    // U+FE00 through U+FE0F Unicode variation selectors
> +    return character >= 0xFE00 && character <= 0xFE0F;
> +}

This is the wrong place for these. These should be somewhere in the platform directory or in WTF since they are used by things like FontCascade.  Maybe in TextBoundaries.h? Maybe wtf/unicode/CharacterNames.h? Maybe a new file.

> Source/WebCore/platform/graphics/FontCascade.cpp:31
> +#include "Text.h"

This is a layering violation.
Comment 10 Enrica Casucci 2015-02-16 15:56:43 PST
Created attachment 246693 [details]
Patch2

Addresses comments.
Comment 11 Enrica Casucci 2015-02-16 17:05:47 PST
Committed revision 180191.