An extra line break is inserted when pasting into a font element

• Component: HTML Editing
• Status: RESOLVED FIXED

[equinux AG]

Created attachment 113020 [details]
Sample file.

Summary:

WebKit editing: Under certain circumstances an additional linebreak is inserted when pasting text from the pasteboard.


Steps to Reproduce:

1. Open the attached document in Safari.
2. Select any of the words in red.
3. Choose Edit > Copy.
4. Place the insertion point anywhere within the red text.
5. Choose Edit > Paste.


Expected Results:

The copied word is inserted at insertion point.


Actual Results:

An additional linebreak is inserted after the word.


Regression:

Reproducible with latest WebKit nightly build (r98835).


Notes:

A similar bug (#71018) has recently been fixed.

[Ryosuke Niwa]

I wouldn't call this a regression. The behavior on Safari 5.1 is far worse (it inserts the pasted content into the previous line AND inserts a line break at where the caret was.

[Ryosuke Niwa]

Confirmed on nightly build.

[Yi Shen]

Created attachment 133329 [details]
First try

[Ryosuke Niwa]

Comment on attachment 133329 [details]
First try

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:116
> +static Position nextPositionInContainerNode(Position pos)

Please don't use abbreviations like pos.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:120
> +    Node* n = pos.deprecatedNode();    

You should use pos.containerNode(). Also please don't use one-letter variable like n.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
> +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);

Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.

[Yi Shen]

Thanks for the reviewing.
(In reply to comment #4)
> (From update of attachment 133329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133329&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:116
> > +static Position nextPositionInContainerNode(Position pos)
> 
> Please don't use abbreviations like pos.
Will fix it.
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:120
> > +    Node* n = pos.deprecatedNode();    
> 
> You should use pos.containerNode(). Also please don't use one-letter variable like n.
Will fix it.
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
> > +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);
> 
> Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.

The insertion position only move up if the visible positions of the current position and the high level node's position are the same. So, if we had a bunch of siblings after pos's container node, they won't get skipped because of the difference visible positions.

[Ryosuke Niwa]

Comment on attachment 133329 [details]
First try

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

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
>>> +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);
>> 
>> Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.
> 
> The insertion position only move up if the visible positions of the current position and the high level node's position are the same. So, if we had a bunch of siblings after pos's container node, they won't get skipped because of the difference visible positions.

Right, but what if they're empty? If that case, I think we do want to skip those empty elements but moving to the next position in the parent may not be possible due to it being a different visible position.

[Yi Shen]

(In reply to comment #6)
> (From update of attachment 133329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133329&action=review
> 
> >>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
> >>> +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);
> >> 
> >> Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.
> > 
> > The insertion position only move up if the visible positions of the current position and the high level node's position are the same. So, if we had a bunch of siblings after pos's container node, they won't get skipped because of the difference visible positions.
> 
> Right, but what if they're empty? If that case, I think we do want to skip those empty elements but moving to the next position in the parent may not be possible due to it being a different visible position.

Correct me if I am wrong :) In the context, the empty elements means they don't occupy any space, right? If so, all the empty sibling elements would have the same visible position as the next position in the parent. For example,
<div><u>abc^<u></u><u></u></u></div>, "^" is the insertion position. After calling the positionAvoidingPrecedingNodes(), it should be <div><u>abc<u></u><u></u></u>^</div>

[Ryosuke Niwa]

Comment on attachment 133329 [details]
First try

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

>>>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
>>>>> +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);
>>>> 
>>>> Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.
>>> 
>>> The insertion position only move up if the visible positions of the current position and the high level node's position are the same. So, if we had a bunch of siblings after pos's container node, they won't get skipped because of the difference visible positions.
>> 
>> Right, but what if they're empty? If that case, I think we do want to skip those empty elements but moving to the next position in the parent may not be possible due to it being a different visible position.
> 
> Correct me if I am wrong :) In the context, the empty elements means they don't occupy any space, right? If so, all the empty sibling elements would have the same visible position as the next position in the parent. For example,
> <div><u>abc^<u></u><u></u></u></div>, "^" is the insertion position. After calling the positionAvoidingPrecedingNodes(), it should be <div><u>abc<u></u><u></u></u>^</div>

Yes, but your code doesn't do that at the moment because you always retrieve the parent of the deprecated node. i.e. the first call to nextPositionInContainerNode would move the position to be anchored at div, then the next call will try to move out of div.

