Last Comment Bug 414526 - [contenteditable] DELETE & BACKSPACE when selection at the beginning or the end of a contenteditable element affects the adjacent contenteditable elements.
: [contenteditable] DELETE & BACKSPACE when selection at the beginning or the e...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: All All
: P3 major with 9 votes (vote)
: mozilla8
Assigned To: :Ehsan Akhgari
:
Mentors:
: 506661 577367 640927 694411 694783 (view as bug list)
Depends on:
Blocks: contenteditable
  Show dependency treegraph
 
Reported: 2008-01-29 02:01 PST by Dan POPA
Modified: 2011-10-15 19:39 PDT (History)
20 users (show)
roc: wanted‑next+
roc: blocking1.9-
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
BACKSPACE touches once the contenteditable element before current one and removes characters from the end of the first child of this contenteditable element (3.32 KB, text/html)
2008-01-29 02:17 PST, Dan POPA
no flags Details
BACKSPACE touches once the contenteditable element before current one and removes characters from the end of the first child of this contenteditable element. (5.17 KB, text/html)
2008-01-29 03:14 PST, Dan POPA
no flags Details
DELETE key inside an contenteditable element affects content in the next adjacent contenteditable element. (4.45 KB, text/html)
2008-01-29 03:33 PST, Dan POPA
no flags 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. (3.91 KB, text/html)
2008-01-29 03:41 PST, Dan POPA
no flags Details
2 more BACKSPACE wrong behavior testcases. (2.77 KB, text/html)
2008-01-29 04:52 PST, Dan POPA
no flags Details
another error case with backspace wrt inline contentEditable=false elements (2.56 KB, text/html)
2008-11-19 17:09 PST, liucougar
no flags Details
testcase showing also other problems (with delete key, caret display and position) (2.75 KB, text/html)
2010-05-06 02:39 PDT, luxigo
no flags Details
testcase showing also other problems (with delete key, replacing selection, caret display and position) (2.82 KB, text/html)
2010-05-06 02:49 PDT, luxigo
no flags Details
Patch v1.0 (11.34 KB, patch)
2010-08-11 01:55 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v2.0 (15.89 KB, patch)
2010-08-11 11:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v3.0 (24.75 KB, patch)
2011-03-21 23:28 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (v4) (35.15 KB, patch)
2011-07-14 08:27 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description Dan POPA 2008-01-29 02:01:57 PST
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 2008-01-29 02:17:48 PST
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
Comment 2 Dan POPA 2008-01-29 02:20:44 PST
This bug is related to bug 332635.
Asking for the blocking1.9 flag.
Comment 3 Dan POPA 2008-01-29 03:04:35 PST
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 2008-01-29 03:14:01 PST
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.
Comment 5 Dan POPA 2008-01-29 03:33:46 PST
Created attachment 299983 [details]
DELETE key inside an contenteditable element affects content in the next adjacent contenteditable element.
Comment 6 Dan POPA 2008-01-29 03:41:33 PST
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.
Comment 7 Dan POPA 2008-01-29 04:52:53 PST
Created attachment 299992 [details]
2 more BACKSPACE wrong behavior testcases.
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2008-02-23 09:50:15 PST
is bug 250891 which has testcase for message compose window related to comment 4?
Comment 9 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2008-04-23 18:08:11 PDT
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 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2008-04-23 19:08:19 PDT
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 2008-11-19 17:09:00 PST
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 Tobias Buschor 2010-03-11 05:18:13 PST
I think in ff 3.6 the bug is closed...
Comment 13 Dan POPA 2010-03-11 05:55:48 PST
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 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-05-04 22:43:30 PDT
*** Bug 506661 has been marked as a duplicate of this bug. ***
Comment 15 luxigo 2010-05-06 02:39:17 PDT
Created attachment 443837 [details]
testcase showing also other problems (with delete key, caret display and position)
Comment 16 luxigo 2010-05-06 02:49:11 PDT
Created attachment 443838 [details]
testcase showing also other problems (with delete key, replacing selection, caret display and position)
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-08-11 01:55:39 PDT
Created attachment 464735 [details] [diff] [review]
Patch v1.0
Comment 18 :Ehsan Akhgari 2010-08-11 08:24:55 PDT
Should this be reviewed?
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-08-11 10:29:29 PDT
Wait a couple of days. I'm writing tests and I should test it on tryserver.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-08-11 11:39:58 PDT
Created attachment 464886 [details] [diff] [review]
Patch v2.0
Comment 21 :Ehsan Akhgari 2011-03-16 13:50:55 PDT
Masayuki: are you planning to continue working on this?
Comment 22 :Ehsan Akhgari 2011-03-16 13:51:32 PDT
*** Bug 640927 has been marked as a duplicate of this bug. ***
Comment 23 :Ehsan Akhgari 2011-03-16 13:52:11 PDT
*** Bug 577367 has been marked as a duplicate of this bug. ***
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-18 04:02:43 PDT
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 :Ehsan Akhgari 2011-03-18 12:26:51 PDT
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] (Mozilla Japan) 2011-03-21 23:28:36 PDT
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.
Comment 27 :Ehsan Akhgari 2011-07-14 08:27:05 PDT
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.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-14 16:52:01 PDT
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 30 Eric Shepherd [:sheppy] 2011-08-23 13:42:11 PDT
Listed on Firefox 8 for developers.
Comment 31 Alice0775 White 2011-10-15 19:39:24 PDT
*** Bug 694783 has been marked as a duplicate of this bug. ***
Comment 32 Alice0775 White 2011-10-15 19:39:41 PDT
*** Bug 694411 has been marked as a duplicate of this bug. ***

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