436703 - select all + Copy/paste in contenteditable div pastes the editable div inside itself

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Stig Vestøl]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0

When a div has the contenteditable attribute set to true, and the user selects all contents (cmd + a, mouse selection, execCommand or range.selectAllChildren(DOMNode) ) and copies the contents: When you then paste the contents, the parent <div /> is pasted inside it self.

Reproducible: Always

Steps to Reproduce:
1. Go to: http://starkravingfinkle.org/blog/wp-content/uploads/2007/07/contenteditable.htm or any other demonstration of contenteditable in firefox 3
2. Select all contents of any of the editable elements
3. Copy the contents the paste it several times.
Actual Results:  
The editable element is pasted inside it self.

Expected Results:  
Selecting the contents of an editable element should not select the element itself.

Comment 1

[Ryan A. C.]

Seeing the same on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008052707 Minefield/3.0pre

-> core > editor

Comment 2

[Ryan]

I have hit the same problem with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008101315 Ubuntu/8.10 (intrepid) Firefox/3.0.3

Comment 3

[Bodo Schulze]

As this bug persists in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5, I deviced a workaround described at http://blog.cms-schulze.de/2009/07/09/work-around-gecko-contenteditable-bug-436703/

Bodo Schulze

Comment 4

[liucougar]

Attachment: proposed patch

don't promote to an ancestor node if it has different isEditable state

Comment 5

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

Comment on attachment 400912 [details] [diff] [review]
proposed patch

Looks OK to me. However an editor person needs to review this. Also needs a test. Thanks!

Comment 6

[liucougar]

Attachment: patch with test case

add unit test to the patch

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 405639 [details] [diff] [review]
patch with test case

Why does this only check the frontNode against its parent?

Comment 8

[Peter Van der Beken [:peterv]]

I meant against the node.

Comment 9

[liucougar]

I don't think I am certain what you are asking, but here is my guess

if you are asking why the patch only checks isEditable on the frontNode, because when it evaluates the (iNode->IsEditable() != isEditable), both (frontNode != parent) and (endNode != parent) evaluate to false, which means frontNode==parent==endNode,

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 405639 [details] [diff] [review]
patch with test case

>diff -r b2057b7554e6 content/base/src/nsDocumentEncoder.cpp

>+  nsCOMPtr<nsINode> inINode = do_QueryInterface(*ioNode);

I'd just name this one |node|.

>+      nsCOMPtr<nsINode> iNode = do_QueryInterface(frontNode);

and make this frontINode.

>       // if both endpoints were promoted one level, keep looping - otherwise we are done.

Please add to this comment, probably making it something like: "Keep looping if both endpoints were promoted one level, or the common ancestor is editable and the node isn't or vice versa."

>-      if ( (frontNode != parent) || (endNode != parent) )
>+      if ( (frontNode != parent) || (endNode != parent) || (iNode->IsEditable() != isEditable) )

I think it's ok to check against the node's editability as opposed to just checking for editability because we only walk up one level at a time.

Comment 11

[liucougar]

Attachment: revised patch with test case

all the points in the last comment are addressed (the last point in the last comment does not look like a suggestion for a change, correct me if I am wrong)

Comment 12

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/5fef6a5ab9dc

Comment 13

[jeansebastienpwarr]

This topic should be reopen.

Bug currently appears if (and only if) tag is a span : https://codepen.io/JNa0/pen/OZbaOO