[Webkit-unassigned] [Bug 61594] REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61594





--- Comment #18 from Annie Sullivan <sullivan at chromium.org>  2011-06-07 21:11:31 PST ---
> > 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.