1190161 - Joining two <div> elements with delete or backspace gives inconsistent results (Part 1)

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: Simple page to show the bug

Open the attached page.
<div><font color="red">red</font></div>
<div><font color="green">green</font></div>

Place cursor after "red". Press (forward) delete.
Result:
<div><font color="green">redgreen</font></div>
Well, I would have expected all red, but that's another issue.

Reload, the page and place the cursor before the green. Press backspace.
Result:
<div><font color="red">red</font></div><font color="green">green</font>
As far as the user is concerned, there is no visible change.
There would of course be a visible change if the removed second div had carried other properties. If we press backspace again, we still don't get the result from the first case.

This behaviour is inconsistent.

See also: bug 200416.
This was noticed when working on bug 772796.

Analysis:
Questionable processing here:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#2758
Backspace goes though here:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#2810

Comment 1

[Jorg K (CEST = GMT+2)]

Very strange: If the two <div> elements are placed onto the same line

<div><font color="red">red</font></div><div><font color="green">green</font></div>

then forward delete and backspace do the same thing.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Simple page to show the bug, 2nd version

Modified test case with two lines and one line.

Looks like the behaviour is caused by the text node containing the line break "\n" that sits between the two <div> elements.

Surely the user doesn't see this, so it should be ignored when using backspace.

Ehsan, can you enlighten us in this?

Comment 3

[(no longer active)]

This is indeed a bug.  I'm not sure what causes it exactly.  I would start debugging it by breaking inside nsHTMLEditRules::WillDeleteSelection().  There is a join variable in that function, and by the time that we get to the end of that function, we must have called JoinBlocks().  Based on the symptoms in comment 0, it seems like we're not trying to join the blocks, so I would check to see if we end up calling JoinBlocks() there.  If we do, there is something that goes wrong when joining...  Hope this gives you a good starting point!

Comment 4

[Jorg K (CEST = GMT+2)]

Thanks, I'll move onto this once bug 772796 is fixed since both affect the same source file.

Comment 5

[Jorg K (CEST = GMT+2)]

Further analysis.
xul.dll!nsHTMLEditRules::JoinBlocks() Line 2582	C++
xul.dll!nsHTMLEditRules::WillDeleteSelection() Line 2274	C++
xul.dll!nsHTMLEditRules::WillDoAction() Line 627	C++
xul.dll!nsPlaintextEditor::DeleteSelection() Line 690	C++
xul.dll!nsEditor::HandleKeyPressEvent() Line 4687	C++

So that answers the question whether JoinBlocks is called. The surprising thing is that is it called with two text nodes. Most likely, the left node is the invisible newline between the two blocks, the right node is the text "green". How can I tell, how do you print DOM nodes or nsINode in C++.

That's not so much a surprise if we look back to comment #0 which gives the result as:
<div><font color="red">red</font></div><font color="green">green</font>.

The problem is: Why is the newline not ignored?

On forward delete, we join a div to a div, so the newline text node got ignored.

Briefly looking at the case that works, where both div elements are written onto the same HTML source line:
Forward delete: joins a div to a div.
Backspace: Joins two text nodes!! Just as in the case that doesn't work. I need to identify the nodes.

Hmm. Please let me know how to print out DOM nodes/trees. In the (Windows) debugger I can't even see the text content of the node to tell which one I'm dealing with. I know about DumpFrameTree(), but I haven't found an equivalent for DOM nodes. However, there layout debugger/content can do it, so there must be a function.
(Sorry to ask such a trivial thing. I looked for a while and didn't find anything. I tried you on IRC but there was no answer.)

Comment 6

[Jorg K (CEST = GMT+2)]

OK, as suspected, nsIContent::DumpContent gives on backspace for the two text nodes:

Two div elements on one line: red/green
Two div elements on two lines: newline/green.

So I think the job would be to ignore the newline before calling the join.

Comment 7

[Jorg K (CEST = GMT+2)]

Again with nsIContent::List - sorry, I had to try my new toy ;-)

Text@105617E0 flags=[02000200] primaryframe=105DBBF0 refcount=6<red>
Text@105618D0 flags=[02000208] ranges:1 primaryframe=105DBD70 refcount=44<green>

Text@10561470 flags=[07800200] primaryframe=00000000 refcount=3<\u000a>
Text@10561560 flags=[02000208] ranges:1 primaryframe=105DB8D8 refcount=38<green>

Comment 8

[Jorg K (CEST = GMT+2)]

Further analysis:

bCollapsed at lines
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#1912
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#1949

Forward delete: first true, then false, since selection was extended.
Backspace: twice true.
Join therefore happens at:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#2273
and the left node was determined here:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#2247

Either the selection should have been extended backwards or the looking for the left/prior node should have skipped the invisible newline between the two blocks.

Comments, please.

(P.S.: I believe fixing this bug will make bug 772796 simpler, since the non-functioning backspace cases will disappear).

Comment 9

[Jorg K (CEST = GMT+2)]

Further analysis.

In *all* cases ExtendSelectionForDelete is called:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#1932

In the backspace case, we end up here:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextEditor.cpp#619
where offset == 0 and nothing gets extended.

The forward delete is much more active, I traced it to here:
xul.dll!nsFrameSelection::MoveCaret() Line 849	C++
xul.dll!nsFrameSelection::CharacterExtendForDelete() Line 2147	C++
xul.dll!PresShell::CharacterExtendForDelete() Line 2104	C++
xul.dll!nsPlaintextEditor::ExtendSelectionForDelete() Line 598	C++
xul.dll!nsHTMLEditRules::WillDeleteSelection() Line 1932	C++

IMHO (someone who (your quote) "who is not deeply familiar with [y]our DOM/layout internals" - no offense taken, you are 100% right, I know very very little) ExtendSelectionForDelete is not working as expected.

Sadly, this bug also touches on layout and frames in MoveCaret, so perhaps it is also not an easy bug for me to pursue.

Speaking with my Thunderbird hat on, this bug here does not affect Thunderbird users, since writing e-mail doesn't produce <div> elements. However, I wanted to help you, since I believe that fixing this bug will make bug 772796 simpler (sorry for saying it again). Of course I'd love to leave them both in your capable hands. So please let me know if I can be of further assistance ;-)

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

This is similar with Bug 779067. Backspace key does not extend the selection, the code falsely chooses an invisible whitespace between elements as merge target, and thus the element unexpectedly merges with its parent, not the sibling.

Comment 11

[Jorg K (CEST = GMT+2)]

There is also the "Part 2" bug.