In general, we shouldn't try to out-smart what Position does. Position.next/previous and all other editing code has been well-tested and deals with all sorts of edge-cases. Adding completely different algorithm to deal with this particular situation is extremely undesirable.

[Yi Shen]

(In reply to comment #8)
> (From update of attachment 133329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133329&action=review
> 
> >>>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:125
> >>>>> +    return createLegacyEditingPosition(parent, n->nodeIndex() + 1);
> >>>> 
> >>>> Why do we always want to move up in the node hierarchy? If we had a bunch of siblings after pos's container node, then we'll still want to skip those. r- because I don't think this is a correct fix.
> >>> 
> >>> The insertion position only move up if the visible positions of the current position and the high level node's position are the same. So, if we had a bunch of siblings after pos's container node, they won't get skipped because of the difference visible positions.
> >> 
> >> Right, but what if they're empty? If that case, I think we do want to skip those empty elements but moving to the next position in the parent may not be possible due to it being a different visible position.
> > 
> > Correct me if I am wrong :) In the context, the empty elements means they don't occupy any space, right? If so, all the empty sibling elements would have the same visible position as the next position in the parent. For example,
> > <div><u>abc^<u></u><u></u></u></div>, "^" is the insertion position. After calling the positionAvoidingPrecedingNodes(), it should be <div><u>abc<u></u><u></u></u>^</div>
> 
> Yes, but your code doesn't do that at the moment because you always retrieve the parent of the deprecated node. i.e. the first call to nextPositionInContainerNode would move the position to be anchored at div, then the next call will try to move out of div.

I think my code does move the position to be anchored at div. In above case, let's say the start insertion position is (#text, 3), first call to nextPositionInContainerNode returns the position (#u, 1) and then the position moves to (#u, 1). The second call to nextPositionInContainerNode returns the position (#div 1) and move the position to be anchored at div. However, it wouldn't move the position out of div because we always check the position node  with the enclosingBlockFlowElement to make sure the position stay at the same block.

> 
> In general, we shouldn't try to out-smart what Position does. Position.next/previous and all other editing code has been well-tested and deals with all sorts of edge-cases. Adding completely different algorithm to deal with this particular situation is extremely undesirable.

The reason why I don't use Position.next here because it may return the child node's position and then moves the insertion position into the child node.
e.g without my changes, <div><u><u>abc^</u><u>efg</u></u></div> ==> <div><u><u>abc</u><u>^efg</u></u></div>

[Ryosuke Niwa]

(In reply to comment #9)
> > In general, we shouldn't try to out-smart what Position does. Position.next/previous and all other editing code has been well-tested and deals with all sorts of edge-cases. Adding completely different algorithm to deal with this particular situation is extremely undesirable.
> 
> The reason why I don't use Position.next here because it may return the child node's position and then moves the insertion position into the child node.
> e.g without my changes, <div><u><u>abc^</u><u>efg</u></u></div> ==> <div><u><u>abc</u><u>^efg</u></u></div>

What you need here is to use/make a function like nodeToSplitToAvoidPastingIntoInlineNodesWithStyle that finds the position outside of the inline elements.

[Yi Shen]

(In reply to comment #10)
> (In reply to comment #9)
> > > In general, we shouldn't try to out-smart what Position does. Position.next/previous and all other editing code has been well-tested and deals with all sorts of edge-cases. Adding completely different algorithm to deal with this particular situation is extremely undesirable.
> > 
> > The reason why I don't use Position.next here because it may return the child node's position and then moves the insertion position into the child node.
> > e.g without my changes, <div><u><u>abc^</u><u>efg</u></u></div> ==> <div><u><u>abc</u><u>^efg</u></u></div>
> 
> What you need here is to use/make a function like nodeToSplitToAvoidPastingIntoInlineNodesWithStyle that finds the position outside of the inline elements.

Thanks, will look at it and provide a new patch :)

[Yi Shen]

Created attachment 133563 [details]
updated patch based on Niwa's review

[Ryosuke Niwa]

Anyway, my preference is to do something along the line of:

Index: Source/WebCore/editing/ReplaceSelectionCommand.cpp
===================================================================
--- Source/WebCore/editing/ReplaceSelectionCommand.cpp	(revision 111896)
+++ Source/WebCore/editing/ReplaceSelectionCommand.cpp	(working copy)
@@ -121,9 +121,14 @@
     // same.  E.g.,
     //   <div>foo^</div>^
     // The two positions above are the same visual position, but we want to stay in the same block.
-    Node* stopNode = pos.deprecatedNode()->enclosingBlockFlowElement();
-    while (stopNode != pos.deprecatedNode() && VisiblePosition(pos) == VisiblePosition(pos.next()))
-        pos = pos.next();
+    Node* enclosingBlock = pos.containerNode()->enclosingBlockFlowElement();
+    for (Position nextPosition = pos; ;pos = nextPosition) {
+        nextPosition = pos.next();
+        if (nextPosition == pos
+            || nextPosition.containerNode()->enclosingBlockFlowElement() != enclosingBlock
+            || VisiblePosition(pos) != VisiblePosition(nextPosition))
+            break;
+    }
     return pos;
 }

Note that a couple of tests fail with this change along, and there's about a dozen test to be rebaselined.

[Yi Shen]

Thanks for the follow up :) But I have two concerns about your code,

1) Since the Position::next() is still used here, it is possible to move the insertion position into sibling node's child, which seems incorrect (that's why my patch doesn't use position::next to avoid it). e.g
<div><u><u>text underline<u>^<u>test underline</u></u></div>
==>
<div><u><u>text underline<u><u>^test underline</u></u></div>

