Selecting with shift+drag results in unexpected drag-n-drop

• Component: UI Events
• Status: RESOLVED FIXED

[Antonio Gomes]

1) Load a page with some text content.
2) Select some text with mouse.
3) Press SHIFT and do a mouse-down on a text elsewhere on the page.
   - original selection in (2) gets extended to the click point in (3).
4) without releasing the SHIFT key the mouse (left) button, start moving the mouse to select more text.

Current result:
- Drag and drop gets triggered.

Expected result (Firefox / newest IE / Opera-Presto)
- Selection continues to get extended 'till mouse left button is released.

[Antonio Gomes]

Created attachment 273593 [details]
For EWS.

[Antonio Gomes]

Created attachment 273677 [details]
Patch v1

[Antonio Gomes]

CC'ing some people that might get interested on this small, but useful change.

What do you think guys?

[Beth Dakin]

(In reply to comment #3)
> CC'ing some people that might get interested on this small, but useful
> change.
> 
> What do you think guys?

This new behavior actually appears to match AppKit, so I think it would be a nice change on the Mac. What do others think?

[Darin Adler]

I’m not sure the new behavior matches AppKit. I tried dragging some selected text in TextEdit with the shift key held down and the drag worked. Maybe I don’t understand what case we are talking about here?

[Antonio Gomes]

(In reply to comment #5)
> I’m not sure the new behavior matches AppKit. I tried dragging some selected
> text in TextEdit with the shift key held down and the drag worked. Maybe I
> don’t understand what case we are talking about here?

Hi Darin. Bug is not about dragging some selected text.

It is when you have an existing selection, and wants to expand that selection.
For example:

1) We have an original selection from "a" to "x" in the first word below.

|--original selection--|   
abcdefghijklmnopqrstuvwxyz abcdefghijklmnopqrstuvwxyz

2) user wants to expand the selection to some content in the second word. To do that he moves the mouse say over the "e" on the second word, and left click with shift key pressed.

3) At this point selection expands to the second "e", like below:
|--------changed selection-----|   
abcdefghijklmnopqrstuvwxyz abcdefghijklmnopqrstuvwxyz

4) Without releasing the mouse left button, if wants to continue selecting more text with the mouse, so he moves the mouse to say over "y" in the second word.

In WebKit/Safari, (4) is not possible, whereas it is in Firefox, latest IE, Opera (Presto), and soon Chrome.
What happens instead is that when user preforms (4), drag-n-drop gets triggered.

Patch tries to fix this.

[Beth Dakin]

Hey Darin,

In Antonio's 4 steps above, this part in step 4 is key:

"Without releasing the mouse left button."

[Darin Adler]

OK, I agree we want this behavior change.

In fact, I was able to reproduce the incorrect behavior that this patch fixes in the OS X Mail app when selecting text in a received message.

[Darin Adler]

Comment on attachment 273677 [details]
Patch v1

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

> LayoutTests/editing/selection/shift-drag-selection-no-drag-n-drop.html:4
> +<p>This test ensures drag-n-drop is not started when selecting with shilt + mouse click.

Typo: shilt

> Source/WebCore/ChangeLog:13
> +        Whenever user tries to extend an existing text selection by dragging the mouse (left button hold)
> +        with shift key pressed, WebKit enters drag-n-drop mode.
> +        This behavior differs from Firefox, Opera (presto) and IE.
> +        Patch changes WebKit so that it matches other browsers here.

I think it’s more than just matching other browsers. It’s about matching normal editing behavior outside of browsers too. This just seems like a straight up bug. Fine to mention the other browsers, but also fine not to. Just seems like an almost obvious type of coding mistake.

> Source/WebCore/ChangeLog:18
> +        Note: Chromium is the process of changing its WebKit-derivated behavior as per [1] and [2].
> +
> +        [1] http://bit.ly/1RbbAPR
> +        [2] https://bugs.chromium.org/p/chromium/issues/detail?id=142023

Not sure we need this note about Chromium in the change log. Seems sufficient to have it in the bug in bugs.webkit.org. If we do keep it, then “derivated” should not be used, it’s not a word. Could say “derived” I suppose.

> Source/WebCore/page/EventHandler.cpp:763
> +    m_mouseDownMayStartDrag = singleClick && !event.event().shiftKey();

The one thing I don’t like here is the this code seems completely separate from the other places that check shiftKey. It would be clearer if these called functions that were about the purpose of the event: “extends selection” or something like that, rather than just directly checking shiftKey at each call site.

Another way to think of this is that this logic matches the extendSelection check in handleMousePressEventSingleClick, but for some reason it doesn’t check “over link” the way that code does. Is that difference correct or incorrect? Do we need another test to cover that case?

A third consideration is that this comment says that we need to match eventMayStartDrag, but eventMayStartDrag does not check the shift key. The comment is, at least, hard to understand since the logic no longer matches. And maybe points to a problem.

Not clear why that’s all OK.

Seems likely the code works correctly, tests are the best proof of that, but the policy here is quite scattered and disorganized.

[Antonio Gomes]

