Bug 155390 – eventMayStartDrag() does not check for shiftKey or isOverLink

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Antonio Gomes]

This is a follow up of Darin's comment in https://bugs.webkit.org/show_bug.cgi?id=155314#c9.

Basically in bug 155314, in order to make shift+click not trigger drag-n-drop when extending an existing selection, we changed the way m_mouseDownMayStartDrag is set on EventHandler::handleMousePressEvent, as shown below:

-    m_mouseDownMayStartDrag = singleClick;
+    m_mouseDownMayStartDrag = singleClick && (!event.event().shiftKey() || event.isOverLink());

Although makes a desired change in WebKit's editing behavior (as per [1] and [2]), it makes this setter code get out of sync with eventMayStartDrag.

Bug is about investigating the implications, and sync both pieces up again.

[1] https://bugs.webkit.org/show_bug.cgi?id=155314#c4
[2] https://bugs.webkit.org/show_bug.cgi?id=155314#c8

[Antonio Gomes]

Relevant commit:

commit ad24ce63d491cafd164c9a233aef4b7ad6deedba
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Tue Apr 12 23:51:59 2011 +0000

    Implement non-activating clicks to allow dragging out of a background window.
    https://bugs.webkit.org/show_bug.cgi?id=55053
    <rdar://problem/9042197>
    
    Source/WebCore: (...)

[Antonio Gomes]

Created attachment 273892 [details]
Patch v2.1

Proper expected results.

[Antonio Gomes]

Created attachment 275206 [details]
Patch v1.

[Antonio Gomes]

Created attachment 275207 [details]
Patch v1.1 - fix iOS build.

[Antonio Gomes]

Created attachment 275265 [details]
Patch v1.2 - fix iOS build.

[Darin Adler]

Comment on attachment 275265 [details]
Patch v1.2 - fix iOS build.

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

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

It’s not good to have a function that returns a boolean be named with the verb "check". I think this should be named "eventShouldTriggerDragging".

> Source/WebCore/page/EventHandler.cpp:743
> +    PlatformMouseEvent platformEvent = event.event();

I don’t think we want to copy the platform event here. If event() returns a reference then we should put the result into a reference and not copy it. I’d use the type auto&.

I kind of hate the word platform in the name of this local variable.

> Source/WebCore/page/EventHandler.cpp:747
> +    // Single mouse down on links or images can always trigger drag-n-drop.

It’s a little strange to have the words "single mouse down" here *after* the code that checks for single mouse down. It would be nicer to have the code say what the comment says. That could mean, for example, a helper function called isSingleMouseDownOnLinkOrImage. The words used in the comment are often a clue to what great names for the actual code would be.

[Antonio Gomes]

Created attachment 275301 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 275301 [details] did not pass style-queue:


ERROR: Source/WebCore/page/EventHandler.cpp:752:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 275302 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 275302 [details]
Patch for landing.

Clearing flags on attachment: 275302

Committed r198909: <http://trac.webkit.org/changeset/198909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

This was reopened because it was rolled out in http://trac.webkit.org/changeset/199317
When double-clicking and dragging text to highlight (such as on this very bugzilla page), this patch made it so we would drag the text instead of highlighting more.

[Darin Adler]

Before landing this again we should make a test case for the "double click and drag" case.

[Antonio Gomes]

(In reply to comment #12)
> This was reopened because it was rolled out in
> http://trac.webkit.org/changeset/199317
> When double-clicking and dragging text to highlight (such as on this very
> bugzilla page), this patch made it so we would drag the text instead of
> highlighting more.

Easily reproducible here too.

I will make sure new patch will be accompanied by a layout test for the double click + drag = select (not drag'n drop) case.

[Antonio Gomes]

Prior to bug 155314, the "can event start drag" logic in  EventHandler::handleMousePressEvent was not taking links and images explicitly into account. We fixed this as part of bug 155314, by making it an explicit check and adding tests.

Then, we filed this bug here (bug 155390) to pair up the logics EventHandler::handleMousePressEvent and EventHandler::eventMayStartDrag with regards to taking images and links into account to start dragging.

The point is, after understanding the situation a bit better...

By the time bug 155314 was fixed, we actually did pair up the logic in EventHandler::handleMousePressEvent and EventHandler::eventMayStartDrag.
This is because the ::eventMayStartDrag already calls DragController::draggableElement which does take links and images into account to trigger dragging.

Last, about the 'shiftKey' check that happens in EventHandler::eventMayStartDrag but does not happen in EventHandler::eventMayStartDrag, IMO it does not make sense for now to add it to the later, because of the how/why ::eventMayStartDrag method is used.

Current ::eventMayStartDrag is called by WebPage (see Source/WebKit2//WebProcess/WebPage/mac/WebPageMac.mm) in order to delay bringing a background host window foreground. It currently only affects dragging a selected content, and I do not think this bug should change this behavior.

I believe this bug is non-actionable. Thus, WONTFIX.