Bug 98173 – Allow EventHandler to handle longpress gestures, including longpress selection on Android.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oli Lan]

[chromium] Allow long press to select word on Android.

[Oli Lan]

Created attachment 166706 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 166706 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:4077
> +    FrameSelection* fs = focusedFrame->selection();

Please don't use abbreviations like fs.

> Source/WebKit/chromium/src/WebViewImpl.cpp:4081
> +    if (node->renderer() && !fs->contains(point) && (node->isContentEditable() || node->isTextNode()) && !result.isLiveLink()
> +        && node->dispatchEvent(Event::create(eventNames().selectstartEvent, true, true))) {
> +        VisiblePosition pos(node->renderer()->positionForPoint(result.localPoint()));
> +        WebFrameImpl::selectWordAroundPosition(focusedFrame, pos);

This code looks fishy to me. Why isn't this in EventHandler.cpp?
In particular, we should be using canMouseDownStartSelect and share most of code with selectClosestWordFromMouseEvent.

[Oli Lan]

Created attachment 166892 [details]
Patch

[Oli Lan]

Thanks for the review. The second patch changes this to be implemented fully within EventHandler. The code to perform the selection is now shared as much as possible with selectClosestWordFromMouseEvent.

I did not call canMouseDownStartSelect for this as that seems to be concerned with preventing mouse drags from starting selection when there is a draggable element.

[WebKit Review Bot]

Comment on attachment 166892 [details]
Patch

Attachment 166892 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14131721

New failing tests:
fast/events/touch/gesture/context-menu-on-two-finger-tap.html

[Oli Lan]

Created attachment 166915 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 166915 [details]
Patch

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

Excellent! This patch looks much, much better.

> Source/WebCore/page/EventHandler.cpp:455
> +        selectClosestWordFromHitTestResult(result.hitTestResult(), (result.event().clickCount() == 2 && m_frame->editor()->isSelectTrailingWhitespaceEnabled()) ? ShouldAppendTrailingWhitespace : DontAppendTrailingWhitespace);

This is a really long time. I would split into two lines (make sure to add curly brackets around the if statement in that case).

[WebKit Review Bot]

Comment on attachment 166915 [details]
Patch

Attachment 166915 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14118975

New failing tests:
fast/events/touch/gesture/context-menu-on-two-finger-tap.html

[Robert Kroeger]

Comment on attachment 166915 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2523
> +        return handleGestureLongPress(gestureEvent);

why do this here instead of in WebViewImpl? Am curious.

> Source/WebKit/chromium/src/WebViewImpl.cpp:-732
> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);

As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.

[Ryosuke Niwa]

Comment on attachment 166915 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:732
> +        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);

