Bug 73497 - Copy/paste of the same content produces increasingly nested markup
Summary: Copy/paste of the same content produces increasingly nested markup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on: 73510
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 16:31 PST by Enrica Casucci
Modified: 2011-12-02 10:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.57 KB, patch)
2011-11-30 16:45 PST, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2011-11-30 16:31:21 PST
Start with the following markup:

<div>hello</div><div>world</div>

Select all, copy and paste over the same content.
The markup produced is:

<div><div>hello</div><div>world</div></div>

Select all again and paste.
The markup produced is:

<div><div><div>hello</div><div>world</div></div></div>

Every time you select and paste one more div is added.
Deleting the selection will leave all the nested divs.

<div><div><div><br></div></div></div>
Comment 1 Enrica Casucci 2011-11-30 16:45:04 PST
Created attachment 117294 [details]
Patch
Comment 2 Darin Adler 2011-11-30 17:00:53 PST
Comment on attachment 117294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117294&action=review

r=me; but the use of deprecatedNode here in this new code seems like a bad idea.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:752
> +    Node* node = m_endingPosition.deprecatedNode();

Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:758
> +            node = node->parentNode();

This should be:

    node = parentNode;

> Source/WebCore/editing/DeleteSelectionCommand.cpp:763
> +            if (node == m_endingPosition.deprecatedNode())

Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:767
> +            node = m_endingPosition.deprecatedNode();

Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:769
> +            node = node->parentNode();

This should be:

    node = parentNode;

> LayoutTests/editing/deleting/delete-and-cleanup.html:31
> +    confimedMarkup = node.innerHTML;

Typo here: "confimedMarkup".
Comment 3 Enrica Casucci 2011-11-30 17:12:37 PST
> Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?
No, it should be anchorNode. Old habit die hard :-)
> This should be:
> 
>     node = parentNode;
Done.
> > LayoutTests/editing/deleting/delete-and-cleanup.html:31
> > +    confimedMarkup = node.innerHTML;
> 
> Typo here: "confimedMarkup".

Fixed: it was wrong on both tests.
Comment 4 Ryosuke Niwa 2011-11-30 17:15:29 PST
Comment on attachment 117294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117294&action=review

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:752
>> +    Node* node = m_endingPosition.deprecatedNode();
> 
> Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

containerNode should be used here since if m_endingPosition was before or after a div, then we're outside of that div.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:763
>> +            if (node == m_endingPosition.deprecatedNode())
> 
> Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

This should be containerNode if we start our start from containerNode.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:767
>> +            node = m_endingPosition.deprecatedNode();
> 
> Do we have to use deprecatedNode? Can this be containerNode or anchorNode instead?

Ditto.
Comment 5 Enrica Casucci 2011-11-30 17:17:54 PST
Committed revision 101575.
Comment 6 Darin Adler 2011-11-30 18:28:40 PST
The patch you landed seems to still have one deprecatedNode.
Comment 7 Enrica Casucci 2011-11-30 23:28:47 PST
(In reply to comment #6)
> The patch you landed seems to still have one deprecatedNode.
I'll fix it.
Comment 8 Enrica Casucci 2011-12-02 10:57:49 PST
(In reply to comment #7)
> (In reply to comment #6)
> > The patch you landed seems to still have one deprecatedNode.
> I'll fix it.

Fixed in revision 101830.