414526 - [contenteditable] DELETE & BACKSPACE when selection at the beginning or the end of a contenteditable element affects the adjacent contenteditable elements.

Status: RESOLVED FIXED

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

Description

[Dan POPA]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

DELETE & BACKSPACE when selection at the beginning or the end of a contenteditable element affects the adjacent contenteditable elements.

Reproduction steps needs verbose description for each case, so I will add an attachment for each case. Attachments may also be usefully for debugging as they contain an event log for the mutation events fired on the document while editing.

Reproducible: Always

Comment 1

[Dan POPA]

Attachment: BACKSPACE touches once the contenteditable element before current one and removes characters from the end of the first child of this contenteditable element

Comment 2

[Dan POPA]

This bug is related to bug 332635.
Asking for the blocking1.9 flag.

Comment 3

[Dan POPA]

Comment on attachment 299976 [details]
BACKSPACE touches once the contenteditable element before current one and removes characters from the end of the first child of this contenteditable element

Marking first attachment obsolete, will be replaced with an updated version.

Comment 4

[Dan POPA]

Attachment: BACKSPACE touches once the contenteditable element before current one and removes characters from the end of the first child of this contenteditable element.

Re-added first attachment, found more details on how to duplicate.

Comment 5

[Dan POPA]

Attachment: DELETE key inside an contenteditable element affects content in the next adjacent contenteditable element.

Comment 6

[Dan POPA]

Attachment: DELETE key removes invalid content (or it doesn't) when selection is inside/over a BR located before some element inside an contenteditable region.

Comment 7

[Dan POPA]

Attachment: 2 more BACKSPACE wrong behavior testcases.

Comment 8

[Wayne Mery (:wsmwk)]

is bug 250891 which has testcase for message compose window related to comment 4?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Adding PeterV to CC, PeterV, what do you think about this?

Firefox 2 suffers from this, though only in designMode, so this isn't a regression.

In the testcase <div contenteditable="true"><br><ul><li>something</li></ul></div>, if you delete the <br>, it removes the next <li>'s text from the <ul>, and renders that text below the <ul>.

The problem is the call to nsHTMLEditor::IsVisBreak() ( http://mxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditor.cpp#1141 ) in nsHTMLEditRules::WillDeleteSelection():
http://mxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditRules.cpp#2015

nsHTMLEditor::IsVisBreak() is returning false, so we're assuming that deleting the <br> it will have no effect, so we delete it, then carry on and delete the next thing or some such. The assumption by IsVisBreak() is that a <br> next to a block node is invisible, but that assumption is not always true. It should be a <br> before a block node is only visible when the <br> has only non-visible whitespace before it, otherwise it's invisible.

I've discussed this with Roc, and his suggestion is that we don't do things this way. We're trying to figure out if deleting the node at the caret will cause a layout change. He suggests instead of trying to figure out what layout will do, we just go ahead and delete the node, flush layout, and figure out if the layout actually changed. This would not be a trivial change, as IsVisBreak() is used a number of places in the editor code, so that would have to be wanted-next.

In the meantime I could hack IsVisBreak() to kinda do what we want... What do people think?

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

There are more problems here too, we're allowing the delete to propagate outside of the contenteditable area, maybe we should check that the node-being-deleted is a descendant of the editable root.

Comment 11

[liucougar]

Attachment: another error case with backspace wrt inline contentEditable=false elements

find another broken cases with backspace, can be reproduced with FF 3.0.4 and FF3.1beta1 20081118 nighly build, please open this attached file and follow the on screen instruction

Comment 12

[Tobias Buschor]

I think in ff 3.6 the bug is closed...

Comment 13

[Dan POPA]

Only the cases described in attachment 299983 [details] (second attachment) seems to be fixed.

I can reproduce all others in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100310 Minefield/3.7a3pre

Comment 15

[luxigo]

Attachment: testcase showing also other problems (with delete key, caret display and position)

Comment 16

[luxigo]

Attachment: testcase showing also other problems (with delete key, replacing selection, caret display and position)

Comment 17

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

Attachment: Patch v1.0

Comment 18

[(no longer active)]

Should this be reviewed?

Comment 19

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

Wait a couple of days. I'm writing tests and I should test it on tryserver.

Comment 20

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

Attachment: Patch v2.0

Comment 21

[(no longer active)]

Masayuki: are you planning to continue working on this?

Comment 24

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

Ehsan:

Sorry for the delay. I'm checking the state of latest patch which is in my local.

But I don't have much time for now, I hope somebody continues to work for this bug after my next post.

Comment 25

[(no longer active)]

OK, no problem.  Please send the latest version of the patch that you have, and I'll start working on it.

Comment 26

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

Attachment: Patch v3.0

This is the latest version.

> 208 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug414526.html | Pressing backspace key at end of editor3 changes the content - got "<p id=\"editor1\" contenteditable=\"true\">editor1</p><p id=\"editor2\" contenteditable=\"true\">editor2</p><div id=\"editor3\" contenteditable=\"true\"><div>editor3editor4editor5</div></div><p id=\"editor4\" contenteditable=\"true\"></p>non-editable text<p id=\"editor5\" contenteditable=\"true\"></p>", expected "<p id=\"editor1\" contenteditable=\"true\">editor1</p><p id=\"editor2\" contenteditable=\"true\">editor2</p><div id=\"editor3\" contenteditable=\"true\"><div>editor3</div></div><p id=\"editor4\" contenteditable=\"true\">editor4</p>non-editable text<p id=\"editor5\" contenteditable=\"true\">editor5</p>"
> 472 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug551704.html | test5: The expected HTML after editing should be correct - got "abc<br>def", expected "abc<br>def<br>"
> 475 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug551704.html | test6: The expected HTML after editing should be correct - got "abc<br>def", expected "abc<br>def<br>"

This still fails backspace key handling in editor3 of new tests when caret is at the end of the editor3.

And also, this patch breaks <br> node insertion at the end of an HTML editor.

The main concept of this patch is, HTML editor must check whether all found nodes are in current focused editor or not. If found node is in another editing host, the handling should be canceled.

Comment 27

[(no longer active)]

Attachment: Patch (v4)

Note that this patch only fixes the bugs in these testcases which affect the contents of other contenteditable elements on the page.  Other bugs should be filed for problems that happen in the testcases only inside a single contenteditable element.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 545916 [details] [diff] [review]
Patch (v4)

Review of attachment 545916 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +3801,5 @@
>  }
> +
> +///////////////////////////////////////////////////////////////////////////
> +// GetNodeAtRangeOffsetPoint: returns the node at this position in a range,
> +// assuming that aParentOrNode is the node itself it if's a text node, or

if it's

Comment 29

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7749420447fa

Comment 30

[Eric Shepherd [:sheppy]]

Listed on Firefox 8 for developers.