New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 666300 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

user gesture bit sometimes lost for clicked links

Project Member Reported by bmcquade@chromium.org, Nov 17 2016

Issue description

To 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.
 
Here's what's currently happening:

1. the link click causes a call to blink::EventHandler::handleMouseReleaseEvent(), which creates a DocumentUserGestureToken that's scoped to the lifetime of this function call
2. as part of the handleMouseReleaseEvent invication, an event is queued for async execution, which eventually triggers a call to blink::HTMLAnchorElement::handleClick(), with the following call stack (at least, I believe this is the cause):

#1 0x7fd68425492c content::RenderFrameImpl::didStartProvisionalLoad()
#2 0x7fd6800e93c8 blink::FrameLoaderClientImpl::dispatchDidStartProvisionalLoad()
#3 0x7fd67865c48e blink::FrameLoader::startLoad()
#4 0x7fd67865880f blink::FrameLoader::load()
#5 0x7fd67821e90d blink::HTMLAnchorElement::handleClick()
#6 0x7fd6781120c1 blink::EventDispatcher::dispatchEventPostProcess()
#7 0x7fd6781111f0 blink::EventDispatcher::dispatch()
#8 0x7fd678127e72 blink::MouseEventDispatchMediator::dispatchEvent()
#9 0x7fd678110890 blink::EventDispatcher::dispatchEvent()

3. didStartProvisionalLoad calls WebUserGestureIndicator::isProcessingUserGesture(), which returns false since the user gesture created during handleMouseReleaseEvent was scoped to the lifetime of the function call, and has been cleared by the time the async callback is invoked that calls blink::HTMLAnchorElement::handleClick().

What I think needs to happen here is that the gesture gets propagated forward to the async event which triggers invocation of HTMLAnchorElement::handleClick. The HTML spec says (https://html.spec.whatwg.org/multipage/interaction.html#triggered-by-user-activation):

An algorithm is triggered by user activation if any of the following conditions is true:

1. The task in which the algorithm is running is currently processing an activation behavior whose click event's isTrusted attribute is true.

2. The task in which the algorithm is running is currently running the event listener for an event whose isTrusted attribute is true and whose type is one of: change, click, dblclick, mouseup, pointerup, reset, submit, touchend

3. 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.

It seems to me that 2 or 3 should apply here. However, we're failing to carry the gesture forward to the invocation of HTMLAnchorElement::handleClick.
Status: Available (was: Untriaged)

Comment 3 by creis@chromium.org, Nov 17 2016

Cc: dominickn@chromium.org jochen@chromium.org alex...@chromium.org
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.)
Cc: rbyers@chromium.org
Owner: dtapu...@chromium.org
+dtapuska@ and rbyers@. 
Components: Blink>Input
Labels: Hotlist-Input-Dev
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.
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?
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_

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?
Status: WontFix (was: Available)
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