Bug 41975 – SelectionController::modify should ask EditingBehavior for platform specific behavior

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

[Antonio Gomes]

... instead, it is accessing EditingMacBehavior directly today.

[Antonio Gomes]

Finally back on it. 

So, this bug is about the following snippet:

669	        case EXTEND: 
670	 	            setExtent(pos, userTriggered); 
671	 	            break; 
681	            if (!settings || settings->editingBehavior() != EditingMacBehavior || m_selection.isCaret() || !isBoundary(granularity)) 
682	                setExtent(position, userTriggered); 
683	            else { 
684	                // Standard Mac behavior when extending to a boundary is grow the selection rather 
685	                // than leaving the base in place and moving the extent. Matches NSTextView. 
686	                if (direction == FORWARD || direction == RIGHT) 
687	                    setEnd(position, userTriggered); 
688	                else 
689	                    setStart(position, userTriggered); 
690	            } 

I tried to come up with a name for this specific editing behavior based on the comment. It was: 

bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }

... and Darin asked "If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?"

Better suggestions?

[Ojan Vafai]

Maybe shouldAlwaysGrowSelectionWhenExtendingToBoundary? The thing with win/linux is that they sometimes grow, sometimes shrink, but on mac it always grows.

Another option would be to name it based on the win/linux behavior. Something like shouldAlwaysMoveExtentWhenExtendingToBoundary. I think the former is better.

[Antonio Gomes]

Created attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

Based on Ojan's naming suggestion.

[Ojan Vafai]

Comment on attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

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

> WebCore/editing/SelectionController.cpp:688
> -        if (!settings || settings->editingBehaviorType() != EditingMacBehavior || 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.
> +        // 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.

I don't think it's beneficial to copy this comment in two places. It just makes it more likely that they'll get out of sync. I'd just leave the comment in EditingBehavior.h and remove this one.

> WebCore/editing/SelectionController.cpp:695
> -        if (!settings || settings->editingBehaviorType() != EditingMacBehavior || 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.
> +        // 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 (m_frame->editor()->behavior().shouldAlwaysGrowSelectionWhenExtendingToBoundary() && !m_selection.isCaret() && isBoundary(granularity)) {
>              if (direction == DirectionForward || direction == DirectionRight)
>                  setEnd(position, userTriggered);
>              else
>                  setStart(position, userTriggered);
> -        }
> +        } else 
> +            setExtent(position, userTriggered);

Why reverse the order of this if-else? I think the other order is better. Having single line else clauses with multi-line if clauses makes for ugly. :) In fact, I believe Darin gave me the same feedback when I originally wrote this patch this way.

[Antonio Gomes]

Comment on attachment 68053 [details]
(committed with r67909, r=ojan) patch v1

Clearing flags on attachment: 68053

Committed r67909 <http://trac.webkit.org/changeset/67909>