Caret cannot leave pasted content once it has been pasted

[Myles C. Maxfield]

Cursor cannot leave pasted content once it has been pasted

[Myles C. Maxfield]

Created attachment 224546 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 224546 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        When copying in the shouldConvertPositionStyleOnCopy mode, insert a Zero Width Space
> +        before and after the copied selection. This allows the cursor to leave the pasted content.
> +
> +        No new tests, but existing tests are updated.

I don't think this makes sense.

We should modify InsertLineBreakCommand and/or InsertParagraphSeparator to break out of the pasted content instead.

[Radar WebKit Bug Importer]

<rdar://problem/16112583>

[Myles C. Maxfield]

Created attachment 224690 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 224690 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1405
> +    if (!document().settings() || !document().settings()->shouldConvertPositionStyleOnCopy())
> +        return false;
> +

I don't think we want to restrict this behavior to just this case mode.
It'll be also fragile because then we won't be testing this code path as much.

> Source/WebCore/editing/CompositeEditCommand.cpp:1408
> +        

Nit: whitespace.

> Source/WebCore/editing/CompositeEditCommand.cpp:1409
> +    VisiblePosition caret(endingSelection().visibleStart());

Nit: Use = instead of copy constructor syntax.

> Source/WebCore/editing/CompositeEditCommand.cpp:1412
> +    Node* body = highestEnclosingNodeOfType(caret.deepEquivalent(), &isBody);
> +    if (!body)
> +        return false;

We should look for the root editable element instead as we talked about the other day.

> Source/WebCore/editing/CompositeEditCommand.cpp:1414
> +    RefPtr<Node> br = createBreakElement(document());

Use createDefaultParagraphElement instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1420
> +    VisiblePosition previous(caret.previous(CannotCrossEditingBoundary));
> +    if (previous == VisiblePosition()) {
> +        insertNodeBefore(br, body->firstChild());
> +        return true;
> +    }

A better condition to check would be misleadingly named isStartOfDocument and isEndOfDocument.

[Ryosuke Niwa]

Comment on attachment 224690 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1405
> +    if (!document().settings() || !document().settings()->shouldConvertPositionStyleOnCopy())
> +        return false;
> +

I don't think we want to restrict this behavior to just this case mode.
It'll be also fragile because then we won't be testing this code path as much.

> Source/WebCore/editing/CompositeEditCommand.cpp:1408
> +        

Nit: whitespace.

> Source/WebCore/editing/CompositeEditCommand.cpp:1409
> +    VisiblePosition caret(endingSelection().visibleStart());

Nit: Use = instead of copy constructor syntax.

> Source/WebCore/editing/CompositeEditCommand.cpp:1412
> +    Node* body = highestEnclosingNodeOfType(caret.deepEquivalent(), &isBody);
> +    if (!body)
> +        return false;

We should look for the root editable element instead as we talked about the other day.

> Source/WebCore/editing/CompositeEditCommand.cpp:1414
> +    RefPtr<Node> br = createBreakElement(document());

Use createDefaultParagraphElement instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1420
> +    VisiblePosition previous(caret.previous(CannotCrossEditingBoundary));
> +    if (previous == VisiblePosition()) {
> +        insertNodeBefore(br, body->firstChild());
> +        return true;
> +    }

A better condition to check would be misleadingly named isStartOfDocument and isEndOfDocument.

[Myles C. Maxfield]

Comment on attachment 224690 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1401
> +bool CompositeEditCommand::breakOutOfInitialOrFinalStyle()

I should move this out of the parent class and into the base class

[Myles C. Maxfield]

This has been fixed a different way.