1797796 - Typing in empty block element may cause inserting new text after the empty block on Android

Status: RESOLVED FIXED

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

Description

[Kevin Brosnan [Ex-Mozilla]]

From github: https://github.com/mozilla-mobile/fenix/issues/27569.

Steps to reproduce

1. Have the following HTML:

<div contenteditable="true">
  <div style="width:20px; height: 20px; border: solid"></div>
</div>

2. Click/tap on the box
3. Type a character

Expected behaviour

In Firefox desktop, Chrome desktop, and Chrome for Android, the typed character is inserted in the child div. This is the behaviour I would expect.

Actual behaviour

The typed character is inserted after the child div.

Device name

Fairphone 4

Android version

Android 11

Firefox release type

Firefox

Firefox version

106.1.0

Device logs

No logs are produced.

Additional information

I originally found this issue when testing a contenteditable that listens to beforeinput events to provide some custom behaviour. In other browsers (including Firefox for desktop) event.getTargetRanges() returns a range pointing at the child div, whereas Fenix returns a range that points at the parent div. I assume this inconsistency is related.

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 2

[Jonathan Almeida [:jonalmeida]]

Correcting the component from triage. I think this is a core bug.

I reproduced this locally as well in 111 nightly.

Comment 3

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

It's a long-standing bug (I'm not sure whether there is an older bug report). And web apps can avoid this with putting <br> into the empty <div> etc, and that is required for making it visible because truly empty blocks are 0-height.

Comment 4

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

Oh, but I cannot reproduce this bug in desktop, so this is different bug what I was thinking. I guess that IME on Android collapse Selection into the invisible text node and insert text there.

Comment 5

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

I guess that invisible text node should be ignored in ContentEventHandler, i.e., IME won't see them and be able to put caret into it. On the other hand, I'm not sure why IME wants to put caret before a line break which is caused by open tag of the empty <div> element. I need to get the log of composition first.

Comment 6

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

The log:

[Child 2304: Main Thread]: D/GeckoEditableSupport IME: IME_REPLACE_TEXT: text="M"
[Child 2304: Main Thread]: D/GeckoEditableSupport IME: Current selection: { mOffset=4, mData="" (Length()=0), Length()=0, EndOffset()=4 }
[Child 2304: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_TEXT_CHANGE: TextChangeData={ mStartOffset=4, mRemoveEndOffset=5, mAddedEndOffset=5, mCausedOnlyByComposition=true, mIncludingChangesDuringComposition=false, mIncludingChangesWithoutComposition=false }
[Child 2304: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_SELECTION_CHANGE: SelectionChangeData={ mOffset=5, mString="" (Length()=0), GetWritingMode()=h-ltr, mReversed=false, mCausedByComposition=true, mCausedBySelectionEvent=false, mOccurredDuringComposition=true }
[Child 2304: Main Thread]: D/GeckoEditableSupport IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED

Odd, it's expected result, but indeed the character is inserted after the <div>. (there are one line break and 2 white-spaces (i.e., 3 characters) before the <div>, and a line break should be inserted after it. Therefore, mOffset=4 is correct. So, this is a bug in XP level code.

Comment 7

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

At first beforeinput, getTargetRanges returns "adjusted" range.

beforeinput
target: <div contenteditable>, isComposing: true, inputType: "insertCompositionText", data: "m", dataTransfer: (null), getTargetRanges: [{ (<div id="editor">, 2) }]

However, Selection immediately before typing text is collapsed in the inner <div>...

Comment 8

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

Hmm, before EditorBase::OnCompositionStart, Selection was "adjusted" to outside of the empty <div>.

TYLin: Do you have any ideas where updates Selection only in Android in this case?

Comment 9

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

Ah, perhaps, I got it.

GeckoEditableSupport tries to set selection even if offset/length is not beeing changed. Then, if the selection is collapsed in an empty block, ContentEventHandler reaches here. Then, it's set to after the empty block. This matches with the symptom.

So I believe that ContentEventHandler should replace block element close tags with line breaks to make the eSetSelection event users possible to distinguish whether before or after a block element close tag.

However, this won't fix similar edge cases aound inline element boundaries, and it requires big changes. Therefore, I think that we should just make GeckoEditableSupport stop setting Selection if the range is same as current one.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

TYLin: Do you have any ideas where updates Selection only in Android in this case?

I'm not familiar with the relevant code you linked in comment 9, so I'm glad you figure it out :)

Comment 11

[Graham Rogers [:TastyPi]]

Thanks for looking into this, looking forward to seeing it fixed!

In the meantime (and for supporting older Firefox versions) is there a bit of JavaScript to detect whether this bug is in play? I have a JS-disabled fallback I'd like to use if this bug is in play, but I don't know if there is an easy way to detect it. I guess using the user agent would work, but maybe during your investigations you've found something else?

Comment 12

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

Attachment: Bug 1797796 - Make `eSetSelection` event not handled if `Selection` is set to same offset and length r=m_kato!

ContentEventHandler may not replace open/close tags of inline elements and
close tags of block elements with a line break etc. Therefore, one offset may
mean multiple points in the DOM tree. From the DOM tree point of view, every
DOM point may have different meaning and styles. Therefore, if IME sets
Selection to current range, we should not update Selection with the log of
ContentEventHandler. Otherwise, new content will appear at different position
in the DOM tree and it may cause different style text, etc.

IMEContentObserver has cache of Selection while an editor has focus.
Therefore, for avoiding the query cost of Selection for each eSetSelection
event, ContentEventHandler should be used in IMEContentObserver instead if
an instance is available. Therefore, this patch makes TextComposition
delegate the handling to IMEContentObserver and check Selection range only
when IMEContentObserver is available. Note that eSetSelection is probably
used only when editable element has focus. Therefore, it must be fine to skip
selection range check in the other cases.

Comment 13

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

(In reply to graham from comment #11)

In the meantime (and for supporting older Firefox versions) is there a bit of JavaScript to detect whether this bug is in play?

Unfortunately, this is a simple bug of IME handling in the case of that IME resets caret position by itself. Therefore, without UA checks, I guess that this could be detectable with checking Selection in compositionstart event listener or at first compositionupdate event. However, I'm not sure whether it works as you expected if you change Selection at these timings.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e65d5413de44
Make `eSetSelection` event not handled if `Selection` is set to same offset and length r=m_kato

Comment 15

[Cristian Tuns]

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