Bug 36542 – shift+click on an existing selection doesn't work right

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

[Ojan Vafai]

Given the following text:
Here is line one.
This is line two.

1. Select all
2. shift+click after the word "here"
3. shift+click before the word "two"
Expected result: The latter half of line one and the first half of line two should be selected.
Actual result: The whole first line and the first half of line two are selected.

The correct Mac behavior is to shrink whichever end of the selection is closest to where you clicked. Windows/Linux have a different expected behavior (the extent of the selection should always be moved on shift+click) that is addressed by bug 25195.

[Ojan Vafai]

Created attachment 55160 [details]
Patch

[Darin Adler]

Comment on attachment 55160 [details]
Patch

> +            // See <rdar://problem/3668157> REGRESSION (Mail): shift-click deselects when selection
> +            // was created right-to-left
> +            Position start = newSelection.start();
> +            Position end = newSelection.end();
> +            int distanceToStart = textDistance(start, pos);
> +            int distanceToEnd = textDistance(pos, end);
> +            if (distanceToStart < distanceToEnd)
> +                newSelection = VisibleSelection(pos, end);
> +            else
> +                newSelection = VisibleSelection(start, pos);

Thanks for working so hard to get the Mac behavior. But I suspect this is still not quite right. I think it's important that the end that moved due to the click is the "extent", so that shift-arrow keys modify that side of the selection. I believe that means we should have VisibleSelection(end, pos) rather than VisibleSelection(pos, end).

I'd like to see a test case that covers that part of the behavior as well. I'm going to say r=me on this as-is, but I hope you go the extra mile and make the refinement I mention above.

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 55160 [details])
> > +            // See <rdar://problem/3668157> REGRESSION (Mail): shift-click deselects when selection
> > +            // was created right-to-left
> > +            Position start = newSelection.start();
> > +            Position end = newSelection.end();
> > +            int distanceToStart = textDistance(start, pos);
> > +            int distanceToEnd = textDistance(pos, end);
> > +            if (distanceToStart < distanceToEnd)
> > +                newSelection = VisibleSelection(pos, end);
> > +            else
> > +                newSelection = VisibleSelection(start, pos);
> 
> Thanks for working so hard to get the Mac behavior. But I suspect this is still
> not quite right. I think it's important that the end that moved due to the
> click is the "extent", so that shift-arrow keys modify that side of the
> selection. I believe that means we should have VisibleSelection(end, pos)
> rather than VisibleSelection(pos, end).
> 
> I'd like to see a test case that covers that part of the behavior as well. I'm
> going to say r=me on this as-is, but I hope you go the extra mile and make the
> refinement I mention above.

Hmm, it also seems like the behavior in NSTextView is:

    if (distanceToStart <= distanceToEnd)

because it seems to move the start of the selection, not the end, when there's a tie.

[Darin Adler]

(In reply to comment #0)
> The correct Mac behavior is to shrink whichever end of the selection is closest
> to where you clicked. Windows/Linux have a different expected behavior (the
> extent of the selection should always be moved on shift+click) that is
> addressed by bug 25195.

You say “shrink” specifically here, but isn't this rule and this code also used to extend selections?

[Ojan Vafai]

Thanks for the quick review!

> > +            if (distanceToStart < distanceToEnd)
> > +                newSelection = VisibleSelection(pos, end);
> > +            else
> > +                newSelection = VisibleSelection(start, pos);
> 
> I think it's important that the end that moved due to the
> click is the "extent", so that shift-arrow keys modify that side of the
> selection. I believe that means we should have VisibleSelection(end, pos)
> rather than VisibleSelection(pos, end).

I totally meant to do that. Just lost track of it. 

> I'd like to see a test case that covers that part of the behavior as well. I'm
> going to say r=me on this as-is, but I hope you go the extra mile and make the
> refinement I mention above.

np. I'll extend the test to check that the anchor/focus of the selection is correct after each shift-click.

> Hmm, it also seems like the behavior in NSTextView is:
> 
>     if (distanceToStart <= distanceToEnd)
> 
> because it seems to move the start of the selection, not the end, when there's
> a tie.

Good catch. Will fix.

(In reply to comment #4)
> (In reply to comment #0)
> > The correct Mac behavior is to shrink whichever end of the selection is closest
> > to where you clicked. Windows/Linux have a different expected behavior (the
> > extent of the selection should always be moved on shift+click) that is
> > addressed by bug 25195.
> 
> You say “shrink” specifically here, but isn't this rule and this code also used
> to extend selections?

Will fix.

[Ojan Vafai]

> > > The correct Mac behavior is to shrink whichever end of the selection is closest
> > > to where you clicked. Windows/Linux have a different expected behavior (the
> > > extent of the selection should always be moved on shift+click) that is
> > > addressed by bug 25195.
> > 
> > You say “shrink” specifically here, but isn't this rule and this code also used
> > to extend selections?
> 
> Will fix.

Hm. I thought this was my changelog description. :) Yes, I was wrong when I originally wrote the bug report. The correct NSTextView behavior is to move whichever end of the selection is closest to the shift+click.

[Ojan Vafai]

Created attachment 55186 [details]
Patch

[Ojan Vafai]

Looks like there's a case where we get the directionality wrong on win/linux. It's a rare edge case though and non-trivial to fix, so I'm going to land as is (once the bots are more caught up) and file a bug for that case.

[Ojan Vafai]

Committed r58892: <http://trac.webkit.org/changeset/58892>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58892 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58891
http://trac.webkit.org/changeset/58892