1758420 - Composition is not updated correctly if some characters are inserted before it in same text node

Status: VERIFIED FIXED

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

Description

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

1. Go to https://codepen.io/trueadm/pen/KKyeMeg
2. Type something with IME at end of the editor
3. Keep typing without comitting composition

Then, you'll see incorrect range will be updated and some part of the composition will be duplicated.

Comment 1

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

Attachment: Bug 1758420 - part 1: Let `TextComposition` know where is changed in storing text node r=m_kato!

IMEContentObserver observes the text node change which contains the current
composition string. Therefore, it can let TextComposition know where is
updated by web apps and adjust offset and length in the text node.

Comment 2

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

Attachment: Bug 1758420 - part 2: Clean up update timing of `TextComposition` r=m_kato!

Currently, it's done at creating CompositionTransaction, not after executing
it. Let's clean it up now.

Depends on D141193

Comment 3

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

Attachment: Bug 1758420 - part 3: Rename `TextComposition::GetStartRef()` and `TextComposition::GetEndRef()` r=m_kato!

They return IME selection range rather than composition string range. I.e.,
when the composition does not have IME selections anymore, e.g., at committing,
they return non-set RawRangeBoundary.

For making the result meaning clearer, they should be
FirstIMESelectionStartRef and LastIMESelectionEndRef.

Depends on D141194

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5608c79beaad
part 1: Let `TextComposition` know where is changed in storing text node r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4c7745b1847a
part 2: Clean up update timing of `TextComposition` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fec88e22af1d
part 3: Rename `TextComposition::GetStartRef()` and `TextComposition::GetEndRef()` r=m_kato

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/5608c79beaad
https://hg.mozilla.org/mozilla-central/rev/4c7745b1847a
https://hg.mozilla.org/mozilla-central/rev/fec88e22af1d

Comment 6

[Makoto Kato [:m_kato]]

When reflowing text control during inputting composition in twitter.com/explore, although EditorBase::InitializeSelection will update mComposition using OnUpdateCompositionInEditor. But if it is called before EditorDidHandleCompositionChangeEvent, mComposition->String() isn't current composing text (Maybe Last() has valid? At least, composition transaction had valid data). Before applying this fix, it is updated when creating composition transaction. So it works expectedly...

Comment 7

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

Ah, thanks. Broken by the 2nd patch due to the timing issue makes sense. I'll take a look this tomorrow.

Comment 8

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

So, according to comment 6, Twitter must cause reframing from an input event listener because input event is fired before destroying TextComposition::CompositionChangeEventHandlingMarker here.
https://searchfox.org/mozilla-central/rev/2c4522f76b23b024d9f29e140d41042853874c07/editor/libeditor/EditorBase.cpp#3603-3605,3639

However, I cannot reproduce the symptom with simple raw DOM testcases. There must be another factor, e.g., requesting query content and/or setting selection from Android, or something different thing coming from gfx to handle RTL text. I give up to write automated test for the regression...

Comment 9

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

Makoto-san, I'm still creating an environment to build Fenix locally, but have not finished yet. So I'd be happy if you'd check the latest patches work or not in mobile.twitter.com?

Comment 10

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

Oops, the patches are here: https://treeherder.mozilla.org/jobs?repo=try&revision=43a8139da3ef87a4db7369d42b2eaab541a82b5b

Comment 11

[Makoto Kato [:m_kato]]

Makoto-san, I'm still creating an environment to build Fenix locally, but have not finished yet. So I'd be happy if you'd check the latest patches work or not in mobile.twitter.com?

It works fine.

Comment 12

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

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

Makoto-san, I'm still creating an environment to build Fenix locally, but have not finished yet. So I'd be happy if you'd check the latest patches work or not in mobile.twitter.com?

It works fine.

Thank you!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54881a6ac73d
part 1: Let `TextComposition` know where is changed in storing text node r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8c0fe0c6c772
part 2: Clean up update timing of `TextComposition` r=m_kato

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/54881a6ac73d
https://hg.mozilla.org/mozilla-central/rev/8c0fe0c6c772

Comment 15

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

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Comment 16

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

No, this fixes a traditional bug.

Comment 17

[Monica Chiorean]

I could not reproduce the issue using the steps from description on Win10x64 and builds 99.0a1(20220215210051) and 99.0a1(20220307093830).
Are there any other details that I am missing like a specific OS or build?
Can you please confirm issue in not reproducing on your side on latest Beta (https://archive.mozilla.org/pub/firefox/candidates/)

Comment 18

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

(In reply to Monica Chiorean from comment #17)

I could not reproduce the issue using the steps from description on Win10x64 and builds 99.0a1(20220215210051) and 99.0a1(20220307093830).
Are there any other details that I am missing like a specific OS or build?

Hmm, I checked it with 3rd party's Japanese IME, but it should be reproducible with any IME which can compose multiple characters once because it's a bug of XP level code in Gecko.

I think that you can reproduce it with using Japanese IME, and type "a" (automatically converted to "あ"), and type it twice and more without pressing other characters into the end of the box having blue borders in the testcase.

Can you please confirm issue in not reproducing on your side on latest Beta (https://archive.mozilla.org/pub/firefox/candidates/)

I don't have much time to do it tonight, but I cannot reproduce this in the Nightly.

Comment 19

[Monica Chiorean]

I was able to finally reproduce the issue on Win10x64 using build 99.0a1(7th of March) and after installing the Japanese IME.
Verified as fixed on Win10x64 using Beta 101.0b8.