[Webkit-unassigned] [Bug 60529] Programmatically set selection should not have direction on Mac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60529





--- Comment #26 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-22 19:18:41 PST ---
(From update of attachment 101801)
View in context: https://bugs.webkit.org/attachment.cgi?id=101801&action=review

mn... it's really hard to tell when we should be preserving isDirectional here.  On one hand, it seems odd to preserve isDirectional if we were creating selection out of a position and new selection is nothing to do with the old selection but on the other hand, in some cases, new selection is visually identical to old selection to users and user would expect base and extent to persist over editing operations.  Even more, setting extent or base will have a different effect depending on whether the caret selection was directionless or not.

Let me think about this issue for a while. Meanwhile, other reviewers are welcome to review or comment on this bug.

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:114
> -        setEndingSelection(VisibleSelection(positionBeforeNode(placeholder.get()), DOWNSTREAM));
> +        setEndingSelection(VisibleSelection(positionBeforeNode(placeholder.get()), DOWNSTREAM, endingSelection().isDirectional()));

It's not clear that we should be preserving isDirectional of the original endingSelection here.

> Source/WebCore/editing/BreakBlockquoteCommand.cpp:81
> -        setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM));
> +        setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional()));

Ditto about preserving isDirectional here.

> Source/WebCore/editing/BreakBlockquoteCommand.cpp:91
> -        setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM));
> +        setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional()));

Ditto.

> Source/WebCore/editing/BreakBlockquoteCommand.cpp:124
> -        setEndingSelection(VisibleSelection(VisiblePosition(firstPositionInOrBeforeNode(startNode))));
> +        setEndingSelection(VisibleSelection(VisiblePosition(firstPositionInOrBeforeNode(startNode)), endingSelection().isDirectional()));

Ditto.

> Source/WebCore/editing/BreakBlockquoteCommand.cpp:197
> -    setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM));
> +    setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional()));

Ditto.

> Source/WebCore/editing/CompositeEditCommand.cpp:913
> -    setEndingSelection(VisibleSelection(start, end, DOWNSTREAM));
> +    setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional()));

I don't think we should be using endingSelection's isDirectional here because this a temporary selection used by deleteSelection to delete the contents we're moving.

> Source/WebCore/editing/CompositeEditCommand.cpp:1014
> -    setEndingSelection(VisibleSelection(start, end, DOWNSTREAM));
> +    setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional()));

Ditto.

> Source/WebCore/editing/CompositeEditCommand.cpp:1123
> -    setEndingSelection(VisibleSelection(firstPositionInNode(newBlock.get()), DOWNSTREAM));
> +    setEndingSelection(VisibleSelection(firstPositionInNode(newBlock.get()), DOWNSTREAM, endingSelection().isDirectional()));

Ditto about setting isDirectional on a position.

> Source/WebCore/editing/CompositeEditCommand.cpp:1161
> -    setEndingSelection(VisibleSelection(atBR));
> +    setEndingSelection(VisibleSelection(atBR, endingSelection().isDirectional()));

Ditto.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:824
> -        setEndingSelection(VisibleSelection(m_endingPosition, affinity));
> +        setEndingSelection(VisibleSelection(m_endingPosition, affinity, endingSelection().isDirectional()));

Ditto about setting isDirectional on caret.

> Source/WebCore/editing/InsertLineBreakCommand.cpp:124
> -        setEndingSelection(VisibleSelection(endingPosition));
> +        setEndingSelection(VisibleSelection(endingPosition, endingSelection().isDirectional()));

Ditto.

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