1484094 - caret should not move into ligated Emoji + ZWJ sequence

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

IsAcceptableCaretPosition in nsTextFrame.cpp doesn't consider ZWJ, so caret cannot move into emoji with ZWJ correctly.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Caret doesn't move into Emoji that has ZWJ

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 9001846 [details] [diff] [review]
Caret doesn't move into Emoji that has ZWJ

When using emoji character that has ZWJ, caret cannot move per character.  We should consider ZWJ to check acceptable caret position.

Comment 3

[Jonathan Kew [:jfkthame]]

For emoji-zwj sequences, which are conceptually single units that just happen to be encoded as sequences rather than single codepoints, I agree it makes sense to do this. But I'm less sure whether it is the best behavior for cases where ZWJ is used in other contexts, such as to control shaping in Indic scripts.

In an example like

   data:text/html,<h1 contenteditable>&%23x930;&%23x94d;&%23x200d;&%23x92f;

the ZWJ is used to obtain the eyelash-RA shape, instead of the default rendering with REPH:

    data:text/html,<h1 contenteditable>&%23x930;&%23x94d;&%23x92f;

Here, I think the fact that the caret can be placed between the eyelash-RA and the following YA is correct, and it would be sad to lose this.

Also, this patch would prevent the caret appearing within an emoji-zwj sequence even if the font being used does not support the sequence, so that it is actually rendered as a series of separate glyphs. It's unclear to me whether that would be the best result in such a case; if the sequence is displayed as multiple emoji glyphs, it seems like the user would expect to be able to place the caret within it.

So I suggest a slightly different approach here. We already check for Regional-Indicator pairs (rendered as flags), and avoid allowing the caret within them (but only if they're actually ligated by the font being used). Perhaps we should do the same thing more generally for any ligatures where the component characters have the emoji-presentation property.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Don't allow the caret to be placed within a ligated emoji sequence

Here's the alternative approach I'd suggest, which should give the desired behavior for emoji sequences while not affecting other text, or non-ligated sequences. Does this seem OK to you?

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 9001903 [details] [diff] [review]
Don't allow the caret to be placed within a ligated emoji sequence

WDYT about this option? (If we go this way, we'll still want to add tests as in your original patch; this is just the actual code change for your consideration.)

Comment 6

[Jonathan Kew [:jfkthame]]

I pushed a try run with the patch in attachment 9001903 [details] [diff] [review] together with your tests from attachment 9001846 [details] [diff] [review], to check that they pass as expected across all our platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9e8615ce0201b0511bca77b7ea2d6d071bb2fe0

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 9001903 [details] [diff] [review]
Don't allow the caret to be placed within a ligated emoji sequence

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

Thanks.  Caret doesn't still move on some emoji sequence (ex. U+1F575 U+1F3FB U+200D U+2640 U+FE0F) correctly.  But it is OK at this time since I should fix to render as emoji (such as bug 1371386).

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 9001846 [details] [diff] [review]
Caret doesn't move into Emoji that has ZWJ

r=me for the added tests here, which I'll push together with attachment 9001903 [details] [diff] [review]. (The code change in nsTextFrame.cpp is superseded by the alternate patch, and no longer applies.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdd7d0adcc0
Don't allow the caret to be placed within a ligated emoji sequence. r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/18195ca7647c
Test that caret doesn't move into ligated Emoji-ZWJ sequence. r=jfkthame

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/dcdd7d0adcc0
https://hg.mozilla.org/mozilla-central/rev/18195ca7647c

Comment 11

[thomas.a.levy]

This issue still occurs for 💂‍♀️. I can place my cursor in the middle of the female guard emoji.

data:text/html;charset=utf-8,<h1 contenteditable>%F0%9F%92%82%E2%80%8D%E2%99%80%EF%B8%8F

Comment 12

[Makoto Kato [:m_kato]]

(In reply to thomas.a.levy from comment #11)

This issue still occurs for 💂‍♀️. I can place my cursor in the middle of the female guard emoji.

data:text/html;charset=utf-8,<h1 contenteditable>%F0%9F%92%82%E2%80%8D%E2%99%80%EF%B8%8F

Could you file a new bug with your Firefox version?

Comment 13

[thomas.a.levy]

Filed #1518401