[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 #13 from Ryosuke Niwa <rniwa at webkit.org>  2011-06-07 16:39:33 PST ---
(From update of attachment 96322)
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.

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