Bug 205378 – TextManipulationController should respect new token orders

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

[Ryosuke Niwa]

TextManipulationController::completeManipulation should use the new order of tokens.

[Ryosuke Niwa]

Created attachment 385941 [details]
WIP

[Ryosuke Niwa]

Created attachment 386138 [details]
WIP2

[Ryosuke Niwa]

Created attachment 386188 [details]
Implements the new behavior

[Ryosuke Niwa]

<rdar://problem/56567065>

[Wenson Hsieh]

Comment on attachment 386188 [details]
Implements the new behavior

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

> Source/WebCore/editing/TextManipulationController.cpp:279
> +    RefPtr<Node> parent;

I found it confusing that `!parent` means that we should append the child node to a separate insertion position instead, but I can't really think of an alternative that doesn't end up making this more complicated :/

Maybe add a comment to clarify this?

> Source/WebCore/editing/TextManipulationController.cpp:369
> +        if (i == currentElementStack.size() && i == currentAncestors.size()) {
> +            insertions.append(NodeInsertion { currentElementStack.size() ? currentElementStack.last().ptr() : nullptr, contentNode.releaseNonNull() });
> +        } else {

Nit - no braces?

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 386188 [details]
> Implements the new behavior
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386188&action=review
> 
> > Source/WebCore/editing/TextManipulationController.cpp:279
> > +    RefPtr<Node> parent;
> 
> I found it confusing that `!parent` means that we should append the child
> node to a separate insertion position instead, but I can't really think of
> an alternative that doesn't end up making this more complicated :/
> 
> Maybe add a comment to clarify this?

Good point. Maybe rename it to parentIfDifferentFromCommonAncestor?

> > Source/WebCore/editing/TextManipulationController.cpp:369
> > +        if (i == currentElementStack.size() && i == currentAncestors.size()) {
> > +            insertions.append(NodeInsertion { currentElementStack.size() ? currentElementStack.last().ptr() : nullptr, contentNode.releaseNonNull() });
> > +        } else {
> 
> Nit - no braces?

Will fix.

[Wenson Hsieh]

Comment on attachment 386188 [details]
Implements the new behavior

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

>>> Source/WebCore/editing/TextManipulationController.cpp:279
>>> +    RefPtr<Node> parent;
>> 
>> I found it confusing that `!parent` means that we should append the child node to a separate insertion position instead, but I can't really think of an alternative that doesn't end up making this more complicated :/
>> 
>> Maybe add a comment to clarify this?
> 
> Good point. Maybe rename it to parentIfDifferentFromCommonAncestor?

Yeah, I think that's a clearer name, given the behavior.

[Ryosuke Niwa]

Committed r253860: <https://trac.webkit.org/changeset/253860>