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

• Component: New Bugs
• Status: RESOLVED FIXED

[Varun Jain]

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

[Varun Jain]

Created attachment 172935 [details]
Patch

[Peter Beverloo (cr-android ews)]

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]

Created attachment 173036 [details]
Patch

[Varun Jain]

(hopefully) fixed broken builds.

[Antonio Gomes]

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

[Varun Jain]

(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]

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 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]

Created attachment 175713 [details]
Patch

[Varun Jain]

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]

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]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Varun Jain]

Created attachment 175717 [details]
Patch

[Antonio Gomes]

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]

Created attachment 175730 [details]
Patch

[Varun Jain]

(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]

(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]

(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]

> > 
> > 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]

Created attachment 175829 [details]
Patch

[Varun Jain]

(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 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]

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

[Adam Barth]

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 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]

Created attachment 175897 [details]
Patch

[Varun Jain]

(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]

Created attachment 175899 [details]
Patch

[Antonio Gomes]

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]

Created attachment 175912 [details]
Patch

[Varun Jain]

(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]

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

[Adam Barth]

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 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]

Created attachment 176086 [details]
Patch

[Varun Jain]

Created attachment 176089 [details]
Patch

[Varun Jain]

(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 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]

Created attachment 176095 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176095 [details]
Patch

Clearing flags on attachment: 176095

Committed r135789: <http://trac.webkit.org/changeset/135789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.