Bug 94727 - Content detection should not disrupt the page behaviour
Summary: Content detection should not disrupt the page behaviour
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-08-22 10:48 PDT by Leandro Graciá Gil
Modified: 2012-08-29 09:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.22 KB, patch)
2012-08-22 10:50 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (14.16 KB, patch)
2012-08-28 11:54 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch for landing (14.01 KB, patch)
2012-08-28 16:54 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2012-08-22 10:48:27 PDT
The content detection feature, which requests the embedder to look for content around a given position tapped by the user, should not interfere with the expected behaviour of the page in terms of management of click, touch and mouse events. Since detecting content might fire Android intents that launch other apps and therefore disrupt browsing, we need to make sure we don't perform content detection if the page is expecting to manage the taps starting it.

At the same time we found during testing that many pages introduce a click listener on the <body> element, which we never found to be related to the content in any way. We want to keep allowing content detection in that special case.
Comment 1 Leandro Graciá Gil 2012-08-22 10:50:43 PDT
Created attachment 159964 [details]
Patch
Comment 2 Leandro Graciá Gil 2012-08-22 10:52:58 PDT
This is what we last had when previously discussing this. Adam, you were suggesting before to move this to another place, possibly the default event handler. How would that affect to detecting event listeners while still allowing content detection to happen for the <body> case?

I'm not sure I fully understood your approach or if the page would need to add any kind of preventDefault. Could you elaborate a bit, please?
Comment 3 Adam Barth 2012-08-22 12:38:01 PDT
> This is what we last had when previously discussing this. Adam, you were suggesting before to move this to another place, possibly the default event handler. How would that affect to detecting event listeners while still allowing content detection to happen for the <body> case?
>
> I'm not sure I fully understood your approach or if the page would need to add any kind of preventDefault. Could you elaborate a bit, please?

Consider how a normal hyperlinks works.  We first dispatch the click event through the DOM.  If the page handles the event and calls its preventDefault() function, then we don't do anything.  If the page doesn't call preventDefault() on the event, then the HTMLAnchorElement has a default event handler that navigates to the hyperlink's URL.

The approach I was suggesting here is analogous.  We dispatch the event through the DOM and give the page a chance to handle the event.  If the page handles the event and calls preventDefault(), then we don't do anything.  Otherwise, if the event makes it through the DOM without preventDefault() being called, then we'll trigger the Android intent behavior.

For the <body> case, the question is whether the event handlers attached to <body> call preventDefault().  For example, if they're just watching for clicks and logging them, they probably aren't going to call preventDefault() because that would stop all the hyperlinks on the page from working.
Comment 4 Eric Seidel (no email) 2012-08-22 13:21:23 PDT
Comment on attachment 159964 [details]
Patch

How do we test this?
Comment 5 Adam Barth 2012-08-22 13:47:38 PDT
> How do we test this?

