Bug 62621 – Span ID duplicated when pressing enter at beginning of span

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

Put the following in a contenteditable div:

<span id="dupe">a</span>

Put the cursor before "a" and press enter. Result:

<span id="dupe"><div><span id="dupe"><br></span></div>a</span>

The id of the span should not be duplicated.

[Annie Sullivan]

Ryosuke, in comment #7 of bug 61594, you suggested that before fixing this issue, we should merge InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock() with CompositeEditCommand::cloneParagraphUnderNewElement() since they are very similar functions. I started looking into this, but several differences in functionality tripped me up:

1. The InsertParagraphSeparator version actually operates as two functions paired together: getAncestorsInsideBlock() collects the ancestors and then cloneHierarchyUnderNewBlock() clones them. There doesn't appear to be much reason for this, so it should be okay to change.

2. For listing the ancestors, getAncestorsInsideBlock takes an insertion Node and an outer block Element. It excludes both these nodes from the list of ancestors, only appending the elements between them. By contrast, cloneParagraphUnderNewElement() takes a start Position and an outer Node, and includes the position.deprecatedNode() and the outer node in the list of ancestors. Swapping between using positions and using nodes, counting ancestors differently, and keeping lists of Nodes instead of Elements is a bigger change in behavior. We also end up with another dependency on deprecatedNode.

3. cloneParagraphUnderNewElement() has no return value. cloneHierarchyUnderNewBlock() returns the deepest Element in the ancestor list that was cloned. We'd need to add this functionality into cloneParagraphUnderNewElement(), but it seems like an arbitrary return value to add.

Do you still think that merging these functions is the right thing to do? I am working on a patch to do this, but because of the differences in functionality I'm having a lot of trouble getting the arguments to cloneParagraphUnderNewElement correct. Each time I fix a layout test, a different layout test breaks with a slightly different problem.

[Ryosuke Niwa]

