1764238 - mobile.twitter.com - Typing the second letter duplicates the first letter in Hebrew language

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Karl Dubost💡 :karlcow]

[Tracking Requested - why for this release]:
Happening on twitter, so need to be careful.

1. Happens only on hebrew:
2. Open twitter on Firefox for Android
3. When trying to enter a search term

Expected:
Search term being displayed

Actual:
typing the second letter duplicates the first letter.

Makoto-san found a regression range
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=692d1b656c582dfacd718c88c49565cadbfdac34&tochange=fec88e22af1dc1a1dbed0781fdf15c7b305e88f8

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I cannot reproduce this on Windows, so it seems that it depends on IME composition and bidi text. I need a simple testcase for this...

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1758420

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sigh, there is no composition event listener breakpoints in the debugger... When I break it at input even at typing 2nd character, the duplication has already occurred. So compositionupdate event listener might do something tricky.

Comment 4

[Makoto Kato [:m_kato]]

This textbox uses <input> and React. So I guess that it isn't easy to minimize reproduce code. I guess that selection/caret position is changed by script, then this issue occurs. I will look this at this week.

Comment 5

[Beth Rennie [:beth] (she/her)]

Can we get an assignee to this bug to investigate? Is this only a twitter.com issue or is this affecting more websites using RTL / bidi text?

:masayuki, should we back this out on beta?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Barret Rennie [:barret] (they/them) from comment #5)

Can we get an assignee to this bug to investigate? Is this only a twitter.com issue or is this affecting more websites using RTL / bidi text?

I think that it's caused by the content update outside the <input> element so that it could occur with implementing similar UI. However, I'm still not sure because we've not yet reached to the conclusion.

:masayuki, should we back this out on beta?

If the fix for this is risky, yes, but the fix is requested by Lexical editor developer so that the fix is also important. If it'd be possible, I don't want to back them out.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, the replacing case looks buggy. However, this should not cause this bug because inserting text into into the anonymous text node should cause committing composition string first. I.e., this path shouldn't run.

So I've not reproduced similar problem with writing simple testcase, we can just stop doing the new thing when the text node is the anonymous text node of TextEditor.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, cannot build Fenix on tryserver? Makoto-san, could you check my partial backout patch?

Comment 9

[Makoto Kato [:m_kato]]

I forget to add a log with MOZ_LOG=GeckoEditableSupport:5

Nightly

04-13 16:43:24.270 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="פ"
04-13 16:43:24.270 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: Current selection: { mOffset=0, mData="" (Length()=0), Length()=0, EndOffset()=0 }
04-13 16:43:24.720 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_TEXT_CHANGE: TextChangeData={ mStartOffset=0, mRemoveEndOffset=0, mAddedEndOffset=1, mCausedOnlyByComposition=true, mIncludingChangesDuringComposition=false, mIncludingChangesWithoutComposition=false }
04-13 16:43:24.721 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_SELECTION_CHANGE: SelectionChangeData={ mOffset=1, mString="" (Length()=0), GetWritingMode()=h-rtl, mReversed=false, mCausedByComposition=true, mCausedBySelectionEvent=false, mOccurredDuringComposition=true }
04-13 16:43:24.721 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED
04-13 16:43:26.558 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: IME_SET_TEXT: text="פ", length=1, range=2
04-13 16:43:26.559 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED

