101545 – LongPress and LongTap gestures should start drag/drop and open context menu respectively.

RESOLVED FIXED101545

LongPress and LongTap gestures should start drag/drop and open context menu respectively.

https://bugs.webkit.org/show_bug.cgi?id=101545

Summary LongPress and LongTap gestures should start drag/drop and open context menu r...

Varun Jain

Reported 2012-11-07 22:27:24 PST

LongPress and LongTap gestures should start drag/drop and open context menu respectively.

Attachments
Patch (17.02 KB, patch) 2012-11-07 22:35 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (19.20 KB, patch) 2012-11-08 07:04 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (27.07 KB, patch) 2012-11-22 15:46 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (27.03 KB, patch) 2012-11-22 16:42 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (27.00 KB, patch) 2012-11-22 20:49 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (27.22 KB, patch) 2012-11-23 11:16 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (28.30 KB, patch) 2012-11-25 16:48 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (28.55 KB, patch) 2012-11-25 16:56 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (27.91 KB, patch) 2012-11-25 18:55 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (28.09 KB, patch) 2012-11-26 15:21 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (28.07 KB, patch) 2012-11-26 15:26 PST, Varun Jain	no flags	Details Formatted Diff Diff
Patch (28.09 KB, patch) 2012-11-26 15:40 PST, Varun Jain	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Varun Jain

Comment 1 2012-11-07 22:35:00 PST

Created attachment 172935 [details] Patch

Peter Beverloo (cr-android ews)

Comment 2 2012-11-08 02:45:16 PST

Comment on attachment 172935 [details] Patch Attachment 172935 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14759690

Varun Jain

Comment 3 2012-11-08 07:04:40 PST

Created attachment 173036 [details] Patch

Varun Jain

Comment 4 2012-11-08 07:10:40 PST

(hopefully) fixed broken builds.

Antonio Gomes

Comment 5 2012-11-08 21:14:26 PST

i am confused about the difference between LongPress and LongTap. mind to clarify?

Varun Jain

Comment 6 2012-11-09 05:02:01 PST

