user gesture bit sometimes lost for clicked links |
||||||
Issue descriptionTo repro: 1. navigate to http://www.nbcnews.com/pop-culture/music/grammys-2016-here-s-why-adele-s-performance-was-out-n519186 2. click the link for 'Adele' at the beginning of the article Expected: the navigation triggered by this click has the user gesture bit set, in either RenderViewImpl::navigation_gesture() or NavigationHandle::HasUserGesture(). Actual: Neither RenderViewImpl nor NavigationHandle have the user gesture bit set I'm marking this as a navigation bug since it affects NavigationHandle, but I believe the root cause is actually in Blink code's of user gesture tracking code.
,
Nov 17 2016
,
Nov 17 2016
Thanks for looking into the cause! Adding dominickn, alexmos, and jochen for knowledge about user gestures. (I wonder if this will help with the occasional false positives I see with the popup blocker.)
,
Nov 17 2016
+dtapuska@ and rbyers@.
,
Nov 17 2016
The click event is a sync event. What really is weird is the DOMActivate event and I really hope we can remove that some day because it appears it was spec'd as sync but blink dispatches it as a scoped event. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.cpp?sq=package:chromium&l=2095 Do you have more details of what event was async? I'll debug it when I'm back in the office though.
,
Nov 17 2016
Sure, here's the sequence of events I'm seeing. I don't know this code especially well so I may be misinterpreting, but I think I see: 1. a function-scoped UserGestureIndicator is instantiated on mouse down, and stored in m_lastMouseDownUserGestureToken: #1 0x7fa634865d28 blink::UserGestureIndicator::UserGestureIndicator() #2 0x7fa62c8b4e88 blink::EventHandler::handleMouseReleaseEvent() #3 0x7fa6345f7562 blink::PageWidgetEventHandler::handleMouseUp() #4 0x7fa634680e3f blink::WebViewImpl::handleMouseUp() #5 0x7fa6345f729c blink::PageWidgetDelegate::handleInputEvent() #6 0x7fa634682ff0 blink::WebViewImpl::handleInputEvent() #7 0x7fa6386f9a5d content::RenderWidgetInputHandler::HandleInputEvent() #8 0x7fa638788c5d _ZN3IPC8MessageTI30InputMsg_HandleInputEvent_MetaSt5tupleIJPKN5blink13WebInputEventEN2ui11LatencyInfoEN7content22InputEventDispatchTypeEEEvE8DispatchINS9_12RenderWidgetESE_vMSE_FvS6_RKS8_SA_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ 2. a new function-scoped UserGestureIndicator, using the m_lastMouseDownUserGestureToken from the earlier mouse down, is instantiated on mouse up: #1 0x7fa634865d28 blink::UserGestureIndicator::UserGestureIndicator() #2 0x7fa62c8b4e88 blink::EventHandler::handleMouseReleaseEvent() #3 0x7fa6345f7562 blink::PageWidgetEventHandler::handleMouseUp() #4 0x7fa634680e3f blink::WebViewImpl::handleMouseUp() #5 0x7fa6345f729c blink::PageWidgetDelegate::handleInputEvent() #6 0x7fa634682ff0 blink::WebViewImpl::handleInputEvent() #7 0x7fa6386f9a5d content::RenderWidgetInputHandler::HandleInputEvent() #8 0x7fa638788c5d _ZN3IPC8MessageTI30InputMsg_HandleInputEvent_MetaSt5tupleIJPKN5blink13WebInputEventEN2ui11LatencyInfoEN7content22InputEventDispatchTypeEEEvE8DispatchINS9_12RenderWidgetESE_vMSE_FvS6_RKS8_SA_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ 3. The UserGestureIndicator token is cleared as it goes out of scope: #1 0x7f64fbf6ffbb blink::UserGestureIndicator::~UserGestureIndicator() #2 0x7f64f3fbf14e blink::EventHandler::handleMouseReleaseEvent() #3 0x7f64fbd01562 blink::PageWidgetEventHandler::handleMouseUp() #4 0x7f64fbd8ae3f blink::WebViewImpl::handleMouseUp() #5 0x7f64fbd0129c blink::PageWidgetDelegate::handleInputEvent() #6 0x7f64fbd8cff0 blink::WebViewImpl::handleInputEvent() #7 0x7f64ffe03a5d content::RenderWidgetInputHandler::HandleInputEvent() #8 0x7f64ffe92c5d _ZN3IPC8MessageTI30InputMsg_HandleInputEvent_MetaSt5tupleIJPKN5blink13WebInputEventEN2ui11LatencyInfoEN7content22InputEventDispatchTypeEEEvE8DispatchINS9_12RenderWidgetESE_vMSE_FvS6_RKS8_SA_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ 4. V8 dispatches a click event async from the earlier call stack, which triggers the handleClick call, which calls didStartProvisionalLoad, which tests to see if there's a user gesture: #1 0x7f64ffe5892c content::RenderFrameImpl::didStartProvisionalLoad() #2 0x7f64fbced3c8 blink::FrameLoaderClientImpl::dispatchDidStartProvisionalLoad() #3 0x7f64f426088e blink::FrameLoader::startLoad() #4 0x7f64f425cc0f blink::FrameLoader::load() #5 0x7f64f3e22d0d blink::HTMLAnchorElement::handleClick() #6 0x7f64f3d161e1 blink::EventDispatcher::dispatchEventPostProcess() #7 0x7f64f3d15310 blink::EventDispatcher::dispatch() #8 0x7f64f3d2c14a blink::MouseEventDispatchMediator::dispatchEvent() #9 0x7f64f3d149b0 blink::EventDispatcher::dispatchEvent() #10 0x7f64f3d23b0e blink::EventTarget::dispatchEventForBindings() #11 0x7f64f4565fcd blink::EventTargetV8Internal::dispatchEventMethodCallback() #12 0x7f64fd6963ab v8::internal::FunctionCallbackArguments::Call() #13 0x7f64fd74920b v8::internal::(anonymous namespace)::HandleApiCallHelper<>() #14 0x7f64fd748075 v8::internal::Builtin_Impl_HandleApiCall() #15 0x7f64fd747c7c v8::internal::Builtin_HandleApiCall() Do we need to carry the gesture token from mouse down and mouse up forward to this async click dispatch, based on "The task in which the algorithm is running was queued by an algorithm that was triggered by user activation, and the chain of such algorithms started within a user-agent defined timeframe." from the spec?
,
Nov 17 2016
the call stack for step 1 is wrong - here's the call stack i meant to include: #1 0x7f64fbf6fd28 blink::UserGestureIndicator::UserGestureIndicator() #2 0x7f64f3fbd660 blink::EventHandler::handleMousePressEvent() #3 0x7f64fbd014d2 blink::PageWidgetEventHandler::handleMouseDown() #4 0x7f64fbd8a6e7 blink::WebViewImpl::handleMouseDown() #5 0x7f64fbd01272 blink::PageWidgetDelegate::handleInputEvent() #6 0x7f64fbd8cff0 blink::WebViewImpl::handleInputEvent() #7 0x7f64ffe03a5d content::RenderWidgetInputHandler::HandleInputEvent() #8 0x7f64ffe92c5d _ZN3IPC8MessageTI30InputMsg_HandleInputEvent_MetaSt5tupleIJPKN5blink13WebInputEventEN2ui11LatencyInfoEN7content22InputEventDispatchTypeEEEvE8DispatchINS9_12RenderWidgetESE_vMSE_FvS6_RKS8_SA_EEEbPKNS_7MessageEPT_PT0_PT1_T2_
,
Nov 17 2016
That looks like a click event being dispatched from JavaScript. And that is actually what user gesture indicator is attempting to block. So from a concept point of view I'm guessing this is working correctly. Is this a regression?
,
Nov 17 2016
Ah, thanks Dave, I think you're right, this navigation is being driven by JS code that listens for clicks, then dispatches MouseEvents based on those clicks. This is not a regression. I'm working on code that wants to process navigations differently depending on whether they were initiated by a user gesture. This is similar to the work japhet is doing to block cross-origin navigations of the topmost window if no user gesture is present (bug 624061). In both cases, we want to understand whether the user would think of the navigation as being initiated by a user action. So even though this particular case is driven by JS, it is indistinguishable from a normal <a href> style click navigation from a user standpoint, and thus my code (and presumably Japhet's code) would like to handle them the same way. RE: whether this particular case should be treated as having a user gesture, I would think that "The task in which the algorithm is running was queued by an algorithm that was triggered by user activation, and the chain of such algorithms started within a user-agent defined timeframe." was intended to cover this case as having a user gesture. Indeed, we chain user gestures on setTimeout, which is a similar case. That said, it sounds like the spec'd behavior is being debated/reviewed currently, in bug 404161. That bug focuses on chaining gestures when using promises, but it seems sensible to consider this case, or really any case where code runs async as a continuation of code that ran in the context of a user gesture, as similar. I'll close this out for now, and add a comment on the bug that's trying to determine the intended behavior, referencing this issue. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bmcquade@chromium.org
, Nov 17 2016