04-13 16:43:26.561 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="פם"
04-13 16:43:26.670 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_TEXT_CHANGE: TextChangeData={ mStartOffset=0, mRemoveEndOffset=0, mAddedEndOffset=2, mCausedOnlyByComposition=true, mIncludingChangesDuringComposition=false, mIncludingChangesWithoutComposition=false }
04-13 16:43:26.671 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_SELECTION_CHANGE: SelectionChangeData={ mOffset=2, mString="" (Length()=0), GetWritingMode()=h-rtl, mReversed=false, mCausedByComposition=true, mCausedBySelectionEvent=false, mOccurredDuringComposition=true }
04-13 16:43:26.671 16533 16559 I Gecko   : [Child 16533: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED

Without bug 1758420

04-13 17:22:39.132 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="פ"
04-13 17:22:39.132 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: Current selection: { mOffset=0, mData="" (Length()=0), Length()=0, EndOffset()=0 }
04-13 17:22:39.624 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_TEXT_CHANGE: TextChangeData={ mStartOffset=0, mRemoveEndOffset=0, mAddedEndOffset=1, mCausedOnlyByComposition=true, mIncludingChangesDuringComposition=false, mIncludingChangesWithoutComposition=false }
04-13 17:22:39.625 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_SELECTION_CHANGE: SelectionChangeData={ mOffset=1, mString="" (Length()=0), GetWritingMode()=h-rtl, mReversed=false, mCausedByComposition=true, mCausedBySelectionEvent=false, mOccurredDuringComposition=true }
04-13 17:22:39.625 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED
04-13 17:22:41.217 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: IME_SET_TEXT: text="פ", length=1, range=2
04-13 17:22:41.218 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED

04-13 17:22:41.219 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="פם"
04-13 17:22:41.332 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_TEXT_CHANGE: TextChangeData={ mStartOffset=0, mRemoveEndOffset=1, mAddedEndOffset=2, mCausedOnlyByComposition=true, mIncludingChangesDuringComposition=false, mIncludingChangesWithoutComposition=false }
04-13 17:22:41.332 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_SELECTION_CHANGE: SelectionChangeData={ mOffset=2, mString="" (Length()=0), GetWritingMode()=h-rtl, mReversed=false, mCausedByComposition=true, mCausedBySelectionEvent=false, mOccurredDuringComposition=true }
04-13 17:22:41.332 18275 18305 I Gecko   : [Child 18275: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED

Although I don't debug it deeply yet, NOTIFY_IME_OF_TEXT_CHANGE might be incorrect mRemoveEndOffset...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you Makoto-san, unless this is caused by the version number 100, changing the value means that the new path runs in <input> too, but it's not assumed at fixing the bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, or "Without bug 1758420" means you back them out from mozilla-central? If so, it's obviously a bug of the patches.

Comment 12

[Makoto Kato [:m_kato]]

Ah, or "Without bug 1758420" means you back them out from mozilla-central? If so, it's obviously a bug of the patches.

Yes, when getting that log, I created backed out patch. Although I don't test #comment 8's binary yet, I will verify it today.

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #12)

Ah, or "Without bug 1758420" means you back them out from mozilla-central? If so, it's obviously a bug of the patches.

Yes, when getting that log, I created backed out patch. Although I don't test comment 8's binary yet, I will verify it today.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. Then, if my experimental patch fixes the issue, I also want the stacktrace printed by the NS_ASSERTION.

Comment 15

[Makoto Kato [:m_kato]]

Oh, cannot build Fenix on tryserver? Makoto-san, could you check my partial backout patch?

No hit assertion.

Maybe, twitter.com uses input.value = XXX by composition or input event? (This is only when inputting Hebrew on GV)

04-14 14:17:42.827 19109 19138 I Gecko   : [Child 19109: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="פ"
04-14 14:17:42.828 19109 19138 I Gecko   : [Child 19109: Main Thread]: D/GeckoEditableSupport IME: Current selection: { mOffset=0, mData="" (Length()=0), Length()=0, EndOffset()=0 }
04-14 14:17:42.842 19109 19138 I Gecko   : [Child 19109, Main Thread] WARNING: Failed to commit composition before setting value.  Investigate the cause!: '!EditorHasComposition()', file /builds/worker/checkouts/gecko/dom/html/TextControlState.cpp:2883
04-14 14:17:42.844 19109 19138 I Gecko   : [Child 19109, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:1601
04-14 14:17:42.845 19109 19138 I Gecko   : [Child 19109, Main Thread] WARNING: Failed to commit composition before setting value.  Investigate the cause!: '!EditorHasComposition()', file /builds/worker/checkouts/gecko/dom/html/TextControlState.cpp:2752

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, not hitting the assertion means that the edit occurs in contenteditable. So it seems that the first patch is completely broken and affects something handling in twitter.com. I'll post a patch to back the first patch out.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, but the second patch could also cause this bug. So, we can keep only the last patch which just renaming some methods and variables.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you try to check the trying patch correctly backs out the patches and fixed the twitter's issue? I'll request the review for it soon, and if you check the patch fixing this bug, give r+.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1764238 - Backout the first and the second patch of bug 1758420 r=m_kato!

Due to breaking mobile.twitter.com/explore for Hebrew IME users on Android,
this patch backs out the patches for bug 1753420 except the last patch which
just rename some methods and variables.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I can reproduce this bug with Hebrew, Arabic keyboard layout of Gboard and 3rd party's Hebrew keyboard too. So, something different thing runs with the characters of RTL language.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf7f47232a62
Backout the first and the second patch of bug 1758420 r=m_kato

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/cf7f47232a62

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9272245 [details]
Bug 1764238 - Backout the first and the second patch of bug 1758420 r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: New regression from 100, the search bar of mobile.twitter.com is broken with RTL language IMEs on Android.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Install Gboard into Android and add Hebrew keyboard layout

2. Go to https://mobile.twitter.com/explore with Fenix
3. Type 2 Hebrew characters

ER: The first typed character should not be duplicated.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Backing out patch excluding the renaming only patch, generated with hg backout without any manual code change.
• String changes made/needed: none

Comment 24

[Dianna Smith [:diannaS]]

Comment on attachment 9272245 [details]
Bug 1764238 - Backout the first and the second patch of bug 1758420 r=m_kato!

Approved for 100.0b8

Comment 25

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d55af0a66ac

Comment 26

[Adina Petridean QA [:apetridean]]

Verified as fixed on Nightly 109.0a1 from 11/24 with Xiaomi Pad5 (Android 12). Typing the second letter doesn't duplicate the first letter in Hebrew language.