Bug 75652 - Inconsistent text selection behavior with option-shift-left/right
Summary: Inconsistent text selection behavior with option-shift-left/right
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-05 14:51 PST by Nicholas Allegra
Modified: 2012-01-19 18:51 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (8.83 KB, patch)
2012-01-10 19:10 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
fixed review comments from enrica (9.83 KB, patch)
2012-01-11 10:50 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
nitfixing (9.68 KB, patch)
2012-01-17 11:22 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Allegra 2012-01-05 14:51:08 PST
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.
Comment 1 Alexey Proskuryakov 2012-01-05 23:11:46 PST
Indeed, this is different from what TextEdit does.
Comment 2 Pablo Flouret 2012-01-10 19:10:50 PST
Created attachment 121960 [details]
proposed patch
Comment 3 Pablo Flouret 2012-01-10 19:14:51 PST
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...
Comment 4 Enrica Casucci 2012-01-11 09:30:24 PST
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.
Comment 5 Pablo Flouret 2012-01-11 10:50:22 PST
Created attachment 122051 [details]
fixed review comments from enrica
Comment 6 Pablo Flouret 2012-01-17 01:05:44 PST
Ping?
Comment 7 Ryosuke Niwa 2012-01-17 11:09:03 PST
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>.
Comment 8 Pablo Flouret 2012-01-17 11:22:15 PST
Created attachment 122785 [details]
nitfixing
Comment 9 Alexey Proskuryakov 2012-01-17 11:33:21 PST
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.
Comment 10 Enrica Casucci 2012-01-17 13:42:03 PST
Comment on attachment 122785 [details]
nitfixing

Looks good to me.
Comment 11 WebKit Review Bot 2012-01-17 17:46:25 PST
Comment on attachment 122785 [details]
nitfixing

Rejecting attachment 122785 [details] from commit-queue.

pablof@motorola.com 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.
Comment 12 Pablo Flouret 2012-01-19 17:37:03 PST
Could anyone land this one for me if there are no further objections? Thanks!
Comment 13 WebKit Review Bot 2012-01-19 18:51:18 PST
Comment on attachment 122785 [details]
nitfixing

Clearing flags on attachment: 122785

Committed r105473: <http://trac.webkit.org/changeset/105473>
Comment 14 WebKit Review Bot 2012-01-19 18:51:23 PST
All reviewed patches have been landed.  Closing bug.