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

RESOLVED FIXED in mozilla8

Status

()

Core
Editor
P3
major
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Dan POPA, Assigned: Ehsan)

Tracking

({dev-doc-complete})

unspecified
mozilla8
dev-doc-complete
Points:
---
Bug Flags:
wanted-next +
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 5 obsolete attachments)

5.17 KB, text/html
Details
4.45 KB, text/html
Details
3.91 KB, text/html
Details
2.77 KB, text/html
Details
2.56 KB, text/html
Details
2.82 KB, text/html
Details
35.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created 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
(Reporter)

Comment 2

10 years ago
This bug is related to bug 332635.
Asking for the blocking1.9 flag.
Flags: blocking1.9?
(Reporter)

Comment 3

10 years ago
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
(Reporter)

Comment 4

10 years ago
Created attachment 299981 [details]
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.
(Reporter)

Comment 5

10 years ago
Created attachment 299983 [details]
DELETE key inside an contenteditable element affects content in the next adjacent contenteditable element.
(Reporter)

Comment 6

10 years ago
Created attachment 299984 [details]
DELETE key removes invalid content (or it doesn't) when selection is inside/over a BR located before some element inside an contenteditable region.
(Reporter)

Comment 7

10 years ago
Created attachment 299992 [details]
2 more BACKSPACE wrong behavior testcases.
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-

Updated

10 years ago
Flags: tracking1.9+

Updated

10 years ago
Blocks: 237964
Status: UNCONFIRMED → NEW
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() ( 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?
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

9 years ago
Created attachment 349088 [details]
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

8 years ago
I think in ff 3.6 the bug is closed...
(Reporter)

Comment 13

8 years ago
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

Comment 15

7 years ago
Created attachment 443837 [details]
testcase showing also other problems (with delete key, caret display and position)

Comment 16

7 years ago
Created attachment 443838 [details]
testcase showing also other problems (with delete key, replacing selection, caret display and position)
Attachment #443837 - Attachment is obsolete: true
Created attachment 464735 [details] [diff] [review]
Patch v1.0
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Should this be reviewed?
Wait a couple of days. I'm writing tests and I should test it on tryserver.
Created attachment 464886 [details] [diff] [review]
Patch v2.0
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
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.
OK, no problem.  Please send the latest version of the patch that you have, and I'll start working on it.
Created attachment 520869 [details] [diff] [review]
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.
Attachment #464886 - Attachment is obsolete: true
Assignee: masayuki → ehsan
Whiteboard: [post-2.0]
Created attachment 545916 [details] [diff] [review]
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.
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+
http://hg.mozilla.org/mozilla-central/rev/7749420447fa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed
Listed on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Duplicate of this bug: 694783

Updated

6 years ago
Duplicate of this bug: 694411
You need to log in before you can comment on or make changes to this bug.