Bug 73497 – Copy/paste of the same content produces increasingly nested markup

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Enrica Casucci]

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>

[Enrica Casucci]

Created attachment 117294 [details]
Patch

[Darin Adler]

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".

[Enrica Casucci]

> 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.

[Ryosuke Niwa]

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.

[Enrica Casucci]

Committed revision 101575.

[Darin Adler]

The patch you landed seems to still have one deprecatedNode.

[Enrica Casucci]

(In reply to comment #6)
> The patch you landed seems to still have one deprecatedNode.
I'll fix it.

[Enrica Casucci]

(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.