(In reply to comment #9)
> Comment on attachment 273677 [details]

> The one thing I don’t like here is the this code seems completely separate
> from the other places that check shiftKey. It would be clearer if these
> called functions that were about the purpose of the event: “extends
> selection” or something like that, rather than just directly checking
> shiftKey at each call site.
> 
> Another way to think of this is that this logic matches the extendSelection
> check in handleMousePressEventSingleClick, but for some reason it doesn’t
> check “over link” the way that code does. Is that difference correct or
> incorrect? Do we need another test to cover that case?

This is a great point. As is, patch actually changes the Safari behavior in case user starts to extend the selection with mouse+click+shift from a link. This is not the scope of this patch, and any link-related behavior change should be discussed on its own.
That said, I will make sure that the link-related behavior is unaffected by this patch.

I will propose a follow up patcg that takes the link scenario into account, so we can discuss further.

> A third consideration is that this comment says that we need to match
> eventMayStartDrag, but eventMayStartDrag does not check the shift key. The
> comment is, at least, hard to understand since the logic no longer matches.
> And maybe points to a problem.

I also found this commit intriguing because it makes no much sense to me. I will dig further on this before committing.

> Seems likely the code works correctly, tests are the best proof of that, but
> the policy here is quite scattered and disorganized.

The lack of test that cover this behavior is worrisome to me too. We should improve it.

[Antonio Gomes]

Created attachment 273891 [details]
Patch v2

[Antonio Gomes]

(In reply to comment #11)
> Created attachment 273891 [details]
> Patch v2

Patch v2 aims at addressing some of Darin's remarks in comment #9:

- more test coverage was added, specially involving images and links.
It is not the intention of this patch to change drag-n-drop triggers when dragging starts off of a link or an image.
This was an uncovered scenario by our test suite, so patch fills this gap.

- filed the follow up bug 155390 (referred in the code) , in order to ensure that the new logic added by attachment 273891 [details] is kept in sync with the existing logic in EventHandler::eventMayStartDrag.
I will take care of it myself.

- improved changelogs as per Darin's comment.

[Antonio Gomes]

Created attachment 273893 [details]
Patch v2.1

Patch v2.1 with oroper expected results.

[Antonio Gomes]

Created attachment 273894 [details]
PAtch v2.2 - fixed iOS build

[Build Bot]

Comment on attachment 273894 [details]
PAtch v2.2 - fixed iOS build

Attachment 273894 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/971934

New failing tests:
editing/selection/shift-drag-selection-on-link-triggers-drag-n-drop.html
editing/selection/shift-drag-selection-on-image-triggers-drag-n-drop.html

[Build Bot]

Created attachment 273895 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Antonio Gomes]

Created attachment 273916 [details]
PAtch v2.3 - fixed iOS sim EWS

Hi Darin. I have made a some changes to the r+'ed patch, explained in more detail in comment #12.

Feel free to look again (I have set r? flag). If you do not disagree, I will commit tomorrow.

[Darin Adler]

Comment on attachment 273916 [details]
PAtch v2.3 - fixed iOS sim EWS

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

Given that we have more test coverage, we likely have behavior that is now more correct than before. But it’s annoying that this check for shiftKey and isOverLink does not share a helper function with handleMousePressEventSingleClick. These separate functions in the event handler class making separate decisions that just happen to match and work together is not as good as organizing the code so the decision making is done by shared functions that always make the same decisions.

> Source/WebCore/page/EventHandler.cpp:738
> +static bool alwaysStartDragAndDropFor(const MouseEventWithHitTestResults& event)

I don’t think the word “always” here makes it clear that we are specifically talking about cases where we drag even if the shift key is down. I would leave out the word "For" from the function name regardless.

I would be tempted to instead put the shift key check and even the single click check inside this function too; separating these make it less clear how everything relates.

We discussed the behavior change when over a link, but not the behavior change when over an image.

[Darin Adler]

Comment on attachment 273916 [details]
PAtch v2.3 - fixed iOS sim EWS

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

>> Source/WebCore/page/EventHandler.cpp:738
>> +static bool alwaysStartDragAndDropFor(const MouseEventWithHitTestResults& event)
> 
> I don’t think the word “always” here makes it clear that we are specifically talking about cases where we drag even if the shift key is down. I would leave out the word "For" from the function name regardless.
> 
> I would be tempted to instead put the shift key check and even the single click check inside this function too; separating these make it less clear how everything relates.
> 
> We discussed the behavior change when over a link, but not the behavior change when over an image.

Sorry, I do think the behavior when over an image is correct. Didn’t mean to leave that lest sentence on my comment.

[Antonio Gomes]

Committed r198163: <https://trac.webkit.org/r198163> .

[Antonio Gomes]

(In reply to comment #18)
> Comment on attachment 273916 [details]
> PAtch v2.3 - fixed iOS sim EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273916&action=review
>
> (...)
> 
> I would be tempted to instead put the shift key check and even the single
> click check inside this function too; separating these make it less clear
> how everything relates.
> 
> We discussed the behavior change when over a link, but not the behavior
> change when over an image.

Be sure I will fix this inconsistencies in bug 155390, Darin.

Thanks for the reviews!