Bug 36539 - shift+home/end and cmd+shift+left/right don't extend the selection correctly
Summary: shift+home/end and cmd+shift+left/right don't extend the selection correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 41975
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-24 09:52 PDT by Ojan Vafai
Modified: 2010-09-19 18:58 PDT (History)
5 users (show)

See Also:


Attachments
Patch (23.20 KB, patch)
2010-03-25 15:26 PDT, Ojan Vafai
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-03-24 09:52:01 PDT
The correct Mac behavior is to extend the selection. The current behavior is to move the extent of the selection.

Given the following test:
This is the first line.
This is the second line.
This is the third line.
This is the fourth line.

1. Click on "third".
2. shift + up
3. cmd+shift+left
4. cmd+shift+right
Expected result: The full second and third lines should be selected.
Actual result: The second half of the third line is selected.

1. Click on "third".
2. shift + up
3. shift + home
4. shift + end
Expected result: All four lines should be selected.
Actual result: The second half of the third line and the whole fourth line is selected.
Comment 1 Ojan Vafai 2010-03-25 15:26:15 PDT
Created attachment 51683 [details]
Patch
Comment 2 Darin Adler 2010-03-26 10:01:16 PDT
Comment on attachment 51683 [details]
Patch

> +    Settings* settings = m_frame ? m_frame->settings() : 0;
> +    // The trialSelectionController does not have access to an m_frame, so we need to get the editing behavior first.
> +    isEditingMacBehavior = isEditingMacBehavior || settings && settings->editingBehavior() == EditingMacBehavior;

Adding yet another parameter to the already complex modify function is annoying and available.

The use of "||" here seems a bit haphazard to me. There's no real need for an "or" here.

Instead it would be better to break the modify function into a public one with the same interface as today, and a private function, probably with a different name, that takes an additional argument. The cleanest type for the additional argument would be EditingBehavior, but you might not want to do all the work to separate out the EditingBehavior enum so it can be included in the SelectionController.h header file without pulling in all of Settings.h. So you the extra argument to the private function could be a Settings*.

> +            // This matches NSTextView behavior. When extending a selection via LineBoundary, ParagraphBoundary or
> +            // DocumentBoundary, the selection always grows instead of just moving the extent.
> +            if (isEditingMacBehavior && !m_selection.isCaret()
> +                && (granularity == LineBoundary || granularity == ParagraphBoundary || granularity == DocumentBoundary)
> +                && ((dir == FORWARD || dir == RIGHT) && !m_selection.isBaseFirst()
> +                    || (dir == LEFT || dir == BACKWARD) && m_selection.isBaseFirst()))

This long expression with nested boolean logic is hard to read.

A couple helper functions might collapse this and make it readable, for example one tells if a granularity is one of the "boundary" styles instead of listing them all.

Another kind of helper function that could make the logic clearer would be setStart and setEnd functions that decide whether to modify the base or extent. Using them instead of having logic here with all that isBaseFirst stuff could make it a lot easier to see what the code is doing.

    if (!isEditingMacBehavior || m_selection.isCaret() || !isBoundary(granularity))
        setExtent(position, userTriggered);
    else {
        // Standard Mac behavior when extending to a boundary is grow the selection rather
        // than leaving the base in place and moving the extent. Matches NSTextView.
        if (direction == FORWARD || direction == RIGHT)
            setEnd(position, userTriggered);
        else
            setStart(position, userTriggered);
    }

I think the code I wrote above is easier to understand.

I'm going to say review+, but I think both of my comments represent opportunities to make this code significantly clearer. Please consider my suggestions.
Comment 3 Ojan Vafai 2010-03-26 13:08:43 PDT
(In reply to comment #2)
> I'm going to say review+, but I think both of my comments represent
> opportunities to make this code significantly clearer. Please consider my
> suggestions.

Thanks. I agree, your comments make the code much better. Will do and commit shortly.
Comment 4 Ojan Vafai 2010-03-26 13:19:24 PDT
Committed r56639: <http://trac.webkit.org/changeset/56639>