We can test this via a WebKit unit test.  However, this function is currently not called by anyone because we haven't resolve the issue of whether to call the function before or after dispatching the event through the DOM.
Comment 6 Ryosuke Niwa 2012-08-22 14:13:30 PDT
Comment on attachment 159964 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3925
> +        if (node->isLink() || (touchType != WebInputEvent::GestureLongPress && (node->hasEventListeners(eventNames().clickEvent)
> +            || node->hasEventListeners(eventNames().touchstartEvent) || node->hasEventListeners(eventNames().touchendEvent)
> +            || node->hasEventListeners(eventNames().mousedownEvent) || node->hasEventListeners(eventNames().mouseupEvent)))) {

You should just use node->willRespondToMouseClickEvents().
Also, Source/WebKit/blackberry/TouchEventHandler.cpp has hasTouchListener, which may more or less check a similar condition.
Given that we may want to add a new method to Node to share code here.
Comment 7 Leandro Graciá Gil 2012-08-23 05:52:38 PDT
Comment on attachment 159964 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3925
>> +            || node->hasEventListeners(eventNames().mousedownEvent) || node->hasEventListeners(eventNames().mouseupEvent)))) {
> 
> You should just use node->willRespondToMouseClickEvents().
> Also, Source/WebKit/blackberry/TouchEventHandler.cpp has hasTouchListener, which may more or less check a similar condition.
> Given that we may want to add a new method to Node to share code here.

Sounds like a very good suggestion to me. I'll add a willRespondToTouchEvents method in Node and use it together with willRespondToMouseClickEvents.

Should I also update the blackberry port code to use the new method? If the answer is yes, who should review that bit?
Comment 8 Leandro Graciá Gil 2012-08-23 05:54:41 PDT
(In reply to comment #3)
> > This is what we last had when previously discussing this. Adam, you were suggesting before to move this to another place, possibly the default event handler. How would that affect to detecting event listeners while still allowing content detection to happen for the <body> case?
> >
> > I'm not sure I fully understood your approach or if the page would need to add any kind of preventDefault. Could you elaborate a bit, please?
> 
> Consider how a normal hyperlinks works.  We first dispatch the click event through the DOM.  If the page handles the event and calls its preventDefault() function, then we don't do anything.  If the page doesn't call preventDefault() on the event, then the HTMLAnchorElement has a default event handler that navigates to the hyperlink's URL.
> 
> The approach I was suggesting here is analogous.  We dispatch the event through the DOM and give the page a chance to handle the event.  If the page handles the event and calls preventDefault(), then we don't do anything.  Otherwise, if the event makes it through the DOM without preventDefault() being called, then we'll trigger the Android intent behavior.
> 
> For the <body> case, the question is whether the event handlers attached to <body> call preventDefault().  For example, if they're just watching for clicks and logging them, they probably aren't going to call preventDefault() because that would stop all the hyperlinks on the page from working.

Ok, sounds reasonable. Let's try using the default event handler. Where is that code exactly located?
Comment 9 Adam Barth 2012-08-23 10:15:42 PDT
> Ok, sounds reasonable. Let's try using the default event handler. Where is that code exactly located?

If you look in WebViewImpl::handleGestureEvent, you'll see a bunch of lines of code like:

        bool gestureHandled = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);

What you want to do is call detectContentOnTouch if gestureHandled is false.

You know, now that I write this, I realize that we don't expose gesture events to web sites anyway, so they won't be able to intercept the event...  That means we'll probably still need to use willRespondToTouchEvents and willRespondToMouseClickEvents.  Sorry I didn't realize that earlier.

We should still only call this function if gestureHandled is false though so that this will work correctly when we do start exposing gesture events.
Comment 10 Leandro Graciá Gil 2012-08-24 11:17:49 PDT
(In reply to comment #9)
> > Ok, sounds reasonable. Let's try using the default event handler. Where is that code exactly located?
> 
> If you look in WebViewImpl::handleGestureEvent, you'll see a bunch of lines of code like:
> 
>         bool gestureHandled = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);
> 
> What you want to do is call detectContentOnTouch if gestureHandled is false.
> 
> You know, now that I write this, I realize that we don't expose gesture events to web sites anyway, so they won't be able to intercept the event...  That means we'll probably still need to use willRespondToTouchEvents and willRespondToMouseClickEvents.  Sorry I didn't realize that earlier.
> 
> We should still only call this function if gestureHandled is false though so that this will work correctly when we do start exposing gesture events.

If there is a click listener on the body that doesn't call preventDefault, will gestureHandled be false?
Comment 11 Adam Barth 2012-08-24 11:44:15 PDT
The return value is per event, so click event handlers won't affect gesture handled, so we'll need logic like what you've got in your patch.
Comment 12 Leandro Graciá Gil 2012-08-24 12:00:47 PDT
(In reply to comment #11)
> The return value is per event, so click event handlers won't affect gesture handled, so we'll need logic like what you've got in your patch.

I'm trying to write a WebKit unit test, but I'll need a way to simulate taps and long press events over specific elements by their ids. Any suggestions? What I'm trying so far doesn't seem to work very well.
Comment 13 Adam Barth 2012-08-24 16:07:14 PDT
I would just make the element huge with CSS and the send the input event in with Widget.handleInputEvent
Comment 14 Adam Barth 2012-08-24 16:07:30 PDT
WebWidget::handleInputEvent
Comment 15 Leandro Graciá Gil 2012-08-28 07:10:18 PDT
(In reply to comment #14)
> WebWidget::handleInputEvent

Looks like tap events are being always consumed, even if there are no listeners on the page. Single tap events are becoming mouse event single click events, which eventually are consumed to create selectstart events  (see the return of EventHandler::handleMousePressEventSingleClick and the EventHandler::updateSelectionForMouseDownDispatchingSelectStart method).

For long taps the situation is similar but with EventHandler::dispatchMouseEvent, which calls to the specific node dispatchMouseEvent and consumes the event. Also, I see we're handling context menu events for long taps. Should we prioritise content detection over the context menu event? We need to make sure content detection has a chance to run.

So, detecting content only if gestureHandled is false doesn't work in either tap case. Any suggestions?
Comment 16 Adam Barth 2012-08-28 07:50:48 PDT
> So, detecting content only if gestureHandled is false doesn't work in either tap case. Any suggestions?

Thanks for investigating that direction.  It sounds like we should do this similarly to how we're doing it in the chromium-android branch (just using willRespondToMouseClickEvents and/or hasTouchListener has rniwa suggests).
Comment 17 Leandro Graciá Gil 2012-08-28 11:40:50 PDT
Removing [Chromium] from the bug name as it will introduce a new method in WebCore::Node.
Comment 18 Leandro Graciá Gil 2012-08-28 11:54:31 PDT
Created attachment 161026 [details]
Patch
Comment 19 Adam Barth 2012-08-28 15:37:08 PDT
Comment on attachment 161026 [details]
Patch

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

Thanks for iterating on the patch!

> Source/WebKit/chromium/src/WebViewImpl.cpp:4009
> +        if (node->isLink() || (touchType != WebInputEvent::GestureLongPress
> +                && (node->willRespondToTouchEvents() || node->willRespondToMouseClickEvents()))) {
> +            return false;

I would have broken this up in to a couple of if-statements to make it easier to understand the logic here, but this is fine too.
Comment 20 WebKit Review Bot 2012-08-28 16:48:58 PDT
Comment on attachment 161026 [details]
Patch

Rejecting attachment 161026 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
webkit-commit-queue/Source/WebKit/chromium/webkit --revision 153466 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 153466.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13656528
Comment 21 Adam Barth 2012-08-28 16:50:32 PDT
Comment on attachment 161026 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

This line is tripping you up.  I'll remove it for you.
Comment 22 Adam Barth 2012-08-28 16:54:21 PDT
Created attachment 161091 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-08-28 18:05:18 PDT
Comment on attachment 161091 [details]
Patch for landing

Clearing flags on attachment: 161091

Committed r126945: <http://trac.webkit.org/changeset/126945>
Comment 24 WebKit Review Bot 2012-08-28 18:05:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Adam Barth 2012-08-28 19:36:27 PDT
Follow up patch in http://trac.webkit.org/changeset/126951