Closed Bug 1735608 (align-join/split-nodes-direction) Opened 3 years ago Closed 1 year ago

In contentEditable, Firefox deletes block-level elements "backwards" compared to OS and other browsers

Categories

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

defect

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
relnote-firefox --- 115+
firefox115 --- fixed

People

(Reporter: just.1.jake, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Hello, I'm working on new features for Notion, a contentEditable-based collaborative application.

In most editors I've used (eg, Pages.app, Google Docs, and contentEditable in Chrome and Safari), the algorithm for deleting several lines in an editor behaves like this:

  1. Remove all the text covered by selection, including all block-level nodes completely covered by the selection
  2. Move the remaining un-selected text in the node containing the end position of the selection after the start position of the selection in the node containing the start position of the selection.
  3. Remove the (now-empty) node containing the end position of the selection.
  4. Collapse the selection to the start position of the selection.

In Firefox, it appears that the text deletion algorithm is reversed: instead of moving the suffix text of the end node into the start node then removing the end node, Firefox moves the prefix text before the start of the selection to the end node before the selection end position, removes the start node, and collapses the selection to the selection end position.

I built a tool that records the DOM mutations as seen by MutationObserver while editing a contentEditable node to explore this behavior in various browsers. You can play with the tool here.

In Safari, I make a selection starting in the first paragraph and ending in the third paragraph:

https://monosnap.com/direct/FsHFffkPRSdUjrM1S05g9qiMQWx2dj

Then, I press backspace. Safari (roughly) follows the algorithm I outlined above, first removing the covered block-level node <p id="second">, then removing the node containing the end position, before finally appending the remaining text in in the end block node into the start block node after the collapsed selection:

https://monosnap.com/direct/aTY9fWTs1UOOc3JjUbFkP7tc95xaHb

Next I'll show the mutation log for Firefox with an identical initial selection state:

https://monosnap.com/direct/v4KQqHSPj9Gj1egiVI7zsRjV6abEqg

The Firefox log is more concise, but the behavior is unexpected: the text content is as expected, but instead of the text being in the first node (<p id="first">), all the text is instead in the third node (<p id="third">).

The issue as described occurs for both deletion using the backspace key (delete on Mac keyboards) as well as "overwrites" by typing new content (including content composed with an InputMethodEditor) over an expanded text selection.

Partial work-around

For non-InputMethodEditor input, it's fine to listen to the beforeinput event using addEventListener('beforeinput', ...) event, prevent default, and apply the edit to our document model ourselves, following the standard deletion algorithm.

Frustrations

It's more difficult to work around this issue for InputMethodEditor input, which is not cancellable, and even if we could cancel the input, that would break the user's expectation that the IME should open in the start node to compose the first keystroke.

The typical approach for contentEditable is to let the browser do whatever it wants, and then to parse the DOM to figure out what kind of semantic action happened. But, this approach is complicated in Firefox because the identity of the remaining node is incorrect, compared to our desired edit and other browsers, so we can't use attributes like id to aid our reconciliation.

Conclusion

  • Is this a bug?
  • Is there a way to convince Firefox to use the standard deletion algorithm as described above? That would significantly reduce implementation burden for contentEditable editors across browsers.

(In reply to Jake Teton-Landis from comment #0)

The Firefox log is more concise, but the behavior is unexpected: the text content is as expected, but instead of the text being in the first node (<p id="first">), all the text is instead in the third node (<p id="third">).

That's a known issue. Bug 1449874 treats the opposite case (i.e., splitting case).

Frustrations

It's more difficult to work around this issue for InputMethodEditor input, which is not cancellable, and even if we could cancel the input, that would break the user's expectation that the IME should open in the start node to compose the first keystroke.

Right. Canceling beforeinput for composition is harder, that's why they are currently not cancelable.

The typical approach for contentEditable is to let the browser do whatever it wants, and then to parse the DOM to figure out what kind of semantic action happened. But, this approach is complicated in Firefox because the identity of the remaining node is incorrect, compared to our desired edit and other browsers, so we can't use attributes like id to aid our reconciliation.

Well, any attributes are really complicated how they are treated when joining 2 elements. There is no standard, and perhaps, following Chrome's behavior makes most developers happy unless Chrome's behavior does not make sense.

Conclusion

  • Is this a bug?

About the difference (which node should be left and the other should be removed) should be fixed.

  • Is there a way to convince Firefox to use the standard deletion algorithm as described above? That would significantly reduce implementation burden for contentEditable editors across browsers.

There is no standard, and it's hard to standardize because your example is really simple case, but joining different elements are harder to standardize, e.g., joining list item element and another block level element, etc. Perhaps, we can fix this soon, but keeping style with <span> element is hard because currently Gecko does not have such code.

On the other hand, if we fix this, some existing web apps would be broken. For example, web apps may not check the DOM tree actually, but considering how to update the DOM tree only with UA check. Once we would meet such case, it's hard to change Gecko's behavior.

Only from your example, updating id (and the other) attribute is a way to fix on web apps side, but I guess that you think it's hard. Could you explain more how serious current Gecko's behavior?

Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(just.1.jake)
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All

First, by "convince Firefox" I didn't mean "convince the people who write Firefox", I meant "make the existing browser implementation to do what I want by constructing a DOM with a specific, magical shape if possible". I apologize for the unclear communication.

Perhaps, we can fix this soon, but keeping style with <span> element is hard because currently Gecko does not have such code.

I am not concerned about inline character styling or really any newly-created DOM nodes.

Only from your example, updating id (and the other) attribute is a way to fix on web apps side, but I guess that you think it's hard. Could you explain more how serious current Gecko's behavior?

The biggest problem is that we use a declarative-style framework (React) to create our DOM elements. The framework expects the DOM's structure to be more-or-less in the state that we specify in the virtual DOM we hand to the framework for rendering, and the framework has its own pointers to these DOM nodes. There's no good way for us to reach into the framework and replace these pointers when we notice a Firefox-style backwards deletion mutation. So, we can't just copy over some attributes so some querySelector matches up unfortunately. This is what I mean by "identity" of the DOM node.

In other browsers, when we record a multi-block deletion, the remaining DOM node containing the collapsed selection caret matches the DOM we want to continue to exist after the deletion, and the removed DOM nodes line up with the DOM nodes we'll ask the framework to remove anyways. But in Firefox, the remaining DOM node containing the selection (in the example above, <p id="third"> ) is associated with a block that we are deleting in the data model. So, the framework happily removes it when we re-render after the state change and renders a fresh copy of <p id="first">, but this breaks the IME input since the node the input is composing into was removed.

Flags: needinfo?(just.1.jake)

It seems like a work-around path forward is to suppress any re-rendering from our framework until the composition ends, and do the kind of remapping between the desired remaining block and the DOM node that Firefox leaves after a deletion above the DOM and framework level. This will take a lot of work and add much complexity to our implementation, but it's still less costly than rewriting our editor (to remove React) in the short term.

Or, if there's some unknown magic that would let us save the current IME composition state in the remaining node, do our re-render, and then restore the composition state into a new DOM node of our choosing, but I haven't heard of any way of doing that.

In any case, thank you for reading over my issue and for any other thoughts or advice you can provide in this matter, I appreciate it!

(In reply to Jake Teton-Landis from comment #2)

The biggest problem is that we use a declarative-style framework (React) to create our DOM elements. The framework expects the DOM's structure to be more-or-less in the state that we specify in the virtual DOM we hand to the framework for rendering, and the framework has its own pointers to these DOM nodes. There's no good way for us to reach into the framework and replace these pointers when we notice a Firefox-style backwards deletion mutation. So, we can't just copy over some attributes so some querySelector matches up unfortunately.

Thank you for providing the detail! Sounds like that this is more important issue than I've thought.

Assignee: nobody → masayuki
Severity: S3 → S2
Status: NEW → ASSIGNED
Priority: P3 → P2

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Thank you for providing the detail! Sounds like that this is more important issue than I've thought.

Hello again, I am trying to evaluate the best path forward on this issue for my project. When do you think this issue would be addressed? Would it help if my organization offered to sponsor this issue?

Flags: needinfo?(masayuki)

(In reply to Jake Teton-Landis from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Thank you for providing the detail! Sounds like that this is more important issue than I've thought.

Hello again, I am trying to evaluate the best path forward on this issue for my project. When do you think this issue would be addressed? Would it help if my organization offered to sponsor this issue?

Well, I'm not at the position I can answer about business. Currently, I'm working on this bug about half of my work time. However, unfortunately, this requires more changes which I've thought mainly at splitting the nodes e.g., typing Enter key (we need to change both splitting/joining nodes behavior at same time because the opposite behavior is required for undo/redo.

Hsin-Yi-san may have some comments about your offer.

Flags: needinfo?(masayuki) → needinfo?(htsai)

Currently, I'm working on this bug about half of my work time.

Okay, good to know and thanks for the help. It's hard to know sometimes from the Bugzilla if an assigned issue is being worked on, even if it's assigned. I'm also not sure what the correct etiquette is, so I appreciate your patience with me.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

(In reply to Jake Teton-Landis from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Thank you for providing the detail! Sounds like that this is more important issue than I've thought.

Hello again, I am trying to evaluate the best path forward on this issue for my project. When do you think this issue would be addressed? Would it help if my organization offered to sponsor this issue?

Well, I'm not at the position I can answer about business. Currently, I'm working on this bug about half of my work time. However, unfortunately, this requires more changes which I've thought mainly at splitting the nodes e.g., typing Enter key (we need to change both splitting/joining nodes behavior at same time because the opposite behavior is required for undo/redo.

Hsin-Yi-san may have some comments about your offer.

Sorry for the late reply. And thank you for being willing to help, Jake!
Helping us testing when the changes are ready is always helpful. That's always the place we'd love to use help with. :)

Flags: needinfo?(htsai)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

(In reply to Jake Teton-Landis from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Thank you for providing the detail! Sounds like that this is more important issue than I've thought.

Hello again, I am trying to evaluate the best path forward on this issue for my project. When do you think this issue would be addressed? Would it help if my organization offered to sponsor this issue?

Well, I'm not at the position I can answer about business. Currently, I'm working on this bug about half of my work time. However, unfortunately, this requires more changes which I've thought mainly at splitting the nodes e.g., typing Enter key (we need to change both splitting/joining nodes behavior at same time because the opposite behavior is required for undo/redo.

Hsin-Yi-san may have some comments about your offer.

Sorry for the late reply. And thank you for being willing to help, Jake!
Helping us testing when the changes are ready is always helpful. That's always the place we'd love to use help with. :)

Filling in for Jake for the next few weeks - we're more than happy to test any changes on nightly builds.

I appreciate seeing all of the fixed issues in this thread. Is there a way for me to gauge progress on this issue otherwise?

The new behavior is not testable yet. I'll restart to create a pref to enable the new behavior. Sorry for the delay to work on this due to my private matter in the last month.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 12/27-1/4) from comment #10)

The new behavior is not testable yet. I'll restart to create a pref to enable the new behavior. Sorry for the delay to work on this due to my private matter in the last month.

Thanks for the quick response - we'll be eagerly awaiting a pref for testing.

Alias: align-join/split-nodes-direction
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(htsai)

Now, no unexpected failures of WPT in my local repository, but there are some unexpected failures in mochitests.

editor/libeditor/tests/test_bug772796.html

caused by our traditional bug which loses white-space style at joining different white-space style blocks, this is handled in bug 1782911.

editor/libeditor/tests/test_dragdrop.html

The errors are:

TEST-PASS | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <input>: dataTransfer should have selected text as "text/plain" 
TEST-PASS | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <input>: dataTransfer should have have selected nodes as "text/html" 
Buffered messages finished
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <input>: Failed to emulate drag and drop ("dragend" event is not fired by nsIDragService.endDragSession()) 
SimpleTest.ok@SimpleTest/SimpleTest.js:414:16
trySynthesizePlainDragAndDrop@editor/libeditor/tests/test_dragdrop.html:119:9
[Child 3883, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:1600
TEST-PASS | editor/libeditor/tests/test_dragdrop.html | copy-dragging multiple-line text in contenteditable to <input>: dataTransfer should have selected text as "text/plain" 
TEST-PASS | editor/libeditor/tests/test_dragdrop.html | copy-dragging multiple-line text in contenteditable to <input>: dataTransfer should have have selected nodes as "text/html" 
[Child 3883, Main Thread] WARNING: '!editingHost', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:875
[Child 3883, Main Thread] WARNING: mozInlineSpellCHecker::UpdateCurrentDictionary() failed, but ignored: 'NS_SUCCEEDED(rvIgnored)', file /builds/worker/checkouts/gecko/editor/libeditor/EditorBase.cpp:5665

and

[Child 3883, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:1600
TEST-PASS | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <textarea>: dataTransfer should have selected text as "text/plain" 
TEST-PASS | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <textarea>: dataTransfer should have have selected nodes as "text/html" 
[Child 3883, Main Thread] WARNING: '!editingHost', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:875
[Child 3883, Main Thread] WARNING: mozInlineSpellCHecker::UpdateCurrentDictionary() failed, but ignored: 'NS_SUCCEEDED(rvIgnored)', file /builds/worker/checkouts/gecko/editor/libeditor/EditorBase.cpp:5665
Not taking screenshot here: see the one that was previously logged
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_dragdrop.html | dragging multiple-line text in contenteditable to <textarea>: Failed to emulate drag and drop ("dragend" event is not fired by nsIDragService.endDragSession()) 
SimpleTest.ok@SimpleTest/SimpleTest.js:414:16
trySynthesizePlainDragAndDrop@editor/libeditor/tests/test_dragdrop.html:119:9
[Child 3883, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:1600

I'm still not sure why the drag synthesizer fails to receive dragend event with the join direction change.

editor/libeditor/tests/test_inline_style_cache.html

The errors are:

[Child 3883, Main Thread] WARNING: '!content', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:6457
[Child 3883, Main Thread] WARNING: '!content', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:6457
[Child 3883, Main Thread] WARNING: '!content', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:6457
[Child 3883, Main Thread] WARNING: '!content', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:6457
[Child 3883, Main Thread] WARNING: '!content', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:6457
Not taking screenshot here: see the one that was previously logged
Buffered messages logged at 08:45:37
must wait for load
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #01-01 At typing something after setting some styles, should cause inserting some nodes to apply the style 
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #01-02-1 At typing something after Delete after setting style, should cause inserting some nodes to apply the style 
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #01-02-2 At typing something after Backspace after setting style, should cause inserting some nodes to apply the style 
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #01-03-1 Typing Enter after setting style should not ignore the styles 
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #01-03-2 Typing Enter after setting style should not ignore the styles 
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #02-01 At replacing "selection" after setting some styles, should keep the styles at inserting text 
Buffered messages finished
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_inline_style_cache.html | #02-02 After removing "selection" after setting some styles, should not keep the styles - got "before<strike><i><b>test</b></i></strike>after", expected "beforetestafter"
SimpleTest.is@SimpleTest/SimpleTest.js:497:14
@editor/libeditor/tests/test_inline_style_cache.html:110:5
SimpleTest.waitForFocus/<@SimpleTest/SimpleTest.js:1038:13
[Child 3883, Main Thread] WARNING: '!HTMLEditUtils::IsSplittableNode(*editableBlockElement)', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditSubActionHandler.cpp:1770
Not taking screenshot here: see the one that was previously logged
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_inline_style_cache.html | #02-03-1 Typing Enter after setting style to selected text should keep the styles - got "<div>beforeaftertest</div><br>", expected "<div>before</div><div><strike><i><b>test</b></i></strike>after</div>"
SimpleTest.is@SimpleTest/SimpleTest.js:497:14
@editor/libeditor/tests/test_inline_style_cache.html:123:5
SimpleTest.waitForFocus/<@SimpleTest/SimpleTest.js:1038:13
Not taking screenshot here: see the one that was previously logged
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_inline_style_cache.html | #02-03-2 Typing Enter after setting style to selected text should keep the styles - got "<p>before</p><p>testafter</p>", expected "<p>before</p><p><strike><i><b>test</b></i></strike>after</p>"
SimpleTest.is@SimpleTest/SimpleTest.js:497:14
@editor/libeditor/tests/test_inline_style_cache.html:135:5
SimpleTest.waitForFocus/<@SimpleTest/SimpleTest.js:1038:13
TEST-PASS | editor/libeditor/tests/test_inline_style_cache.html | #03-01 Replacing text in styled inline elements should respect the styles

The warning are the big hint. It means that we fail to compute active editing host from Selection. I.e., we fail to put caret somewhere. I guess that this can be fixed with a simple patch for this (or needs refactoring if it's caused by our traditional spaghetti).

The browser scope test failures

TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browserscope/test_richtext2.html | Browserscope richtext2 selection: CC-Proposed-BC:gray_P-SPANs:bc:b-3_SO-dM - got +0, expected 1
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browserscope/test_richtext2.html | Browserscope richtext2 selection: CC-Proposed-BC:gray_P-SPANs:bc:b-3_SO-body - got +0, expected 1
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browserscope/test_richtext2.html | Browserscope richtext2 selection: CC-Proposed-BC:gray_P-SPANs:bc:b-3_SO-div - got +0, expected 1
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browserscope/test_richtext2.html | Browserscope richtext2 value: U-Proposed-U_U-S-1_SO-dM - got +0, expected 1
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browserscope/test_richtext2.html | Browserscope richtext2 value: U-Proposed-U_U-S-1_SO-body - got +0, expected 1 

I've expected that this is fixed by the fix of bug 1781994, but I still see these failures. I need to investigate this.

Although there are some problems with execCommand("bold") etc, you can check the compatible behavior with the other browsers with enabling editor.join_split_direction.compatible_with_the_other_browsers to true from about:config or add a domain to enable it in editor.join_split_direction.force_use_compatible_direction in about:config in the latest Nightly build.

If you find problems with new behavior, please file a bug and let me know with making it block this bug or commented the new bug# in this bug.

Update: Now, we started QA to ship the new behavior by default. We'd be happy if anybody reports new issues in existing web apps with enabling editor.join_split_direction.compatible_with_the_other_browsers in 109 or later before shipping the new behavior.

Now, we have patches to fix all known failures in automated tests. We'll enable new behavior by default in the Nightly channel and early beta builds (bug 1820116).

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/ba0e443021ba Ship the new (compatible) join/split node direction in all channels r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
Regressions: 1834068

I think this is worth noting in the release note. WDYT? And if it sounds good to you, please nominate this to the release note. Thank you.

Flags: needinfo?(masayuki)

Release Note Request (optional, but appreciated)
[Why is this notable]: This is one of the biggest builtin editor behavior changes.
[Affects Firefox for Android]: Yes.
[Suggested wording]:
The builtin editor (for contenteditable and designMode) starts to behave similar to the other browsers when splitting a node (e.g., typing Enter to split a paragraph) and joining two nodes (e.g., typing Backspace at start of a paragraph to join the paragraph and previous one). When a node is split, builtin editor creates a new node after the original one instead of before, i.e., creates the right node instead of the left node. Similarly, when two nodes are joined, the builtin editor deletes the latter node and moves its children to end of the preceding node instead of deleting the former node and moving its child to start of the following node.
[Links (documentation, blog post, etc)]: Nothing.

relnote-firefox: --- → ?
Flags: needinfo?(masayuki)
Duplicate of this bug: 1449874

Thanks, added a slightly reworded note to the Fx115 Nightly release notes. Keeping the relnote? flag open to keep it on the radar for inclusion in our final release notes.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: