RESOLVED FIXED 94727
Content detection should not disrupt the page behaviour 
https://bugs.webkit.org/show_bug.cgi?id=94727
Summary Content detection should not disrupt the page behaviour
Leandro Graciá Gil
Reported 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.
Attachments
Patch (2.22 KB, patch)
2012-08-22 10:50 PDT, Leandro Graciá Gil 		no flags
DetailsFormatted DiffDiff
Patch (14.16 KB, patch)
2012-08-28 11:54 PDT, Leandro Graciá Gil 		no flags
DetailsFormatted DiffDiff
Patch for landing (14.01 KB, patch)
2012-08-28 16:54 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Leandro Graciá Gil
Comment 1 2012-08-22 10:50:43 PDT
Created attachment 159964 [details] Patch
Leandro Graciá Gil
Comment 2 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?
Adam Barth
Comment 3 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.
Eric Seidel (no email)
Comment 4 2012-08-22 13:21:23 PDT
Comment on attachment 159964 [details] Patch How do we test this?
Adam Barth
Comment 5 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.
Ryosuke Niwa
Comment 6 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.
Leandro Graciá Gil
Comment 7 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?
Leandro Graciá Gil
Comment 8 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?
Adam Barth
Comment 9 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.
Leandro Graciá Gil
Comment 10 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?
Adam Barth
Comment 11 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.
Leandro Graciá Gil
Comment 12 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.
Adam Barth
Comment 13 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
Adam Barth
Comment 14 2012-08-24 16:07:30 PDT
WebWidget::handleInputEvent
Leandro Graciá Gil
Comment 15 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?
Adam Barth
Comment 16 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).
Leandro Graciá Gil
Comment 17 2012-08-28 11:40:50 PDT
Removing [Chromium] from the bug name as it will introduce a new method in WebCore::Node.
Leandro Graciá Gil
Comment 18 2012-08-28 11:54:31 PDT
Created attachment 161026 [details] Patch
Adam Barth
Comment 19 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.
WebKit Review Bot
Comment 20 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
Adam Barth
Comment 21 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.
Adam Barth
Comment 22 2012-08-28 16:54:21 PDT
Created attachment 161091 [details] Patch for landing
WebKit Review Bot
Comment 23 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>
WebKit Review Bot
Comment 24 2012-08-28 18:05:23 PDT
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 25 2012-08-28 19:36:27 PDT
Follow up patch in http://trac.webkit.org/changeset/126951
Note You need to log in before you can comment on or make changes to this bug.