2) It is possible to move the insertion position to the end of the enclosingBlock. That's why some tests failed.
e.g.
<div><u>abc^</u>#text "\n"<div> ==> <div><u>abc^</u>#text "\n"<div>^
This can be fixed by changing the for statement to 
for (Position nextPosition = pos; nextPosition.containerNode() != enclosingBlock; pos = nextPosition) {
 ...
}

(In reply to comment #13)
> Anyway, my preference is to do something along the line of:
> 
> Index: Source/WebCore/editing/ReplaceSelectionCommand.cpp
> ===================================================================
> --- Source/WebCore/editing/ReplaceSelectionCommand.cpp    (revision 111896)
> +++ Source/WebCore/editing/ReplaceSelectionCommand.cpp    (working copy)
> @@ -121,9 +121,14 @@
>      // same.  E.g.,
>      //   <div>foo^</div>^
>      // The two positions above are the same visual position, but we want to stay in the same block.
> -    Node* stopNode = pos.deprecatedNode()->enclosingBlockFlowElement();
> -    while (stopNode != pos.deprecatedNode() && VisiblePosition(pos) == VisiblePosition(pos.next()))
> -        pos = pos.next();
> +    Node* enclosingBlock = pos.containerNode()->enclosingBlockFlowElement();
> +    for (Position nextPosition = pos; ;pos = nextPosition) {
> +        nextPosition = pos.next();
> +        if (nextPosition == pos
> +            || nextPosition.containerNode()->enclosingBlockFlowElement() != enclosingBlock
> +            || VisiblePosition(pos) != VisiblePosition(nextPosition))
> +            break;
> +    }
>      return pos;
>  }
> 
> Note that a couple of tests fail with this change along, and there's about a dozen test to be rebaselined.

[Yi Shen]

Created attachment 134055 [details]
updated patch without using a help function

[Ryosuke Niwa]

Comment on attachment 134055 [details]
updated patch without using a help function

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:127
> +        if (pos.containerNode()->nonShadowBoundaryParentNode())
> +            nextPosition = positionInParentAfterNode(pos.containerNode());

Again, this is incorrect. You need to skip siblings of pos.containerNode() first.
I'd say that any solution that involves calling positionInParentAfterNode won't be correct.
On my second thought, we should be able to replace this enture function by pos.downstream().

[Yi Shen]

Well, I think I misunderstood your comments #6 & #8. 