Oh oops, this is going to fire a gesture event :(

[Oli Lan]

Comment on attachment 166915 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732
>> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
> 
> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.

"Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap.

The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that.

This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:732
>> +        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);
> 
> Oh oops, this is going to fire a gesture event :(

This will have the effect that the longpress gesture goes to the node in the same way that a tap gesture already does. If it's not swallowed, then the result will be the same as before, in that sendContextMenuEventForGesture will be called. Is that not ok?

[Varun Jain]

Comment on attachment 166915 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732
>>> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
>> 
>> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.
> 
> "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap.
> 
> The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that.
> 
> This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only.

I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown.

[Oli Lan]

Comment on attachment 166915 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732
>>>> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
>>> 
>>> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.
>> 
>> "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap.
>> 
>> The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that.
>> 
>> This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only.
> 
> I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown.

Longpress on android can show a context menu, for example on a link or an image. Selection of text only occurs if the longpress was on non-link text.

The reason for the failing test is that I didn't notice that WebInputEvent::GestureTwoFingerTap uses the same code path as GestureLongPress in WebViewImpl::handleGestureEvent, whereas EventHandler::handleGestureEvent doesn't handle two finger tap so this change disables that gesture. I will restore two finger tap in a new patch.

[Oli Lan]

Created attachment 167097 [details]
Patch

[Oli Lan]

The last patch restores GestureTwoFingerTap handling to how it was (except without the detectContentOnTouch call which is Android-only and shouldn't be called for a two-finger tap).

The layout test context-menu-on-two-finger-tap.html should now pass.

[Varun Jain]

(In reply to comment #13)
> (From update of attachment 166915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review
> 
> >>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732
> >>>> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
> >>> 
> >>> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.
> >> 
> >> "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap.
> >> 
> >> The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that.
> >> 
> >> This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only.
> > 
> > I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown.
> 
> Longpress on android can show a context menu, for example on a link or an image. Selection of text only occurs if the longpress was on non-link text.
> 
> The reason for the failing test is that I didn't notice that WebInputEvent::GestureTwoFingerTap uses the same code path as GestureLongPress in WebViewImpl::handleGestureEvent, whereas EventHandler::handleGestureEvent doesn't handle two finger tap so this change disables that gesture. I will restore two finger tap in a new patch.

I was not talking about the failing test at all. My concern is that for android, we are setting the state of context menu allowed to true but not showing the context menu in some cases. This seems wrong. It appears to me that a script listening to selection change will be able to invoke context menu since WebViewImpl will allow it.
FWIW, I am not a reviewer :) so you can totally ignore my comments if the reviewers agree to this.

[Ryosuke Niwa]

Comment on attachment 167097 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:726
> +        m_contextMenuAllowed = true;
> +        PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);
> +        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
> +        m_contextMenuAllowed = false;

I agree with Varun that it's strange to set m_contextMenuAllowed true and then not show the context menu.
On the other hand, I don't understand the value of having this boolean variable in the first place.

[Oli Lan]

Comment on attachment 167097 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:726
>> +        m_contextMenuAllowed = false;
> 
> I agree with Varun that it's strange to set m_contextMenuAllowed true and then not show the context menu.
> On the other hand, I don't understand the value of having this boolean variable in the first place.

m_contextMenuAllowed is only read in ContextMenuClientImpl::getCustomMenuFromDefaultItems, where the aim is to ensure that the context menu is only shown in response to user input.

I'd argue that, as longpress is a gesture that can show a context menu, it's correct for m_contextMenuAllowed to be true when a longpress is handled by the EventHandler, regardless of whether or not the default handling will show the menu.

[Robert Kroeger]

Comment on attachment 167097 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:433
> +void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& result, AppendTrailingWhitespace appendTrailingWhitespace)

this code looks a lot like WebFrameImpl::selectWordAroundPosition Is there an opportunity to remove some duplicate code especially since selectClosestWord* doesn't seem to be used anywhere else yet?

> Source/WebCore/page/EventHandler.cpp:2575
> +#if OS(ANDROID)

I'd prefer it if you added a flag or a different kind of event instead of the #ifdef.

[Ryosuke Niwa]

Comment on attachment 167097 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2575
>> +#if OS(ANDROID)
> 
> I'd prefer it if you added a flag or a different kind of event instead of the #ifdef.

In this case, ifdef is better.

[Oli Lan]

Comment on attachment 167097 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:433
>> +void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& result, AppendTrailingWhitespace appendTrailingWhitespace)
> 
> this code looks a lot like WebFrameImpl::selectWordAroundPosition Is there an opportunity to remove some duplicate code especially since selectClosestWord* doesn't seem to be used anywhere else yet?

selectClosestWordFromMouseEvent and selectClosestWordOrLinkFromMouseEvent are preexisting methods, and are used when double-clicks are handled and for selecting a word on Mac when a context menu is generated. This method selectClosestWordFromHitTestResult was added just to make the code no longer dependent on a mouse event so we could use it from a touch gesture - as you can see it mostly uses the previous code from selectClosestWordFromMouseEvent.

My first attempt at this change (patch 1) was in WebViewImpl and used selectWordAroundPosition, but on rniwa's suggestion I moved it into EventHandler. I think this it makes sense for it to be in EventHandler, seeing as it's to do with handling gesture events.

[Oli Lan]

Ryosuke (and other reviewers), what do you think of the latest version of this patch? Does it need further changes?

[Ryosuke Niwa]

Comment on attachment 167097 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:743
> -        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent);
> +        eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);

handleGestureEvent is going to fire a gesture event. Why is that okay?
If that's intended, we should have a test for that, right? Or am I misunderstanding something?

[Adam Barth]

> handleGestureEvent is going to fire a gesture event. Why is that okay?
> If that's intended, we should have a test for that, right? Or am I misunderstanding something?

I don't think we expose gesture events to the web.  We might do that in the future, depending on how the touch events / pointer events / gesture events discussion plays out in the standards world.

[Ryosuke Niwa]

Adam clarified his comment. If we look at GestureEvent::create, we see that we only instantiate the event for a subset of gestures.

[WebKit Review Bot]

Comment on attachment 167097 [details]
Patch

Clearing flags on attachment: 167097

Committed r130547: <http://trac.webkit.org/changeset/130547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.