Bug 25002 – Repeated copy/paste can lead to deeply nested divs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

Steps to reproduce:
1. Go to Midas demo: http://www.mozilla.org/editor/midasdemo/
2. Type the following:
a
b
c

3. Highlight "b" and press Indent button. Resulting HTML:
a<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;">b</blockquote><div>c</div>
4. Select everything and copy. Paste in newline after "c".
5. Now select pasted content and paste in newline at end.
6. Repeat step 5 a couple of times

Actual result:
Each paste ends up in a nested div. Resulting HTML (blockquote styles removed for readability):
a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div><div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div><div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div><div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div></div></div></div>

Expected result:
No extra divs should be added:
a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div>a<blockquote class="webkit-indent-blockquote">b</blockquote><div>c</div>

[Tony Chang]

Created attachment 47813 [details]
Patch

[Tony Chang]

(In reply to comment #1)
> Created an attachment (id=47813) [details]
> Patch

When inserting a new line, we grab the last block node and clone it as a sibling. For example, in the original bug report, the last block is the <div> around "c" and it gets cloned as a sibling of c.  This causes a problem in this test case because the pasted content includes a <div> at the end.  When we create a new line after a paste, we are now one level deeper than before.

For tags other than divs, this is what we want because we want to keep our indention level.  For example, for blockquote tags, when you insert a newline, you want to stay at the same depth as before (i.e., you want nested blockquotes).  The same is true for lists.

The patch just special cases nested divs and breaks out of them when inserting a new line.

[Enrica Casucci]

Comment on attachment 47813 [details]
Patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index b3b56b3..7a80056 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,25 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=25002
> +        When inserting a new paragraph, avoid nesting empty divs.  When
> +        pasting near the end of a paragraph, this prevents each paste
> +        command for getting nested one level deeper.
> +
> +        Three paste tests have been rebaselined since this causes the pasted
> +        content to be outside the last div instead of inside.  E.g.,
> +        <div>foo<div>bar</div>[pasted content]</div> is now
> +        <div>foo<div>bar</div></div><div>[pasted content]</div>
> +
> +        The new test verifies this behavior.
> +
> +        * editing/inserting/paragraph-outside-nested-divs-expected.txt: Added.
> +        * editing/inserting/paragraph-outside-nested-divs.html: Added.
> +        * platform/mac/editing/pasteboard/paste-text-012-expected.txt:
> +        * platform/mac/editing/pasteboard/paste-text-013-expected.txt:
> +        * platform/mac/editing/pasteboard/paste-text-017-expected.txt:
> +
>  2010-01-31  Kent Tamura  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> diff --git a/LayoutTests/editing/inserting/paragraph-outside-nested-divs-expected.txt b/LayoutTests/editing/inserting/paragraph-outside-nested-divs-expected.txt
> new file mode 100644
> index 0000000..7f1d2d5
> --- /dev/null
> +++ b/LayoutTests/editing/inserting/paragraph-outside-nested-divs-expected.txt
> @@ -0,0 +1,9 @@
> +When inserting a new line, we should break out of nested divs.
> +
> +SUCCESS
> +
> +first
> +second
> +This should be in nested divs.
> +third
> +This should be in a single div.
> diff --git a/LayoutTests/editing/inserting/paragraph-outside-nested-divs.html b/LayoutTests/editing/inserting/paragraph-outside-nested-divs.html
> new file mode 100644
> index 0000000..c366b6d
> --- /dev/null
> +++ b/LayoutTests/editing/inserting/paragraph-outside-nested-divs.html
> @@ -0,0 +1,45 @@
> +<body contentEditable="true">
> +<p>When inserting a new line, we should break out of nested divs.</p>
> +<p id="results"><p>
> +<div>first<div>second</div><div>third</div></div>
> +</body>
> +<script src="../editing.js"></script>
> +<script>
> +    if (window.layoutTestController)
> +        window.layoutTestController.dumpAsText();
> +
> +    function fail(msg) {
> +        document.getElementById("results").innerText = "FAIL";
> +        throw msg;
> +    }
> +
> +    // Try inserting a new line after the last div.
> +    var div2 = document.getElementsByTagName("div")[1];
> +    var div3 = document.getElementsByTagName("div")[2];
> +    execSetSelectionCommand(div3, 0, div3, 0);
> +    execMoveSelectionForwardByWordCommand();
> +
> +    execTypeCharacterCommand("\n");
> +    execInsertHTMLCommand("This should be in a single div.");
> +
> +    var div4 = document.getElementsByTagName("div")[3];
> +    if (div4.innerHTML != "This should be in a single div.")
> +        fail("wrong div4? " + div4.innerHTML);
> +    if (div4.parentNode.tagName != "BODY")
> +        fail("div should not be nested: " + div4.parentNode.tagName);
> +
> +    // Try inserting a new line after the second div.  This should be nested.
> +    execSetSelectionCommand(div2, 0, div2, 0);
> +    execMoveSelectionForwardByWordCommand();
> +
> +    execTypeCharacterCommand("\n");
> +    execInsertHTMLCommand("This should be in nested divs.");
> +
> +    var nestedDiv = document.getElementsByTagName("div")[2];
> +    if (nestedDiv.innerHTML != "This should be in nested divs.")
> +        fail("wrong nestedDiv? " + nestedDiv.innerHTML);
> +    if (nestedDiv.parentNode.tagName != "DIV")
> +        fail("div should be nested: " + nestedDiv.parentNode.tagName);
> +
> +    document.getElementById("results").innerText = "SUCCESS";
> +</script>
> diff --git a/LayoutTests/platform/mac/editing/pasteboard/paste-text-012-expected.txt b/LayoutTests/platform/mac/editing/pasteboard/paste-text-012-expected.txt
> index 429e300..7916c24 100644
> --- a/LayoutTests/platform/mac/editing/pasteboard/paste-text-012-expected.txt
> +++ b/LayoutTests/platform/mac/editing/pasteboard/paste-text-012-expected.txt
> @@ -7,7 +7,7 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: shouldInsertNode:#document-fragment replacingDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document givenAction:WebViewInsertActionPasted
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > DIV > DIV > SPAN > DIV > BODY > HTML > #document to 0 of DIV > DIV > DIV > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > DIV > SPAN > DIV > BODY > HTML > #document to 0 of DIV > DIV > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  layer at (0,0) size 800x600
> @@ -36,12 +36,12 @@ layer at (0,0) size 800x600
>            RenderInline {SPAN} at (0,0) size 0x0
>          RenderBlock (anonymous) at (14,14) size 756x132
>            RenderBlock {DIV} at (0,0) size 756x132 [border: (2px solid #FF0000)]
> -            RenderBlock {DIV} at (14,38) size 728x80
> +            RenderBlock {DIV} at (14,38) size 728x28
>                RenderBlock {BLOCKQUOTE} at (40,0) size 648x28
>                  RenderText {#text} at (0,0) size 32x28
>                    text run at (0,0) width 32: "foo"
> -              RenderBlock {DIV} at (0,52) size 728x28
> -                RenderBR {BR} at (0,0) size 0x28
> +            RenderBlock {DIV} at (14,90) size 728x28
> +              RenderBR {BR} at (0,0) size 0x28
>          RenderBlock (anonymous) at (14,146) size 756x0
>            RenderInline {SPAN} at (0,0) size 0x0
> -caret: position 0 of child 0 {BR} of child 1 {DIV} of child 0 {DIV} of child 0 {DIV} of child 0 {SPAN} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> +caret: position 0 of child 0 {BR} of child 1 {DIV} of child 0 {DIV} of child 0 {SPAN} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> diff --git a/LayoutTests/platform/mac/editing/pasteboard/paste-text-013-expected.txt b/LayoutTests/platform/mac/editing/pasteboard/paste-text-013-expected.txt
> index 3c1bbda..b0bd9e1 100644
> --- a/LayoutTests/platform/mac/editing/pasteboard/paste-text-013-expected.txt
> +++ b/LayoutTests/platform/mac/editing/pasteboard/paste-text-013-expected.txt
> @@ -10,7 +10,7 @@ EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  EDITING DELEGATE: shouldInsertNode:#document-fragment replacingDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document givenAction:WebViewInsertActionPasted
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  layer at (0,0) size 800x600
> @@ -40,10 +40,10 @@ layer at (0,0) size 800x600
>          RenderText {#text} at (14,14) size 12x28
>            text run at (14,14) width 12: "x"
>        RenderBlock {DIV} at (0,238) size 784x132 [border: (2px solid #FF0000)]
> -        RenderBlock {DIV} at (14,38) size 756x80
> +        RenderBlock {DIV} at (14,38) size 756x28
>            RenderBlock {BLOCKQUOTE} at (40,0) size 676x28
>              RenderText {#text} at (0,0) size 32x28
>                text run at (0,0) width 32: "foo"
> -          RenderBlock {DIV} at (0,52) size 756x28
> -            RenderBR {BR} at (0,0) size 0x28
> -caret: position 0 of child 0 {BR} of child 1 {DIV} of child 0 {DIV} of child 8 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> +        RenderBlock {DIV} at (14,90) size 756x28
> +          RenderBR {BR} at (0,0) size 0x28
> +caret: position 0 of child 0 {BR} of child 1 {DIV} of child 8 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> diff --git a/LayoutTests/platform/mac/editing/pasteboard/paste-text-017-expected.txt b/LayoutTests/platform/mac/editing/pasteboard/paste-text-017-expected.txt
> index b1fff1c..af6d2a0 100644
> --- a/LayoutTests/platform/mac/editing/pasteboard/paste-text-017-expected.txt
> +++ b/LayoutTests/platform/mac/editing/pasteboard/paste-text-017-expected.txt
> @@ -6,7 +6,7 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: shouldInsertNode:#document-fragment replacingDOMRange:range from 5 of #text > DIV > DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > DIV > BODY > HTML > #document givenAction:WebViewInsertActionPasted
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > DIV > DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  layer at (0,0) size 800x600
> @@ -40,14 +40,14 @@ layer at (0,0) size 800x600
>                text run at (0,0) width 35: "one"
>            RenderBlock {DIV} at (2,30) size 780x28
>              RenderBR {BR} at (0,0) size 0x28
> -          RenderBlock {DIV} at (2,58) size 780x56
> +          RenderBlock {DIV} at (2,58) size 780x28
>              RenderBlock {DIV} at (0,0) size 780x28
>                RenderText {#text} at (0,0) size 36x28
>                  text run at (0,0) width 36: "two"
>              RenderBlock (anonymous) at (0,28) size 780x0
> -            RenderBlock {DIV} at (0,28) size 780x28
> -              RenderBR {BR} at (0,0) size 0x28
> +          RenderBlock {DIV} at (2,86) size 780x28
> +            RenderBR {BR} at (0,0) size 0x28
>            RenderBlock {DIV} at (2,114) size 780x28
>              RenderText {#text} at (0,0) size 49x28
>                text run at (0,0) width 49: "three"
> -caret: position 0 of child 0 {BR} of child 1 {DIV} of child 5 {DIV} of child 1 {DIV} of child 3 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> +caret: position 0 of child 0 {BR} of child 6 {DIV} of child 1 {DIV} of child 3 {DIV} of child 1 {BODY} of child 0 {HTML} of document
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 240f69c..d7d1e72 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,18 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=25002
> +        When inserting a new paragraph, avoid nesting empty divs.  When
> +        pasting near the end of a paragraph, this prevents each paste
> +        command for getting nested one level deeper.
> +
> +        Test: editing/inserting/paragraph-outside-nested-divs.html
> +
> +        * editing/InsertParagraphSeparatorCommand.cpp:
> +        (WebCore::findInsertionSibling):
> +        (WebCore::InsertParagraphSeparatorCommand::doApply):
> +
>  2010-01-31  Kent Tamura  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> diff --git a/WebCore/editing/InsertParagraphSeparatorCommand.cpp b/WebCore/editing/InsertParagraphSeparatorCommand.cpp
> index 695f46a..75b85a3 100644
> --- a/WebCore/editing/InsertParagraphSeparatorCommand.cpp
> +++ b/WebCore/editing/InsertParagraphSeparatorCommand.cpp
> @@ -44,6 +44,25 @@ namespace WebCore {
>  
>  using namespace HTMLNames;
>  
> +// When inserting a new line, we want to avoid nesting empty divs if we can.  Otherwise, when
> +// pasting, it's easy to have each new line be a div deeper than the previous.  E.g., in the case
> +// below, we want to insert at ^ instead of |.
> +// <div>foo<div>bar</div>|</div>^
> +static Element* findInsertionSibling(Element* startBlock, const Element* blockToInsert)
> +{
> +    if (!blockToInsert->hasTagName(divTag))
> +        return startBlock;
> +
> +    Element* curBlock = startBlock;
> +    while (!curBlock->nextSibling() && curBlock->parentElement()->hasTagName(divTag)) {
> +        NamedNodeMap* attributes = curBlock->parentElement()->attributes(true);
> +        if (attributes && !attributes->isEmpty())
> +            break;
> +        curBlock = curBlock->parentElement();
> +    }
> +    return curBlock;
> +}
> +
>  InsertParagraphSeparatorCommand::InsertParagraphSeparatorCommand(Document *document, bool mustUseDefaultParagraphElement) 
>      : CompositeEditCommand(document)
>      , m_mustUseDefaultParagraphElement(mustUseDefaultParagraphElement)
> @@ -214,7 +233,11 @@ void InsertParagraphSeparatorCommand::doApply()
>                  // When inserting the newline after the blockquote, we don't want to apply the original style after the insertion
>                  shouldApplyStyleAfterInsertion = false;
>              }
> -            insertNodeAfter(blockToInsert, startBlock);
> +
> +            // Most of the time we want to stay at the nesting level of the startBlock (e.g., when nesting within lists).  However,
> +            // for div nodes, this can result in nested div tags that are hard to break out of.
> +            Element* siblingNode = findInsertionSibling(startBlock, blockToInsert.get());
> +            insertNodeAfter(blockToInsert, siblingNode);
>          }
>  
>          // Recreate the same structure in the new paragraph.

The change looks ok to me. I don't really like the name of the new method. You should come up with something more descriptive.
What you are looking for is a visually equivalent position that avoids the nesting.

[Tony Chang]

Created attachment 47890 [details]
Patch

[Eric Seidel (no email)]

Attachment 47890 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/230114

[Tony Chang]

Comment on attachment 47890 [details]
Patch

sorry, fixing compile errors.

[Tony Chang]

Created attachment 47892 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 47892 [details]
Patch

Not your fault, but I hate it when code uses bools when we should be using Enums:
 55         NamedNodeMap* attributes = curBlock->parentElement()->attributes(true);

I have no idea what that does w/o looking at the original function.

Turns out it means "readonly":
    NamedNodeMap* attributes(bool readonly) const;

But then that makes me wonder what plain old "attributes()" returns.  Bah!

Your change itself looks fine.  Looks like it does what it says it does, and the test looks functional.

[WebKit Commit Bot]

Comment on attachment 47892 [details]
Patch

Clearing flags on attachment: 47892

Committed r54395: <http://trac.webkit.org/changeset/54395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.