(In reply to comment #5) > i am confused about the difference between LongPress and LongTap. mind to clarify? Hi Antonio, LongTap is just completion of a tap after a LongPress. So: LongPress = touch_pressed+wait_for_timeout LongTap = touch_pressed+wait_for_timeout+touch_released

Varun Jain

Comment 7 2012-11-12 21:42:26 PST

ping@ tonikitoo (In reply to comment #6) > (In reply to comment #5) > > i am confused about the difference between LongPress and LongTap. mind to clarify? > > Hi Antonio, LongTap is just completion of a tap after a LongPress. So: > LongPress = touch_pressed+wait_for_timeout > LongTap = touch_pressed+wait_for_timeout+touch_released

Antonio Gomes

Comment 8 2012-11-13 06:59:42 PST

Comment on attachment 173036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173036&action=review It looks reasonable. I have a request that will require another round, so r-'ing for now. > Source/WebCore/page/EventHandler.cpp:338 > + , m_didStartDrag(false) I would like this variable to be named more specific. What started a drag? m_longPressGestureDidStartDrag or something like this. > Source/WebCore/page/EventHandler.cpp:2620 > +#if !OS(ANDROID) that made me curious about what port you are testing this on... :) > Source/WebCore/page/EventHandler.cpp:3312 > +bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event, bool checkForDragHysteresis) The added bool can very much be an enum (even reuse the existing boolean naming).

Varun Jain

Comment 9 2012-11-22 15:46:01 PST

Created attachment 175713 [details] Patch

Varun Jain

Comment 10 2012-11-22 15:49:27 PST

Comment on attachment 173036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173036&action=review >> Source/WebCore/page/EventHandler.cpp:338 >> + , m_didStartDrag(false) > > I would like this variable to be named more specific. What started a drag? m_longPressGestureDidStartDrag or something like this. I would be happy to rename the variable. However, consider that this variable simply indicates whether the call to DragController::startDrag returned true (ie successfully started a drag), regardless of what event initiated the drag. >> Source/WebCore/page/EventHandler.cpp:2620 >> +#if !OS(ANDROID) > > that made me curious about what port you are testing this on... :) This is for general touch support in chrome. I have changed this a bit so that it can now be turned on or off by the embedder. PTAL. >> Source/WebCore/page/EventHandler.cpp:3312 >> +bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event, bool checkForDragHysteresis) > > The added bool can very much be an enum (even reuse the existing boolean naming). Done.

Varun Jain

Comment 11 2012-11-22 15:51:18 PST

Hi Antonio. Sorry for the delay on getting back. I got distracted with some other stuff. I have addressed your comments and changed the CL slightly so that now the embedder has control over whether to start drag on touch or not. PTAL. (In reply to comment #10) > (From update of attachment 173036 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173036&action=review > > >> Source/WebCore/page/EventHandler.cpp:338 > >> + , m_didStartDrag(false) > > > > I would like this variable to be named more specific. What started a drag? m_longPressGestureDidStartDrag or something like this. > > I would be happy to rename the variable. However, consider that this variable simply indicates whether the call to DragController::startDrag returned true (ie successfully started a drag), regardless of what event initiated the drag. > > >> Source/WebCore/page/EventHandler.cpp:2620 > >> +#if !OS(ANDROID) > > > > that made me curious about what port you are testing this on... :) > > This is for general touch support in chrome. I have changed this a bit so that it can now be turned on or off by the embedder. PTAL. > > >> Source/WebCore/page/EventHandler.cpp:3312 > >> +bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event, bool checkForDragHysteresis) > > > > The added bool can very much be an enum (even reuse the existing boolean naming). > > Done.

WebKit Review Bot

Comment 12 2012-11-22 15:53:00 PST

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Varun Jain

Comment 13 2012-11-22 16:42:45 PST

Created attachment 175717 [details] Patch

Antonio Gomes

Comment 14 2012-11-22 19:38:20 PST

Comment on attachment 175717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175717&action=review The sequence is not send a long press then a long tap, once the user lifts its finger? > Source/WebCore/page/EventHandler.cpp:2665 > + if (!gestureEvent.area().isEmpty()) There is a shouldAdjustXXX method for this check now.

Varun Jain

Comment 15 2012-11-22 20:49:26 PST

Created attachment 175730 [details] Patch

Varun Jain

Comment 16 2012-11-22 20:50:09 PST

(In reply to comment #14) > (From update of attachment 175717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175717&action=review > > The sequence is not send a long press then a long tap, once the user lifts its finger? yes... this is the sequence. > > > Source/WebCore/page/EventHandler.cpp:2665 > > + if (!gestureEvent.area().isEmpty()) > > There is a shouldAdjustXXX method for this check now. Done.

Antonio Gomes

Comment 17 2012-11-23 04:47:39 PST

(In reply to comment #16) > (In reply to comment #14) > > (From update of attachment 175717 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175717&action=review > > > > The sequence is not send a long press then a long tap, once the user lifts its finger? > > yes... this is the sequence. Right, so my point was: if "long press" triggers context menu, and "long tap" would happen after a "long press", would not the context menu be already triggered by "long press" when "long tap" happens?

Varun Jain

Comment 18 2012-11-23 09:57:19 PST

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > (From update of attachment 175717 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=175717&action=review > > > > > > The sequence is not send a long press then a long tap, once the user lifts its finger? > > > > yes... this is the sequence. > > Right, so my point was: if "long press" triggers context menu, and "long tap" would happen after a "long press", would not the context menu be already triggered by "long press" when "long tap" happens? Typically, if a context menu shows up, the context menu window takes capture and eats all events. So no long tap is received by the EH. But if you'd prefer, I can check for an already invoked context menu in handleGestureLongTap().

Antonio Gomes

Comment 19 2012-11-23 10:16:30 PST

> > > > Right, so my point was: if "long press" triggers context menu, and "long tap" would happen after a "long press", would not the context menu be already triggered by "long press" when "long tap" happens? > > Typically, if a context menu shows up, the context menu window takes capture and eats all events. So no long tap is received by the EH. But if you'd prefer, I can check for an already invoked context menu in handleGestureLongTap(). So we can have cases where context menu wont be shown by long press, but by long tap instead? I would prefer to check if context menu is already invoked by ling press first, yes.

Varun Jain

Comment 20 2012-11-23 11:16:05 PST

Created attachment 175829 [details] Patch

Varun Jain

Comment 21 2012-11-23 11:19:08 PST

(In reply to comment #19) > > > > > > Right, so my point was: if "long press" triggers context menu, and "long tap" would happen after a "long press", would not the context menu be already triggered by "long press" when "long tap" happens? > > > > Typically, if a context menu shows up, the context menu window takes capture and eats all events. So no long tap is received by the EH. But if you'd prefer, I can check for an already invoked context menu in handleGestureLongTap(). > > So we can have cases where context menu wont be shown by long press, but by long tap instead? > Yes. If a long press happens on a draggable element, it starts a drag and context menu is show if the user does not drag but just lifts up their finger (trigerring a long tap). If, however, the long press happens over an element that does not trigger drag (such as unselected text), then we show the context menu right away instead of waiting for the long tap. I have documented this behavior in the changelog. > I would prefer to check if context menu is already invoked by ling press first, yes. Done.

Antonio Gomes

Comment 22 2012-11-23 12:01:29 PST

Comment on attachment 175829 [details] Patch <tonikitoo> what is the difference between m_didStartDrag and the return value or ::handleDrag? [15:51] <varunjain> I think handleDrag is very inclusive.. it returns false only if the event is not a left mouse button drag... it does not necessarily return true only when a drag was actually started [15:53] <tonikitoo> so it might return TRUE even if dragging actually did not start [15:53] <tonikitoo> ? [15:58] <varunjain> yes

Varun Jain

Comment 23 2012-11-23 15:43:15 PST

Thanks for the review Antonio. Will wait for abarth to review the WebKit API changes.

Adam Barth

Comment 24 2012-11-24 18:13:20 PST

Comment on attachment 175829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175829&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:3209 > +void WebViewImpl::enableTouchDragDrop(bool enable) > +{ > + if (mainFrameImpl() && mainFrameImpl()->frame()) > + mainFrameImpl()->frame()->eventHandler()->enableTouchDragDrop(enable); > +} This appears to be per-frame state in WebCore. Should the API be on WebFrame rather than WebView?

Adam Barth

Comment 25 2012-11-24 18:14:07 PST

Comment on attachment 175829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175829&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:3209 >> +} > > This appears to be per-frame state in WebCore. Should the API be on WebFrame rather than WebView? Another question: why isn't this a WebCore::Setting (in which case the API would be on WebSettings)?

Varun Jain

Comment 26 2012-11-25 16:48:43 PST

Created attachment 175897 [details] Patch

Varun Jain

Comment 27 2012-11-25 16:50:06 PST

(In reply to comment #25) > (From update of attachment 175829 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175829&action=review > > >> Source/WebKit/chromium/src/WebViewImpl.cpp:3209 > >> +} > > > > This appears to be per-frame state in WebCore. Should the API be on WebFrame rather than WebView? > > Another question: why isn't this a WebCore::Setting (in which case the API would be on WebSettings)? Agree. Moved to WebSettings.

Varun Jain

Comment 28 2012-11-25 16:56:06 PST

Created attachment 175899 [details] Patch

Antonio Gomes

Comment 29 2012-11-25 18:22:52 PST

Comment on attachment 175899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175899&action=review > Source/WebCore/page/EventHandler.cpp:2658 > +#if ENABLE(DRAG_SUPPORT) > + if (m_frame->settings()->touchDragDropEnabled()) { Did you add an explanation to the changelog? I remember you mentioning something about a "developers' menu" that would allow users to toggle it on/off? also does "touch drag&drop" means: use the same code that handles drag&drop for mouse events, but with touch (gesture) events? > Source/WebCore/page/Settings.h:299 > + void setTouchDragDropEnabled(bool enabled) { m_touchDragDropEnabled = enabled; } > + bool touchDragDropEnabled() { return m_touchDragDropEnabled; } do not we have Settings.in to be modified instead now? > Source/WebCore/page/Settings.h:376 > + bool m_touchDragDropEnabled; why not bitfy it like the other bools?

Varun Jain

Comment 30 2012-11-25 18:55:58 PST

Created attachment 175912 [details] Patch

Varun Jain

Comment 31 2012-11-25 19:01:48 PST

(In reply to comment #29) > (From update of attachment 175899 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175899&action=review > > > Source/WebCore/page/EventHandler.cpp:2658 > > +#if ENABLE(DRAG_SUPPORT) > > + if (m_frame->settings()->touchDragDropEnabled()) { > > Did you add an explanation to the changelog? I remember you mentioning something about a "developers' menu" that would allow users to toggle it on/off? > Updated Changelog. Its up to the platform to enable/disable the setting using whatever method they choose. > also does "touch drag&drop" means: use the same code that handles drag&drop for mouse events, but with touch (gesture) events? Yes. The Changelog mentions that the drag/drop is initiated using fake mouse events. > > > Source/WebCore/page/Settings.h:299 > > + void setTouchDragDropEnabled(bool enabled) { m_touchDragDropEnabled = enabled; } > > + bool touchDragDropEnabled() { return m_touchDragDropEnabled; } > > do not we have Settings.in to be modified instead now? Done. I was not aware of this. Thanks for pointing out. > > > Source/WebCore/page/Settings.h:376 > > + bool m_touchDragDropEnabled; > > why not bitfy it like the other bools? Not necessary now that its defined in Settings.in

Varun Jain

Comment 32 2012-11-25 19:02:48 PST

wha!! lightning fast review :P I could not even finish replying to comments! Thanks! :)

Adam Barth

Comment 33 2012-11-26 15:04:06 PST

Comment on attachment 175912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175912&action=review > Source/WebCore/page/EventHandler.cpp:2658 > + if (m_frame->settings()->touchDragDropEnabled()) { Can Settings be 0 here?

Adam Barth

Comment 34 2012-11-26 15:05:14 PST

Comment on attachment 175912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175912&action=review > Source/WebKit/chromium/src/WebSettingsImpl.h:167 > virtual void setXSSAuditorEnabled(bool); > + virtual void setTouchDragDropEnabled(bool); This should actually be alphabetical.

Varun Jain

Comment 35 2012-11-26 15:21:29 PST

Created attachment 176086 [details] Patch

Varun Jain

Comment 36 2012-11-26 15:26:06 PST

Created attachment 176089 [details] Patch

Varun Jain

Comment 37 2012-11-26 15:33:53 PST

(In reply to comment #33) > (From update of attachment 175912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175912&action=review > > > Source/WebCore/page/EventHandler.cpp:2658 > > + if (m_frame->settings()->touchDragDropEnabled()) { > > Can Settings be 0 here? Done. Also fixed the ordering mentioned in next comment.

Adam Barth

Comment 38 2012-11-26 15:37:04 PST

Comment on attachment 176089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176089&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by Adam Barth. This was actually reviewed by tonikitoo

Varun Jain

Comment 39 2012-11-26 15:40:32 PST

Created attachment 176095 [details] Patch

WebKit Review Bot

Comment 40 2012-11-26 16:57:17 PST

Comment on attachment 176095 [details] Patch Clearing flags on attachment: 176095 Committed r135789: <http://trac.webkit.org/changeset/135789>

WebKit Review Bot

Comment 41 2012-11-26 16:57:23 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Varun Jain

Reported

2012-11-07 22:27 PST

Modified

2012-11-26 16:57 PST History

CC List

11 users Show

URL

Keywords

Depends on

Blocks