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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

RESOLVED FIXED
11 years ago
10 months ago

People

(Reporter: stig, Assigned: liucougar)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

11 years ago
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
Status: UNCONFIRMED → NEW
Component: General → Editor
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → editor
Version: unspecified → Trunk

Comment 2

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

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

Comment 4

10 years ago
Posted patch proposed patch (obsolete) — Splinter Review
don't promote to an ancestor node if it has different isEditable state
(Assignee)

Updated

10 years ago
Attachment #400912 - Flags: review?(roc)
Attachment #400912 - Flags: review?(roc) → review?(peterv)
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!
(Assignee)

Comment 6

10 years ago
Posted patch patch with test case (obsolete) — Splinter Review
add unit test to the patch
Assignee: nobody → liucougar
Attachment #400912 - Attachment is obsolete: true
Attachment #405639 - Flags: review?(peterv)
Attachment #400912 - Flags: review?(peterv)
Comment on attachment 405639 [details] [diff] [review]
patch with test case

Why does this only check the frontNode against its parent?
I meant against the node.
(Assignee)

Comment 9

10 years ago
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 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.
Attachment #405639 - Flags: review?(peterv) → review+
(Assignee)

Comment 11

10 years ago
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)
Attachment #405639 - Attachment is obsolete: true
Attachment #407076 - Flags: review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5fef6a5ab9dc
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1

Comment 13

11 months ago
This topic should be reopen.

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

Updated

10 months ago
See Also: → bug 1466417
You need to log in before you can comment on or make changes to this bug.