1868641 - Ctrl+A selecting text in Slack's "New message" "To:" field and typing appends next characters instead of replacing the selected text

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: New_message_button.png

Masayuki, I bisected this regression to this pushlog for your fix for bug 1851951:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=97dc7be3b93c4c9e3ed360b635b42b292a275f53&tochange=81c485d2c1573d1ecf79565b72cfe05e134b0dfe

Bug 1851951 landed in Fx 121. I can reproduce this bug in Nightly 121 and 122, but not Beta 121 because the change is behind a pref: editor.block_inline_check.use_computed_style that is only enabled for IS_EARLY_BETA_OR_EARLIER.

Steps to reproduce

1. Log into Slack.
2. Click Slack's "New message" button.
3. Start to type some random characters into the "To:" field.
4. Press Ctrl+A to select all the characters you just typed.
5. Type some other random characters to replace the selected random characters.

Expected behavior

The new characters should replace the selected characters.

Actual behavior

The new characters are appended to the selected characters. The STR works correctly for other Slack text fields. I can only reproduce this bug in Slack's New message "To:" field.

I have only tested Firefox on Windows 11.

Comment 1

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

Hmm, it's really complicated case. The editing host <div> is a flex container. And that has 3 <span>s, first 2 are editable, and the other is not editable. The middle span contains the text which you type. The <span>s may be treated as blocks newly, so, deleting range may have been changed by the new behavior.

On the other hand, while HTMLEditor replaces text, it works as far as checked in the debugger. Therefore, the text was restored by Slack itself. So, I need to check how the legacy mode worked tomorrow.

Comment 2

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

Okay, when I type text in the editor the DOM tree becomes:

<div contenteditable style="display:flex">
  <span class="c-multi_select_input__space" />
  <span class="c-multi_select_input__filter_query">text which I typed</span>
  <span class="c-multi_select_input__space" contenteditable="false" />
</div>

In both modes, selection start is adjusted to <span class="c-multi_select_input__filter_query">[text. Then, in the legacy mode, the range is not extended and only the text node and the last <span> are deleted, however, in the new mode, the range is extended and the 2nd and the last <span>s are deleted. Then, Slack restores the content before the deletion. So, I need to think how to behave in this case without breaking the other web apps...

Comment 3

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

Hmm, Chrome does same behavior. So, Slack might do something for Chrome...
https://jsfiddle.net/d_toybox/q7ez3bg2/

Comment 4

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

Oh, I found an odd behavior of Chrome.
https://jsfiddle.net/d_toybox/q7ez3bg2/2/

If there is the empty <span> before the text container, the last non-editable <span> is not removed in Chrome. That might avoid this bug in Slack.

Comment 5

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

Comment 2 is wrong. Maybe I was confused between Chrome and Firefox windows.

Comment 6

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

Okay, Chrome does not remove the non-editable <span>, however, even if we stopped removing the non-editable node, I still see this bug. So, it seems that Slack does something different (I couldn't align the behavior with changing UA string though).

Comment 7

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

Filed a Chromium issue about odd behavior when flex container has a non-editable element.

Comment 8

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

Okay, let's forget about the non-editable node. That is not related to us. The root cause is,

<div contenteditable><span>abc</span><span style="display:block">{def</span>}</div> should not delete the 2nd <span> before inserting text. Chrome does not do that.

Comment 9

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

Attachment: Bug 1868641 - Make `AutoDeleteRangesHandler::ExtendOrShrinkRangeToDelete` not cross flex-item and grid-item boundary r=m_kato!

The text input of the multi-select combobox of Slack has the following
structure:

<div contenteditable style="display:flex">
  <span class="c-multi_select_input__space">&nbsp;</span>
  <span class="c-multi_select_input__filter_query">text which you typed</span>
  <span class="c-multi_select_input__space" contenteditable="false">&nbsp;<span>
</div>

When you do "Select All", they adjust selection to start from start of the
text container <span> (i.e., .c-multi_select_input__filter_query).
Then, typing new character deletes selection first. At this time, in the
legacy mode, AutoDeleteRangesHandler::ExtendOrShrinkRangeToDelete does not
extend the deleting range because <span> is an inline element. However,
in the new mode, it extends the range because the <span> is a block since
it's a flex-item, and selection starts from current block boundary. Then,
deleting range starts before the text container. Finally, Gecko removes the
text container and the following non-editable <span>. Therefore, typing text
will be inserted as a child of the flex container which is the editing host.
Finally, Slack restores the previous structure and collapse selection to end
of the text.

Currently, Chrome does same thing for normal blocks without flex/gird container.
However, doing i in AutoDeleteRangesHandler::ExtendOrShrinkRangeToDelete
causes a lot of regressions. Therefore, this patch tries to avoid only the bug
in Slack. (I think that we need to redesign the deletion handler to fix it.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13548863a724
Make `AutoDeleteRangesHandler::ExtendOrShrinkRangeToDelete` not cross flex-item and grid-item boundary r=m_kato

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43641 for changes under testing/web-platform/tests

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/13548863a724

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 14

[Ciprian Georgiu, Desktop QA]

I have reproduced this issue with STR from comment 0, on an affected Nightly build (122.0a1, 2023-12-06).

The issue is verified as fixed on Beta 122.0b8 across platforms, Win 11 x64, macOS 13 and Ubuntu 20.04 x64.