Bug 36539 – shift+home/end and cmd+shift+left/right don't extend the selection correctly

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

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 51683 [details]
Patch

[Darin Adler]

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.

[Ojan Vafai]

(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.

[Ojan Vafai]

Committed r56639: <http://trac.webkit.org/changeset/56639>