(In reply to comment #1)
> 1. The InsertParagraphSeparator version actually operates as two functions paired together: getAncestorsInsideBlock() collects the ancestors and then cloneHierarchyUnderNewBlock() clones them. There doesn't appear to be much reason for this, so it should be okay to change.

Just having one function that collects and clones ancestors should work.

> 2. For listing the ancestors, getAncestorsInsideBlock takes an insertion Node and an outer block Element. It excludes both these nodes from the list of ancestors, only appending the elements between them. By contrast, cloneParagraphUnderNewElement() takes a start Position and an outer Node, and includes the position.deprecatedNode() and the outer node in the list of ancestors. Swapping between using positions and using nodes, counting ancestors differently, and keeping lists of Nodes instead of 
Elements is a bigger change in behavior. We also end up with another dependency on deprecatedNode.

The merged function should take a starting Position and the outer block node.  We may need to get rid of deprecatedNode dependency in a separate bug.

> 3. cloneParagraphUnderNewElement() has no return value. cloneHierarchyUnderNewBlock() returns the deepest Element in the ancestor list that was cloned. We'd need to add this functionality into cloneParagraphUnderNewElement(), but it seems like an arbitrary return value to add.

I agree. I'm curious why we'd need that.

> Do you still think that merging these functions is the right thing to do? I am working on a patch to do this, but because of the differences in functionality I'm having a lot of trouble getting the arguments to cloneParagraphUnderNewElement correct. Each time I fix a layout test, a different layout test breaks with a slightly different problem.

I think we should eventually merge these two functions, but that isn't really a requirement for you to fix other bugs.

[Annie Sullivan]

(In reply to comment #2)
> > 2. For listing the ancestors, getAncestorsInsideBlock takes an insertion Node and an outer block Element. It excludes both these nodes from the list of ancestors, only appending the elements between them. By contrast, cloneParagraphUnderNewElement() takes a start Position and an outer Node, and includes the position.deprecatedNode() and the outer node in the list of ancestors. Swapping between using positions and using nodes, counting ancestors differently, and keeping lists of Nodes instead of 
> Elements is a bigger change in behavior. We also end up with another dependency on deprecatedNode.
> 
> The merged function should take a starting Position and the outer block node.  We may need to get rid of deprecatedNode dependency in a separate bug.

CompositeEditCommand::cloneParagraphUnderNewElement() also takes an end position (in case there are also sibling nodes to clone) and a block element (to clone the paragraph under). I think both of these are needed as well.

> > 3. cloneParagraphUnderNewElement() has no return value. cloneHierarchyUnderNewBlock() returns the deepest Element in the ancestor list that was cloned. We'd need to add this functionality into cloneParagraphUnderNewElement(), but it seems like an arbitrary return value to add.
> 
> I agree. I'm curious why we'd need that.

I think InsertParagraphSeparatorCommand is the only caller that would need it. The reason is that it inserts the actual line break by calling appendBlockPlaceholder() on that deepest cloned node. Maybe there is a better way to do this, like creating the block placeholder before cloning?

> > Do you still think that merging these functions is the right thing to do? I am working on a patch to do this, but because of the differences in functionality I'm having a lot of trouble getting the arguments to cloneParagraphUnderNewElement correct. Each time I fix a layout test, a different layout test breaks with a slightly different problem.
> 
> I think we should eventually merge these two functions, but that isn't really a requirement for you to fix other bugs.

The whole thing seems pretty fragile; the existing code calls deprecatedNode() in both places and it seems like we'd want to get rid of those before we do a bigger refactor. But I think just getting rid of IDs when cloning can be done in a much simpler change that doesn't require any refactoring.

[Annie Sullivan]

Created attachment 97191 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97191 [details]
Patch

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

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:139
> +        // Delete id attribute from the cloned child element because the same id cannot be used for more than one element

I don't think this comment is necessary.  It's self-evident.
More interesting question to ask is whether if we'd ever delete the original element after cloning hierarchy, in which case we shouldn't be removing id attribute.

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:141
> +        child->removeAttribute(HTMLNames::idAttr, ec);

Do you really need "HTMLNames::"?

[Annie Sullivan]

(In reply to comment #5)

Will create a patch to address the rest of the comments soon, wanted to answer this one right away in case I'm missing something:

> More interesting question to ask is whether if we'd ever delete the original element after cloning hierarchy, in which case we shouldn't be removing id attribute.

This function is called from two places in InsertParagraphCommand, and they both look like this:

        appendBlockPlaceholder(cloneHierarchyUnderNewBlock(ancestors, blockToInsert));

        // In this case, we need to set the new ending selection.
        setEndingSelection(VisibleSelection(insertionPosition, DOWNSTREAM));
        return;

Neither appendBlockPlaceholder nor setEndingSelection ever delete any nodes, so I think we're safe here.

[Ryosuke Niwa]

(In reply to comment #6)
> > More interesting question to ask is whether if we'd ever delete the original element after cloning hierarchy, in which case we shouldn't be removing id attribute.
> 
> This function is called from two places in InsertParagraphCommand, and they both look like this:
> 
>         appendBlockPlaceholder(cloneHierarchyUnderNewBlock(ancestors, blockToInsert));
> 
>         // In this case, we need to set the new ending selection.
>         setEndingSelection(VisibleSelection(insertionPosition, DOWNSTREAM));
>         return;
> 
> Neither appendBlockPlaceholder nor setEndingSelection ever delete any nodes, so I think we're safe here.

Right, and that'll be a useful comment to have.

[Annie Sullivan]

Created attachment 97197 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97197 [details]
Patch

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

> LayoutTests/editing/inserting/return-key-span-start-expected.txt:7
> +| <span>
> +|   id="dupe"
> +|   <div>
> +|     <span>
> +|       <br>

Wow!  Why are we nesting div inside span?  Can we fix this?

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:140
> +        // It should always be okay to delete the id of cloned elements, because the codepaths that call this method
> +        // only call appendBlockPlaceholder and setEndingSelection afterward, which never delete the original node.

I'm sad that there's no way to assert this.  Also this comment might be too verbose.  What matters is the fact we don't delete the cloned elements or the original elements, not the fact we call appendBlockPlaceholder and setEndingSelection.

[Ryosuke Niwa]

Comment on attachment 97197 [details]
Patch

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

> LayoutTests/editing/inserting/return-key-span-start-expected.txt:7
> +| <span>
> +|   id="dupe"
> +|   <div>
> +|     <span>
> +|       <br>

Wow!  Why are we nesting div inside span?  Can we fix this?

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:140
> +        // It should always be okay to delete the id of cloned elements, because the codepaths that call this method
> +        // only call appendBlockPlaceholder and setEndingSelection afterward, which never delete the original node.

I'm sad that there's no way to assert this.  Also this comment might be too verbose.  What matters is the fact we don't delete the cloned elements or the original elements, not the fact we call appendBlockPlaceholder and setEndingSelection.

[Annie Sullivan]

(In reply to comment #10)
> (From update of attachment 97197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97197&action=review
> 
> > LayoutTests/editing/inserting/return-key-span-start-expected.txt:7
> > +| <span>
> > +|   id="dupe"
> > +|   <div>
> > +|     <span>
> > +|       <br>
> 
> Wow!  Why are we nesting div inside span?  Can we fix this?

This is the current behavior, and my change hasn't affected it. If you look at InsertParagraphSeparatorCommand::doApply(), we have started with the following tree:

<div contenteditable>
  <span>
    hello

startBlock gets set to the contenteditable div. The code falls into the "// Handle case when position is in the first visible position in its block.." block that starts on line 250 of my patch. That code looks for a refNode to insert the new div before:

        if (isFirstInBlock && !nestNewBlock)
            refNode = startBlock;
        else if (insertionPosition.deprecatedNode() == startBlock && nestNewBlock) {
            refNode = startBlock->childNode(insertionPosition.deprecatedEditingOffset());
            ASSERT(refNode); // must be true or we'd be in the end of block case
        } else
            refNode = insertionPosition.deprecatedNode();

nestNewBlock is true and the insertion position is at the start of the text node (not the startBlock), so it falls through to the "refNode = insertionPosition.deprecatedNode();" line. So the div gets inserted right before the "hello" text, as a child of the span.

We know that if we've fallen into this code path, and isFirstInBlock and nestNewBlock are true, that the insertion position is in the first visible position in the block. So I think we want to modify the code in that case to always insert the new div as this first child of startBlock. Does that sound right? In this case, we'd end up with:

<div contenteditable>
  <div>
    <span>
      <br>
  <span>
    hello

It seems like something that should be done in a separate patch. I know my layout test result looks strange, but other than that it doesn't seem to have much to do with this patch. So does that fix need to block this one?

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> This is the current behavior, and my change hasn't affected it. If you look at InsertParagraphSeparatorCommand::doApply(), we have started with the following tree:

Oh sure, I didn't meant that your patch broke it.

> startBlock gets set to the contenteditable div. The code falls into the "// Handle case when position is in the first visible position in its block.." block that starts on line 250 of my patch. That code looks for a refNode to insert the new div before:
> 
>         if (isFirstInBlock && !nestNewBlock)
>             refNode = startBlock;
>         else if (insertionPosition.deprecatedNode() == startBlock && nestNewBlock) {
>             refNode = startBlock->childNode(insertionPosition.deprecatedEditingOffset());
>             ASSERT(refNode); // must be true or we'd be in the end of block case
>         } else
>             refNode = insertionPosition.deprecatedNode();
> 
> nestNewBlock is true and the insertion position is at the start of the text node (not the startBlock), so it falls through to the "refNode = insertionPosition.deprecatedNode();" line. So the div gets inserted right before the "hello" text, as a child of the span.
> 
> We know that if we've fallen into this code path, and isFirstInBlock and nestNewBlock are true, that the insertion position is in the first visible position in the block. So I think we want to modify the code in that case to always insert the new div as this first child of startBlock. Does that sound right? In this case, we'd end up with:

I see. There's a code in ReplaceSelectionCommand enrica recently added and I subsequently modified that deals with similar situation.  We may want to extract that up into CompositeEditCommand and share code if possible.

> It seems like something that should be done in a separate patch. I know my layout test result looks strange, but other than that it doesn't seem to have much to do with this patch. So does that fix need to block this one?

Fixing it in a separate patch sounds good to me.

[Ryosuke Niwa]

Comment on attachment 97197 [details]
Patch

I'd say r+ although you may want to consider revising the comment in cloneHierarchyUnderNewBlock.

[Ryosuke Niwa]

Let me know if you want to revise the comment or rather land the patch as is.  I'm fine either way.

[Annie Sullivan]

Created attachment 97208 [details]
Patch

[Annie Sullivan]

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

Added a shorter comment.

[Ryosuke Niwa]

Comment on attachment 97208 [details]
Patch

Thanks for revising the comment!

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=97208) [details] [details]
> > Patch
> 
> Added a shorter comment.

By the way, I think you can do webkit-patch --no-obsolete --commit-queue to leave r+ on old patch intact and ask for cq+ on new patch.

[WebKit Review Bot]

Comment on attachment 97208 [details]
Patch

Clearing flags on attachment: 97208

Committed r88890: <http://trac.webkit.org/changeset/88890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Added the test to Mac WebKit2's skipped list in http://trac.webkit.org/changeset/88902
since TestRunner doesn't support eventSender.keyDown.

[Ryosuke Niwa]

Ugh... I should have realized this during the review but you should have done document.execCommand('InsertParagraph', false, null) instead of eventSender.keyDown('\n').  Then this test would have worked on all ports.

Annie, could you post a patch to fix this?

[Annie Sullivan]

(In reply to comment #22)
> Ugh... I should have realized this during the review but you should have done document.execCommand('InsertParagraph', false, null) instead of eventSender.keyDown('\n').  Then this test would have worked on all ports.
> 
> Annie, could you post a patch to fix this?

Sure. I'll try to fix editing/inserting/return-key-in-hidden-field.html as well, which is what I used as an example. Thanks for skipping the test in the meantime!

[Ryosuke Niwa]

(In reply to comment #23)
> Sure. I'll try to fix editing/inserting/return-key-in-hidden-field.html as well, which is what I used as an example. Thanks for skipping the test in the meantime!

Great!  Thanks for the follow up.