Bug 75652 – Inconsistent text selection behavior with option-shift-left/right

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

[Nicholas Allegra]

To reproduce, enter a word in a text field, place the cursor in the middle of the word, and press option-shift-left followed by option-shift-right.  In WebKit text fields, the latter changes the selection to go from the middle to the end of the word, but in OS text fields it only brings the cursor back to the middle, and another option-shift-right is required to select to the end of the word.

[Alexey Proskuryakov]

Indeed, this is different from what TextEdit does.

[Pablo Flouret]

Created attachment 121960 [details]
proposed patch

[Pablo Flouret]

Don't have a unix machine handy, but i assume it's the same behavior as windows. Also, option+alt+arrows seem to fire ParagraphGranularity, but programatically LineGranularity works the same way, as far as i could see, so i set it to handle both (in addition to word).

Don't know how to emulate SentenceGranularity with the keyboard, so i'm not sure if it should also be handled the same way or not, i guess it's not really important anyway...

[Enrica Casucci]

Comment on attachment 121960 [details]
proposed patch

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

> Source/WebCore/ChangeLog:14
> +

There is no description of what the fix is. You should also provide some information of there changes to each file.

> Source/WebCore/editing/FrameSelection.cpp:943
> +            // behavior when, for instance, word-selecting backwards starting with the caret on

typo "on". Should be "in". You should provide some of this information in the ChangeLog.

> Source/WebCore/editing/FrameSelection.cpp:946
> +            VisibleSelection testSelection(m_selection);

This is a non descriptive variable name. I would rather use newSelection.

[Pablo Flouret]

Created attachment 122051 [details]
fixed review comments from enrica

[Pablo Flouret]

Ping?

[Ryosuke Niwa]

Comment on attachment 122051 [details]
fixed review comments from enrica

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

r=me provided nits below are fixed.

> Source/WebCore/editing/FrameSelection.cpp:946
> +            VisibleSelection newSelection(m_selection);

We prefer newSelection = m_selection over newSelection(m_selection).

> LayoutTests/editing/selection/selection-extend-should-not-move-across-caret-on-mac.html:21
> +    function editingTest(behavior) {
> +        if (window.layoutTestController) {
> +            layoutTestController.dumpAsText();
> +            layoutTestController.setEditingBehavior(behavior);
> +        }

It seems redundant to indent the entire contents inside <script>.

[Pablo Flouret]

Created attachment 122785 [details]
nitfixing

[Alexey Proskuryakov]

Comment on attachment 122785 [details]
nitfixing

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

> Source/WebCore/editing/EditingBehavior.h:73
> +    // On Mac, selecting backwards by word/line from the middle of a word/line, and then going
> +    // forward leaves the caret back in the middle with no selection, instead of directly selecting
> +    // to the other end of the line/word (Unix/Windows behavior).
> +    bool shouldExtendSelectionByWordOrLineAcrossCaret() const { return m_type != EditingMacBehavior; }

Just a drive by comment that shouldn't prevent landing: isn't this overly specific? This sounds like one example of anchored selection behavior.

[Enrica Casucci]

Comment on attachment 122785 [details]
nitfixing

Looks good to me.

[WebKit Review Bot]

Comment on attachment 122785 [details]
nitfixing

Rejecting attachment 122785 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Pablo Flouret]

Could anyone land this one for me if there are no further objections? Thanks!

[WebKit Review Bot]

Comment on attachment 122785 [details]
nitfixing

Clearing flags on attachment: 122785

Committed r105473: <http://trac.webkit.org/changeset/105473>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.