Bug 61594 – REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span

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

[Adele Peterson]

REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span

I'm not sure when this regressed, but it works fine in Safari 5.

[Adele Peterson]

Created attachment 95097 [details]
test case

[Annie Sullivan]

It looks like this regressed in ChangeSet 83247.

The original editing area contains <span>textnode1<br>textnode2</span>. Previously, a new div was created as a sibling to the span, a new span appended as a child of the new div, and the <br> and textnode2 were moved inside the new span. Now the div is created, but no new span is added and the <br> and textnode2 aren't moved.

Two things changed:

1. The calls to getAncestorsInsideBlock() on line 305 and cloneHierarchyUnderNewBlock() on line 340 were removed, which prevents the new span from being added into the new div.

2. The loop on lines 345-356 was changed so that n is set to the newly inserted div. Previously, it was set to the <br> after the cursor, so that the loop on lines 358-363 copied the <br> and the second text node to the new <div>. Now that loop does nothing.

[Ryosuke Niwa]

This is a bug in splitTreeToNode.  In the following DOM tree, splitTreeToNode doesn't split span because it thinks we're splitting the tree at the beginning of the first text node.

BODY	0x116a3f370
*	#text	0x116a3f660 "\n    "
	SPAN	0x116a3f6c0
		#text	0x116a3f890 "Hit Enter after this line"
		BR	0x116a3f970
		#text	0x116a3fb70 "See the cursor appear at the end of this line"
	#text	0x116a3fc50 "\n\n"
	DIV	0x113408bf0

We'd have to modify splitTreeToNode to take a Position instead to handle this case in general.

[Annie Sullivan]

Created attachment 96004 [details]
Work in Progress

[Annie Sullivan]

Some notes about this patch:

I wasn't sure whether to implement a new splitTreeToPosition method, or modify splitTreeToNode to take either. Please let me know what you think.

splitTreeToPosition currently doesn't handle splitting nodes if the position is not at the start or end of the node. Currently InsertParagraphSeparatorCommand is the only caller, and it never creates a case like that because it splits text nodes before calling splitTreeTo(Node|Position). I worry that callers may expect the method to split a node if the position is in the middle, but I don't want to add code that's not used or tested. What do you think? If you think I should add the code, I could try to use window.internal to add a test for it.

I was pretty surprised at the results for the layout test when the cursor is at the start of the text node. It turns out that this is handled by a different codepath in InsertParagraphSeparatorCommand. I get the same results in Safari 5. The reason that the id is duplicated for the span "wrapper" is that InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock() doesn't avoid duplicating ids, as SplitElementCommand::executeApply() does. Should I file a separate bug/write a separate patch for that?

[Ryosuke Niwa]

Comment on attachment 96004 [details]
Work in Progress

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1250
> +        // If the position is at the end of the node, split at the next node.
> +        if ((p.containerNode()->isTextNode() && p.offsetInContainerNode() == (int)static_cast<Text*>(p.containerNode())->length())
> +            || (p.containerNode()->isElementNode() && p.offsetInContainerNode() == (int)splitNode->childNodes()->length()))
> +            splitNode = splitNode->nextSibling();

This logic is likely wrong as it doesn't take invisible nodes into account.

[Ryosuke Niwa]

Thanks for working on this bug.  I'm excited to see a progress.

