Closed Bug 414526 Opened 12 years ago Closed 9 years ago

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


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






(Reporter: dpopa, Assigned: ehsan)



(Keywords: dev-doc-complete)


(7 files, 5 obsolete files)

5.17 KB, text/html
4.45 KB, text/html
3.91 KB, text/html
2.77 KB, text/html
2.56 KB, text/html
2.82 KB, text/html
35.15 KB, patch
: review+
Details | Diff | Splinter Review
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
This bug is related to bug 332635.
Asking for the blocking1.9 flag.
Flags: blocking1.9?
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.
Attachment #299976 - Attachment is obsolete: true
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
is bug 250891 which has testcase for message compose window related to comment 4?
Flags: wanted-next+
Flags: blocking1.9-
Flags: tracking1.9+
Ever confirmed: true
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() ( ) in nsHTMLEditRules::WillDeleteSelection():

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?
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.
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
I think in ff 3.6 the bug is closed...
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
Duplicate of this bug: 506661
Attached patch Patch v1.0 (obsolete) — Splinter Review
Assignee: nobody → masayuki
Should this be reviewed?
Wait a couple of days. I'm writing tests and I should test it on tryserver.
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #464735 - Attachment is obsolete: true
Masayuki: are you planning to continue working on this?
Duplicate of this bug: 640927
Duplicate of this bug: 577367

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.
OK, no problem.  Please send the latest version of the patch that you have, and I'll start working on it.
Attached patch Patch v3.0 (obsolete) — Splinter Review
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.
Attachment #464886 - Attachment is obsolete: true
Assignee: masayuki → ehsan
Whiteboard: [post-2.0]
Attached patch Patch (v4)Splinter Review
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.
Attachment #520869 - Attachment is obsolete: true
Attachment #545916 - Flags: review?(roc)
Whiteboard: [post-2.0]
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
Attachment #545916 - Flags: review?(roc) → review+
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Listed on Firefox 8 for developers.
Duplicate of this bug: 694783
Duplicate of this bug: 694411
You need to log in before you can comment on or make changes to this bug.