[webkit-reviews] review denied: [Bug 61594] REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span : [Attachment 98094] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Annie Sullivan
<sullivan at chromium.org>'s request for review:
Bug 61594: REGRESSION: Hitting enter in the middle of this span causes the
cursor to go to the end of the span
https://bugs.webkit.org/show_bug.cgi?id=61594

Attachment 98094: Patch
https://bugs.webkit.org/attachment.cgi?id=98094&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?