(In reply to comment #5)
> I wasn't sure whether to implement a new splitTreeToPosition method, or modify splitTreeToNode to take either. Please let me know what you think.

We should modify splitTreeToNode to take a Position instead of adding a new function.

> splitTreeToPosition currently doesn't handle splitting nodes if the position is not at the start or end of the node. Currently InsertParagraphSeparatorCommand is the only caller, and it never creates a case like that because it splits text nodes before calling splitTreeTo(Node|Position).

It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.

> The reason that the id is duplicated for the span "wrapper" is that InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock() doesn't avoid duplicating ids, as SplitElementCommand::executeApply() does. Should I file a separate bug/write a separate patch for that?

Yes, please file a separate bug for that.  But maybe we should first merge that function with CompositeEditCommand::cloneParagraphUnderNewElement.  It's really terrible that we have all these functions that do slightly different things all over the place.

[Annie Sullivan]

(In reply to comment #6)
> (From update of attachment 96004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96004&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:1250
> > +        // If the position is at the end of the node, split at the next node.
> > +        if ((p.containerNode()->isTextNode() && p.offsetInContainerNode() == (int)static_cast<Text*>(p.containerNode())->length())
> > +            || (p.containerNode()->isElementNode() && p.offsetInContainerNode() == (int)splitNode->childNodes()->length()))
> > +            splitNode = splitNode->nextSibling();
> 
> This logic is likely wrong as it doesn't take invisible nodes into account.

You're right that I didn't take invisible nodes into account at all when writing this code. By invisible, do you mean display:none or something else? Is there a case I should add as a layout test?

(In reply to comment #7)
> Thanks for working on this bug.  I'm excited to see a progress.
> 
> (In reply to comment #5)
> > I wasn't sure whether to implement a new splitTreeToPosition method, or modify splitTreeToNode to take either. Please let me know what you think.
> 
> We should modify splitTreeToNode to take a Position instead of adding a new function.

Sounds good. I rewrote as a separate function to start with so that it's easier to see what I'm trying to do and point out mistakes in early review. I also wasn't clear on what the arguments should be--it could take a node AND a position, expecting one of them to be null, or just take a position, expecting that in the common case the position would be at the start of the node.

> > splitTreeToPosition currently doesn't handle splitting nodes if the position is not at the start or end of the node. Currently InsertParagraphSeparatorCommand is the only caller, and it never creates a case like that because it splits text nodes before calling splitTreeTo(Node|Position).
> 
> It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.

At the same time, complicating the arguments and adding one-off functionality to a simple, well-understood function used in many places seems quite dangerous, too. When I modify the function to take a position, I'll still need to answer this question about whether the position should be split, and make sure I don't introduce bugs with invisible nodes or anything else. Am I missing a less complicated way to fix this bug?

Also, it took me a bit of extra time to understand what splitTreeToNode() is supposed to do because there wasn't a unit testing framework that allowed me to pass in arbitrary nodes/dom trees and look at the output. Do you think it would be worth trying to add common editing methods like this to window.internals so that we can test them independently? (Or maybe I'm missing some existing way to do this?)

> 
> > The reason that the id is duplicated for the span "wrapper" is that InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock() doesn't avoid duplicating ids, as SplitElementCommand::executeApply() does. Should I file a separate bug/write a separate patch for that?
> 
> Yes, please file a separate bug for that.  But maybe we should first merge that function with CompositeEditCommand::cloneParagraphUnderNewElement.  It's really terrible that we have all these functions that do slightly different things all over the place.

Okay, I'll look into that.

[Ryosuke Niwa]

(In reply to comment #8)
> (In reply to comment #6) 
> > This logic is likely wrong as it doesn't take invisible nodes into account.
> 
> You're right that I didn't take invisible nodes into account at all when writing this code. By invisible, do you mean display:none or something else? Is there a case I should add as a layout test?

Yes. display: none and text nodes with collapsible whitespace, etc... In general, we need to be consistent in how we deal with these invisible nodes and the best way to do is to rely on VisiblePosition.

> > It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.
> 
> At the same time, complicating the arguments and adding one-off functionality to a simple, well-understood function used in many places seems quite dangerous, too. When I modify the function to take a position, I'll still need to answer this question about whether the position should be split, and make sure I don't introduce bugs with invisible nodes or anything else. Am I missing a less complicated way to fix this bug?

By splitting the position, do you mean splitting a text node which is the anchor node of a Position (where AnchorType is PositionIsOffsetInAnchor)?  We should be doing that iff we can come up with a test case where that is necessary.  As far as I know, we deal with that in callers.  But maybe it's better to encapsulate all of that in splitTreeToNode instead of imposing implicit pre-condition like that.

> Also, it took me a bit of extra time to understand what splitTreeToNode() is supposed to do because there wasn't a unit testing framework that allowed me to pass in arbitrary nodes/dom trees and look at the output. Do you think it would be worth trying to add common editing methods like this to window.internals so that we can test them independently? (Or maybe I'm missing some existing way to do this?)

I'm not so sure because that'll require us adding a custom editing command for splitTreeToNode since you can't call splitTreeToNode without instantiating a CompositeEditCommand.

[Annie Sullivan]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6) 
> > > This logic is likely wrong as it doesn't take invisible nodes into account.
> > 
> > You're right that I didn't take invisible nodes into account at all when writing this code. By invisible, do you mean display:none or something else? Is there a case I should add as a layout test?
> 
> Yes. display: none and text nodes with collapsible whitespace, etc... In general, we need to be consistent in how we deal with these invisible nodes and the best way to do is to rely on VisiblePosition.

I wanted to add a layout test where this doesn't work, so I could understand the problem better before trying to fix it. So I experimented with a few cases, and I wasn't able to find one where I was sure my function splits in the wrong place. Using "|" for cursor, I tried:

<span>Line1|    <br>Line2</span>
(extra whitespace after cursor removed when pressing enter)

<span>Line1|<span style="display:none">Invisible</span><br>Line2</span>
(invisible span moved to next line)

I must be misunderstanding the problem--do you have a more specific example of a case where this code is likely to break, so I can add a layout test for it?


When I do change the code to use VisiblePosition, is the correct approach to call Position::next instead of Node::nextSibling()?

> > > It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.
> > 
> > At the same time, complicating the arguments and adding one-off functionality to a simple, well-understood function used in many places seems quite dangerous, too. When I modify the function to take a position, I'll still need to answer this question about whether the position should be split, and make sure I don't introduce bugs with invisible nodes or anything else. Am I missing a less complicated way to fix this bug?
> 
> By splitting the position, do you mean splitting a text node which is the anchor node of a Position (where AnchorType is PositionIsOffsetInAnchor)?  We should be doing that iff we can come up with a test case where that is necessary.  As far as I know, we deal with that in callers.  But maybe it's better to encapsulate all of that in splitTreeToNode instead of imposing implicit pre-condition like that.

Yes, by splitting the position, I mean if the position is in the middle of a text node, it seems logical that splitTreeToPosition() would split the text node so that the tree is split exactly at the given position. Right now, all the callers do the splitting themselves, since the function used to only split to node boundaries. I agree that it's not a great idea to add code if no one uses it yet. But I think that since the new function will take a position, it will be confusing to callers who aren't familiar with the history of the editing code why callers are expected to do any actual splitting of text nodes at the position. Not sure how this type of problem is usually handled--a comment above the function?

Also, I'm still not clear on what the declaration of the new function should be:
Should it be renamed to splitTreeToPosition?
Should it take Position or a VisiblePosition?
Its arguments should be a start position and and end node?

> > Also, it took me a bit of extra time to understand what splitTreeToNode() is supposed to do because there wasn't a unit testing framework that allowed me to pass in arbitrary nodes/dom trees and look at the output. Do you think it would be worth trying to add common editing methods like this to window.internals so that we can test them independently? (Or maybe I'm missing some existing way to do this?)
> 
> I'm not so sure because that'll require us adding a custom editing command for splitTreeToNode since you can't call splitTreeToNode without instantiating a CompositeEditCommand.

My understanding is that CompositeEditingCommand exists to allow shared code between various editing commands. I know in some codebases, making a UnitTestEditingCommand that derives from CompositeEditingCommand that we could use to write unit tests for all those functions would be a way to do this. Not sure about WebKit.

[Ryosuke Niwa]

(In reply to comment #10)
> Yes, by splitting the position, I mean if the position is in the middle of a text node, it seems logical that splitTreeToPosition() would split the text node so that the tree is split exactly at the given position. Right now, all the callers do the splitting themselves, since the function used to only split to node boundaries.

Another concern I have is that callers of splitTree wouldn't be aware of the fact text node has been split.  So if they kept some positions anchored at the text node or have some assumptions about DOM structure around the text node, they might get affected by this change.  

> I agree that it's not a great idea to add code if no one uses it yet. But I think that since the new function will take a position, it will be confusing to callers who aren't familiar with the history of the editing code why callers are expected to do any actual splitting of text nodes at the position. Not sure how this type of problem is usually handled--a comment above the function?

I'd rather rename the function or change the function behavior than adding a comment. In general, we favor refactoring over adding comments.

> Also, I'm still not clear on what the declaration of the new function should be:
> Should it be renamed to splitTreeToPosition?

I always thought ToNode didn't any value to the function name.  I'd try renaming it to splitTree.

> Should it take Position or a VisiblePosition?

Is there a benefit in this function knowing upstream/downstream?  VisiblePosition is an expensive object to instantiate so I'd start with Position unless there is a compelling reason.

> My understanding is that CompositeEditingCommand exists to allow shared code between various editing commands. I know in some codebases, making a UnitTestEditingCommand that derives from CompositeEditingCommand that we could use to write unit tests for all those functions would be a way to do this. Not sure about WebKit.

That'll require us creating a Document, DocumentLoader, etc... because CompositeEditCommand can't be instantiated without a document.  In the discussion we had in webkit-dev, the general consensus (as far as I understand it) was to try to add unit tests for things in wtf and other components that are isolated from the rest of WebKit.  And I don't think editing is such a component because it depends heavily on CSS, DOM, and rendering components.

[Annie Sullivan]

Created attachment 96322 [details]
Work in Progress

[Ryosuke Niwa]

Comment on attachment 96322 [details]
Work in Progress

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

> LayoutTests/editing/inserting/return-key-before-br-in-span-expected.txt:1
> +This sets the selection to the end of the first line, and hits the enter key. Expected behavior: a div is created around the second line, and the cursor is placed at the start of the second line. See bug 61594.

This line is probably too long. You should put \n before "Expected behavior:"

> LayoutTests/editing/inserting/return-key-middle-of-span-expected.txt:1
> +This sets the selection to the middle of the first line, and hits the enter key. Expected behavior: the text node is split at the cursor. The span is cloned, and everything that was split out is placed inside the clone in a new div which is a child of the root. See bug 61594.

Ditto about the line length.

> Source/WebCore/editing/CompositeEditCommand.cpp:1232
> +PassRefPtr<Node> CompositeEditCommand::splitTree(Position const* start, Node* end, bool shouldSplitAncestor)

We don't normally pass stack allocated objects by pointers. Either Position& or const Position&.  Since you're changing the signature anyway, you may consider replacing the boolean by an enum.

> Source/WebCore/editing/CompositeEditCommand.cpp:1248
> +        if ((unsigned)start->offsetInContainerNode() == static_cast<Text*>(start->containerNode())->length())

Please use C++ style cast (i.e. static_cast<unsigned>()).

> Source/WebCore/editing/CompositeEditCommand.cpp:1252
> +            startNode = startNode->nextSibling();

This can't be right. SplitTextNodeCommand inserts a new text node before (not after) start->containerNode(). That tells me that this code isn't well-tested.

> Source/WebCore/editing/CompositeEditCommand.cpp:1257
> +    for (node = startNode; node && node->parentNode() != endNode; node = node->parentNode()) {

You should probably move the declaration of node into for loop as in
for (RefPtr<Node> node =...) {

> Source/WebCore/editing/IndentOutdentCommand.cpp:174
> +        Position beforeEnclosingBlockFlow(firstPositionInNode(enclosingBlockFlow));
> +        splitBlockquoteNode = splitTree(&beforeEnclosingBlockFlow, enclosingNode, true);

You wouldn't need to declare beforeEnclosingBlockFlow if you made splitTree to take a reference.

> Source/WebCore/editing/InsertListCommand.cpp:293
> +        Position beforeNextListChild(firstPositionInNode(nextListChild));
> +        splitElement(listNode, splitTree(&beforeNextListChild, listNode));

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:949
> +                nodeToSplitTo = splitTree(&insertionPos, nodeToSplitTo.get(), true).get();

The fact we're passing true here strongly suggests that shouldSplitAncestor should be an enum.

[Annie Sullivan]

Comment on attachment 96322 [details]
Work in Progress

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1243
> +    Node* startNode = start->containerNode();

The way I rewrote this function, it is the same as splitTreeToNode, except that it handles some special cases for choosing which starting node to use:
* If the position is at the end of a text node, it starts at the next node in the tree.
* If the position is in the middle of a text node, it splits the text node and starts at the second half of the split. It turns out I did find a clear use case for this behavior; see changes to ReplaceSelectionCommand::insertAsListItems

The new layout tests I added work fine with this new code. However, when I converted the other callers of splitTreeToNode to call splitTree() with a position, I ran into some trouble because I am making too many assumptions about positions, and several layout tests regressed. I think the problems all have the same root cause--I am assuming the containerNode of the position is the same node that was passed into splitTreeToNode originally. However, stepping through the debugger I have found that this is definitely not always the case. Many of the callers of splitTreeToNode() were actually already using a position, and passing in deprecatedNode() or anchorNode(). So I thought I could just pass the position in directly. But there are multiple ways a position can be anchored, and so often the containerNode I check below is not the same as the deprecatedNode/anchorNode that was passed in originally. I think I want to normalize the position so that the containerNode is always what used to be the start argument. I think I want to use containerNode instead of deprecatedNode/anchorNode, because I also call offsetInContainerNode(). But I  don't see a normalization function for Position except parentAnchoredEquivalent(), which I think sets the containerNode to the parent of the node I wanted it to be. Do you know what the correct thing to do here is?

> Source/WebCore/editing/CompositeEditCommand.cpp:1247
> +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?

If traverseNextNode() returns null on line 1249, it is because the position is at the end of the last node inside the end node. I am not sure how the function should behave in that case--should it start splitting at the containerNode's parent?

[Ryosuke Niwa]

(In reply to comment #14)
> The new layout tests I added work fine with this new code. However, when I converted the other callers of splitTreeToNode to call splitTree() with a position, I ran into some trouble because I am making too many assumptions about positions, and several layout tests regressed. I think the problems all have the same root cause--I am assuming the containerNode of the position is the same node that was passed into splitTreeToNode originally. However, stepping through the debugger I have found that this is definitely not always the case.

Right, I was expecting this to happen.

> I think I want to use containerNode instead of deprecatedNode/anchorNode, because I also call offsetInContainerNode(). But I  don't see a normalization function for Position except parentAnchoredEquivalent(), which I think sets the containerNode to the parent of the node I wanted it to be. Do you know what the correct thing to do here is?

The correct thing to do is to fix each caller of splitTree.  You're getting into the nasty part of editing now where fixing one bug gives you new bug.  Unfortunately, there's no easy way out of this other than looking at each caller of splitTree and fixing each one of them.

parentAnchoredEquivalent is specifically designed to create a Position to be passed to Range objects and we don't want to call it everywhere because as we discussed during the contributor's meeting, we want to move away from having positions with offset inside an element and calling parentAnchoredEquivalent will move us away from that goal.

> > Source/WebCore/editing/CompositeEditCommand.cpp:1247
> > +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?
> 
> If traverseNextNode() returns null on line 1249, it is because the position is at the end of the last node inside the end node. I am not sure how the function should behave in that case--should it start splitting at the containerNode's parent?

If traverseNextNode returns true, then there's no node left in the tree after this node.  There's no point in splitting tree at that point.  You should probably assert that this never happens.

[Annie Sullivan]

Created attachment 96368 [details]
Work in Progress

[Ryosuke Niwa]

Comment on attachment 96368 [details]
Work in Progress

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1246
> +        if (start.offsetInContainerNode() == caretMaxOffset(start.containerNode()))

should be >= instead of ==.

> Source/WebCore/editing/CompositeEditCommand.h:40
> +enum AncestorSplitPolicy { DoNotSplit, Split };

I'm not sure if 'policy' is a good noun here.  Maybe rule?  I'd also declare it inside CompositeEditCommand to avoid polluting WebCore namespace.  And I'd name values DoNotSplitEnd and SplitEnd.

> Source/WebCore/editing/FormatBlockCommand.cpp:64
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);

You shouldn't be calling deprecatedNode here.

> Source/WebCore/editing/IndentOutdentCommand.cpp:106
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);

Ditto.

[Annie Sullivan]

> > Source/WebCore/editing/FormatBlockCommand.cpp:64
> > +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
> 
> You shouldn't be calling deprecatedNode here.
> 
> > Source/WebCore/editing/IndentOutdentCommand.cpp:106
> > +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
> 
> Ditto.

The good news about this patch is that all the layout tests pass. It turns out that on these two lines of code, there would be consistent failures because containerNode() was always the parent of the correct node. That means that unless further testing proves otherwise, at least the selection seems to have consistent behavior about how deprecatedNode is set.

The bad news, of course, is that I am calling deprecatedNode() where I shouldn't be. And unfortunately, the problem goes deeper than just these two lines. If you look at the code surrounding my other calls, you'll see that it has dependencies on deprecatedNode() in other places, too:

In IndentOutdentCommand::outdentParagraph(), the code that finds enclosingBlockFlow looks like this: enclosingBlock(visibleStartOfParagraph.deepEquivalent().deprecatedNode()). Then I make a Position from enclosingBlockFlow and pass it to splitTree.

In InsertListCommand::unlistifyParagraph, the same thing happens with the nextListChild and listChildNode nodes used to make the selections passed into splitTree().

In ReplaceSelectionCommand::insertAsListItems(), there used to be code that called deprecatedNode() to split the children, but my new code seems to work fine calling containerNode()/offsetInContainer instead. Is that one okay? I think ReplaceSelectionCommand::doApply() and InsertParagraphSeparatorCommand::doApply() are okay as-is as well. Let me know if you disagree.

So I wanted to check about the right way to move forward. I think it is:
1. File dependent bugs to not use deprecatedNode() in the places I listed above. Are there some existing refactoring patches I can look at to see examples of how this is usually done?
2. File dependent bugs on the refactoring bugs above to add test cases.
3. Add more tests cases to this patch, too. It turns out that editing/pasteboard/paste-list-004.html does test the case where we need to split a text node, but we should have more test cases as well.
4. Finally finish off this patch, without depending on deprecatedNode.

Is that right? Or is there some simpler way to do things? Maybe we could add more tests to the codepaths that use deprecatedNode in this patch, and if they prove to be consistent, do some smaller patch-up?

[Ryosuke Niwa]

(In reply to comment #18)
> In IndentOutdentCommand::outdentParagraph(), the code that finds enclosingBlockFlow looks like this: enclosingBlock(visibleStartOfParagraph.deepEquivalent().deprecatedNode()). Then I make a Position from enclosingBlockFlow and pass it to splitTree.

We should be calling containerNode instead of deprecatedNode there.  Because if we're before or after a block, the block isn't enclosing the position at all.  If something goes wrong with that change, then we'd have to fix that bug.

Once you do that, the change is straight-forward.  You'd just need to split the tree at the beginning of this block.

> In ReplaceSelectionCommand::insertAsListItems(), there used to be code that called deprecatedNode() to split the children, but my new code seems to work fine calling containerNode()/offsetInContainer instead. Is that one okay? I think ReplaceSelectionCommand::doApply() and InsertParagraphSeparatorCommand::doApply() are okay as-is as well. Let me know if you disagree.

That sounds like a reasonable change.

> So I wanted to check about the right way to move forward. I think it is:
> 1. File dependent bugs to not use deprecatedNode() in the places I listed above. Are there some existing refactoring patches I can look at to see examples of how this is usually done?

You can see blockers on https://bugs.webkit.org/show_bug.cgi?id=52099 that have been resolved.

> 2. File dependent bugs on the refactoring bugs above to add test cases.

That sounds great!

> Is that right? Or is there some simpler way to do things? Maybe we could add more tests to the codepaths that use deprecatedNode in this patch, and if they prove to be consistent, do some smaller patch-up?

That sounds like a very reasonable approach.

[Annie Sullivan]

(In reply to comment #19)
> (In reply to comment #18)
> > In IndentOutdentCommand::outdentParagraph(), the code that finds enclosingBlockFlow looks like this: enclosingBlock(visibleStartOfParagraph.deepEquivalent().deprecatedNode()). Then I make a Position from enclosingBlockFlow and pass it to splitTree.
> 
> We should be calling containerNode instead of deprecatedNode there.  Because if we're before or after a block, the block isn't enclosing the position at all.  If something goes wrong with that change, then we'd have to fix that bug.

I made some progress on this part. In 2 of the 3 cases I mentioned, all the layout tests pass when I switch to calling containerNode(). But I got two test failures in InsertListCommand::unlistifyParagraph().

Line 268 of my patch used to be:
nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
I switched it to:
nextListChild = enclosingListChild(end.next().deepEquivalent().containerNode(), listNode);

I got two layout tests failures:
editing/execCommand/remove-list-items.html
editing/execCommand/switch-list-type.html

In both tests, sometimes the position is a PositionIsOffsetInAnchor, and the containerNode, anchorNode, and deprecatedNode are all the same node. Everything works fine in that case. But sometimes the position is a PositionIsBeforeAnchor, before the <br><br> after item1. In that case, the deprecatedNode and anchorNode are the <br>, where the split is supposed to occur. But the containerNode() is the parent <ol>. So the split occurs in the wrong place.

Is there some better way to get the correct Position before end.next() (where end is a VisiblePosition)?

[Ryosuke Niwa]

(In reply to comment #20)
> Line 268 of my patch used to be:
> nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
> I switched it to:
> nextListChild = enclosingListChild(end.next().deepEquivalent().containerNode(), listNode);

I don't understand why you'd call next.  That seems wrong.

> I got two layout tests failures:
> editing/execCommand/remove-list-items.html
> editing/execCommand/switch-list-type.html
> 
> In both tests, sometimes the position is a PositionIsOffsetInAnchor, and the containerNode, anchorNode, and deprecatedNode are all the same node. Everything works fine in that case. But sometimes the position is a PositionIsBeforeAnchor, before the <br><br> after item1. In that case, the deprecatedNode and anchorNode are the <br>, where the split is supposed to occur. But the containerNode() is the parent <ol>. So the split occurs in the wrong place.

I see.  <br> is certainly NOT the enclosing list child for a position before or after the br.  I think what you need to do is check whether the containerNode is a list element or not, and if it is, just pass end as the place to split rather than finding the enclosing list child.  Another way to fix this is to make enclosingListChild also take const Position&.

[Dave Comfort]

This bug is really hurting Zimbra. All of our users on Chrome who have a signature are struggling just to compose an email message.

[Annie Sullivan]

Created attachment 97678 [details]
Patch

[Annie Sullivan]

The newest patch is a cleaned-up version that fixes usages of deprecatedNode() where possible, but leaves some in where the code would need significant refactoring.

There are a couple places where I switched deprecatedNode to containerNode because it didn't break anything, but maybe this is too aggressive. If you think so I'd be happy to change them back.

About the FIXME in splitTree: what should we do if the position is the last visible position in the node, and traverseNextNode returns null? Probably exit early, but not sure which node the function should return.

If you can think of more tests to add, please let me know.

[Early Warning System Bot]

Comment on attachment 97678 [details]
Patch

Attachment 97678 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8907479

[Ryosuke Niwa]

Comment on attachment 97678 [details]
Patch

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

This patch looks great but I have a few questions and nits.  I'd say r- for now since I'd like see change log and some comments being revised before this patch is landed.

> Source/WebCore/ChangeLog:16
> +        (WebCore::CompositeEditCommand::splitTree):
> +            Implement new function. Like splitTreeToNode(), but picks start node based on position so that text nodes are split correctly.

So we usually start sentences after "splitTree): " then wrap at wherever is appropriate.  This line is probably too long.  You should also mention that this new function replaces the old splitTreeToNode.

> Source/WebCore/editing/CompositeEditCommand.cpp:-1235
> -    ASSERT(start);

We should probably assert that start.containerNode is not null.

> Source/WebCore/editing/CompositeEditCommand.cpp:1243
> +    // For text nodes, handle the case where the offset is in the middle of the node or at the end,
> +    // moving the start node to the beginning of the position, splitting the node if necessary.

This comment repeats what code says.  You should either remove the comment or explain why this is needed instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1245
> +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?

If startNode ends up being null, then we're trying to split a node at the very end of the tree.  In that case, we shouldn't be splitting the tree so this should be fine.
I think you should remove this comment.

> Source/WebCore/editing/FormatBlockCommand.cpp:64
> -    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTreeToNode(start.deprecatedNode(), nodeToSplitTo);
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);

So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.

> Source/WebCore/editing/InsertListCommand.cpp:-271
> -        nextListChild = listChildNode->nextSibling();
> -        previousListChild = listChildNode->previousSibling();
>      } else {
>          // A paragraph is visually a list item minus a list marker.  The paragraph will be moved.
>          start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
>          end = endOfParagraph(start, CanSkipOverEditingBoundary);
> -        nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
> -        ASSERT(nextListChild != listChildNode);
> -        previousListChild = enclosingListChild(start.previous().deepEquivalent().deprecatedNode(), listNode);
> -        ASSERT(previousListChild != listChildNode);

Nice to see code being removed!

> Source/WebCore/editing/InsertListCommand.cpp:277
> +    // Unless this is the first node in the list, the list needs to be split.

Again, this comment seems to repeat code.  Can we explain why this needs to be done instead?  If not, we should get rid of this comment.

> Source/WebCore/editing/InsertListCommand.cpp:282
> +    // If this is the only node in the list, insert the placeholder after it, so that if splitAt
> +    // and the node before the placeholder are both text nodes, they will not be merged. See
> +    // LayoutTests/editing/execCommand/switch-list-type.html

This comment seems a bit verbose.  I'd just say "insert a placeholder to avoid texts nodes of splitAt and the node before the placeholder won't get merged"

[Annie Sullivan]

Comment on attachment 97678 [details]
Patch

I'm working on addressing the comments but I had a question and a follow-up comment.

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

>> Source/WebCore/editing/CompositeEditCommand.cpp:1245
>> +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?
> 
> If startNode ends up being null, then we're trying to split a node at the very end of the tree.  In that case, we shouldn't be splitting the tree so this should be fine.
> I think you should remove this comment.

Should there be a return value in this case though?

>> Source/WebCore/editing/FormatBlockCommand.cpp:64
>> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
> 
> So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.

Unfortunately, a lot of edge cases around top-level non-block elements fail here. For example, if the editable div contains only a <hr>, start.deprectatedNode() is the <hr> and start.containerNode() is the editable div. The ASSERT(start.containerNode() != end) fails, and if I take it out I end up with a crash later on.

I'll try firstPositionInOrBeforeNode instead.

[Ryosuke Niwa]

Comment on attachment 97678 [details]
Patch

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

>>> Source/WebCore/editing/CompositeEditCommand.cpp:1245
>>> +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?
>> 
>> If startNode ends up being null, then we're trying to split a node at the very end of the tree.  In that case, we shouldn't be splitting the tree so this should be fine.
>> I think you should remove this comment.
> 
> Should there be a return value in this case though?

You should probably return the original startNode in that case because that's what we do when we don't split any node.

>>> Source/WebCore/editing/FormatBlockCommand.cpp:64
>>> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
>> 
>> So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.
> 
> Unfortunately, a lot of edge cases around top-level non-block elements fail here. For example, if the editable div contains only a <hr>, start.deprectatedNode() is the <hr> and start.containerNode() is the editable div. The ASSERT(start.containerNode() != end) fails, and if I take it out I end up with a crash later on.
> 
> I'll try firstPositionInOrBeforeNode instead.

I see. Okay. I suppose we can live with this code for now.

[Annie Sullivan]

Created attachment 98094 [details]
Patch

[Annie Sullivan]

Comment on attachment 97678 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +            Implement new function. Like splitTreeToNode(), but picks start node based on position so that text nodes are split correctly.
> 
> So we usually start sentences after "splitTree): " then wrap at wherever is appropriate.  This line is probably too long.  You should also mention that this new function replaces the old splitTreeToNode.

Done.

>> Source/WebCore/editing/CompositeEditCommand.cpp:-1235
>> -    ASSERT(start);
> 
> We should probably assert that start.containerNode is not null.

Good catch! Done.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1243
>> +    // moving the start node to the beginning of the position, splitting the node if necessary.
> 
> This comment repeats what code says.  You should either remove the comment or explain why this is needed instead.

Comment removed.

>>>> Source/WebCore/editing/FormatBlockCommand.cpp:64
>>>> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
>>> 
>>> So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.
>> 
>> Unfortunately, a lot of edge cases around top-level non-block elements fail here. For example, if the editable div contains only a <hr>, start.deprectatedNode() is the <hr> and start.containerNode() is the editable div. The ASSERT(start.containerNode() != end) fails, and if I take it out I end up with a crash later on.
>> 
>> I'll try firstPositionInOrBeforeNode instead.
> 
> I see. Okay. I suppose we can live with this code for now.

It turns out that calling firstPositionInOrBeforeNode trips on the same edge cases that break when I pass the position into splitTree directly--the container node of the position ends up being the editable div instead of the <hr>, and I hit the assert and it crashes.

>> Source/WebCore/editing/InsertListCommand.cpp:277
>> +    // Unless this is the first node in the list, the list needs to be split.
> 
> Again, this comment seems to repeat code.  Can we explain why this needs to be done instead?  If not, we should get rid of this comment.

comment removed.

>> Source/WebCore/editing/InsertListCommand.cpp:282
>> +    // LayoutTests/editing/execCommand/switch-list-type.html
> 
> This comment seems a bit verbose.  I'd just say "insert a placeholder to avoid texts nodes of splitAt and the node before the placeholder won't get merged"

done.

[Ryosuke Niwa]

Comment on attachment 97678 [details]
Patch

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

>>>>> Source/WebCore/editing/FormatBlockCommand.cpp:64
>>>>> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
>>>> 
>>>> So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.
>>> 
>>> Unfortunately, a lot of edge cases around top-level non-block elements fail here. For example, if the editable div contains only a <hr>, start.deprectatedNode() is the <hr> and start.containerNode() is the editable div. The ASSERT(start.containerNode() != end) fails, and if I take it out I end up with a crash later on.
>>> 
>>> I'll try firstPositionInOrBeforeNode instead.
>> 
>> I see. Okay. I suppose we can live with this code for now.
> 
> It turns out that calling firstPositionInOrBeforeNode trips on the same edge cases that break when I pass the position into splitTree directly--the container node of the position ends up being the editable div instead of the <hr>, and I hit the assert and it crashes.

That makes me think that we should probably be instantiating a legacy editing position here.

[Ryosuke Niwa]

(In reply to comment #31)
> > It turns out that calling firstPositionInOrBeforeNode trips on the same edge cases that break when I pass the position into splitTree directly--the container node of the position ends up being the editable div instead of the <hr>, and I hit the assert and it crashes.
> 
> That makes me think that we should probably be instantiating a legacy editing position here.

Also where do we hit this assertion?

[Ryosuke Niwa]

Comment on attachment 98094 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1246
> +        // FIXME: startNode can be null if traverseNextNode returns null. What should it be set to in that case?

You should remove this comment now that you handle this case.

> Source/WebCore/editing/FormatBlockCommand.cpp:64
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);

We should really be cautious when we call firstPositionInNode because we really don't want to instantiate a position inside a br, hr, etc... So whenever there's a chance that a node can be one of those atomic/content-ignored node, we can't pass it to firstPositionInNode as it creates a new kind of position that doesn't have legacy editing quirks. r- because deprecatedNode can be one of those elements.

> Source/WebCore/editing/IndentOutdentCommand.cpp:106
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);

Ditto.

> Source/WebCore/editing/InsertListCommand.cpp:279
> +    // Insert a placeholder to avoid texts nodes of splitAt and the node before the placeholder won't get merged

Nit: avoid ~~ won't get merged.  Maybe it was my fault?

[Annie Sullivan]

Created attachment 98209 [details]
Alternate approach

This approach fixes the specific problem that regressed. It is much lower risk than refactoring splitTreeToNode, but it does not improve the code at all.

[Annie Sullivan]

I filed bug 63164 to continue the work on refactoring splitTreeToNode to take a position.

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 98209 [details]:

http/tests/misc/dns-prefetch-control.html bug 63200 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 98209 [details]
Alternate approach

Clearing flags on attachment: 98209

Committed r89492: <http://trac.webkit.org/changeset/89492>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.