Regarding the pos.downstream(), I have tried it on my code and seems it doesn't work. Also, you can see it by checking Tony's comments (#10) in bug Bug 21840.

Now my question is why we need to skip the siblings of pos.containerNode()? Considering following case,

e.g.   <div><u>abc^</u>/n</div>

if we skip all the siblings, then the new insertion position is after the "/n", which seems incorrect for me.

(In reply to comment #16)
> (From update of attachment 134055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134055&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:127
> > +        if (pos.containerNode()->nonShadowBoundaryParentNode())
> > +            nextPosition = positionInParentAfterNode(pos.containerNode());
> 
> Again, this is incorrect. You need to skip siblings of pos.containerNode() first.
> I'd say that any solution that involves calling positionInParentAfterNode won't be correct.
> On my second thought, we should be able to replace this enture function by pos.downstream().

[Ryosuke Niwa]

Comment on attachment 134055 [details]
updated patch without using a help function

Okay, I thought about this for a while but I couldn't come up with a better solution. So let's land this patch as is. We'll need to refactor ReplaceSelectionCommand in the future anyway so let us think through by then.

Thanks for the patch and explanations.

[WebKit Review Bot]

Comment on attachment 134055 [details]
updated patch without using a help function

Rejecting attachment 134055 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://queues.webkit.org/results/12146860

[WebKit Review Bot]

Comment on attachment 134055 [details]
updated patch without using a help function

Rejecting attachment 134055 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://queues.webkit.org/results/12147943

[Ryosuke Niwa]

Comment on attachment 134055 [details]
updated patch without using a help function

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

> Source/WebCore/ChangeLog:1
> +02012-03-27  Yi Shen  <[e-mail]>,  Ryosuke Niwa  <[e-mail]>

Oops, you need to fix this line :( Please delete my name.

> LayoutTests/editing/inserting/insert-text-into-font.html:1
> +<body contentEditable="true">

No DOCTYPE?

> LayoutTests/editing/inserting/insert-text-into-font.html:39
> +    // Inserting HTML to replace the "line" in the "Second line" should not insert an extra div (line break).
> +    var targetDiv = document.getElementById("secondDiv");
> +    var targetText = targetDiv.firstChild;
> +    execSetSelectionCommand(targetText, 8, targetText, targetText.textContent.length);
> +    document.execCommand("inserthtml", false, "<span id='red' style='color:red'>line</span>");
> +
> +    // Verify that the font still only has one div.
> +    var font = document.getElementById("fonts");
> +    var divs = font.querySelectorAll("div");
> +    if (divs.length != 1)
> +        fail("An extra div is inserted which is incorrect. There are " + divs.length + " divs inside font element.");
> +
> +    // Inserting HTML into "Second line" should not insert an extra div (line break).
> +    execSetSelectionCommand(targetText, 8, targetText, 8);
> +    document.execCommand("inserthtml", false, "<span id='green' style='color:green'>green</span>");
> +
> +    // Verify that the font still only has one div.
> +    var font = document.getElementById("fonts");
> +    var divs = font.querySelectorAll("div");
> +    if (divs.length != 1)
> +        fail("An extra div is inserted which is incorrect. There are " + divs.length + " divs inside font element.");

It's probably better to use dump-as-markup.js here.

[Yi Shen]

Committed r112399: <http://trac.webkit.org/changeset/112399>

[Yi Shen]

Thanks for the review :)

(In reply to comment #21)
> (From update of attachment 134055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134055&action=review
> 
> > Source/WebCore/ChangeLog:1
> > +02012-03-27  Yi Shen  <[e-mail]>,  Ryosuke Niwa  <[e-mail]>
> 
> Oops, you need to fix this line :( Please delete my name.
> 
> > LayoutTests/editing/inserting/insert-text-into-font.html:1
> > +<body contentEditable="true">
> 
> No DOCTYPE?
> 
> > LayoutTests/editing/inserting/insert-text-into-font.html:39
> > +    // Inserting HTML to replace the "line" in the "Second line" should not insert an extra div (line break).
> > +    var targetDiv = document.getElementById("secondDiv");
> > +    var targetText = targetDiv.firstChild;
> > +    execSetSelectionCommand(targetText, 8, targetText, targetText.textContent.length);
> > +    document.execCommand("inserthtml", false, "<span id='red' style='color:red'>line</span>");
> > +
> > +    // Verify that the font still only has one div.
> > +    var font = document.getElementById("fonts");
> > +    var divs = font.querySelectorAll("div");
> > +    if (divs.length != 1)
> > +        fail("An extra div is inserted which is incorrect. There are " + divs.length + " divs inside font element.");
> > +
> > +    // Inserting HTML into "Second line" should not insert an extra div (line break).
> > +    execSetSelectionCommand(targetText, 8, targetText, 8);
> > +    document.execCommand("inserthtml", false, "<span id='green' style='color:green'>green</span>");
> > +
> > +    // Verify that the font still only has one div.
> > +    var font = document.getElementById("fonts");
> > +    var divs = font.querySelectorAll("div");
> > +    if (divs.length != 1)
> > +        fail("An extra div is inserted which is incorrect. There are " + divs.length + " divs inside font element.");
> 
> It's probably better to use dump-as-markup.js here.

[Simon Fraser (smfr)]

Comment on attachment 133563 [details]
updated patch based on Niwa's review

I assume this patch is obsolete since the bug is RESOLVED