[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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97678|review?                     |review-
               Flag|                            |




--- Comment #26 from Ryosuke Niwa <rniwa at webkit.org>  2011-06-21 14:23:26 PST ---
(From update of attachment 97678)
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"

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