New issue
Advanced search Search tips

Issue 827659 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Calls to ResourceDispatchHost::OnUserGesture may be filtering on the wrong signals

Project Member Reported by dcheng@chromium.org, Mar 30 2018

Issue description

Today, WebContentsImpl::OnUserInteraction() filters for kMouseWheel before calling RDH::OnUserGesture().

However, RenderWidgetHostImpl::FilterInputEvent() won't even call OnUserInteraction() if the type is kMouseWheel (since it checks for a number of event types, none of which are kMouseWheel).

However however, mouse wheel events *also* generate gesture scroll begin input events... so maybe we need to be filtering those out?

*brain explodes from complexity of input events*
 

Comment 1 by dcheng@chromium.org, Mar 30 2018

Also, https://chromium-review.googlesource.com/c/chromium/src/+/987694 shuffles some of this logic around, but the core question remains if we should be filtering out kGestureScrollBegin before calling RDH::OnUserGesture().

In the long-term, we can hopefully converge the complexity of maintaining correctness here by settling on one definition for user activation (i.e. issue 826293)
Buenas noches no entiendo lo único que se es que no puedo entrar más en Chrome que es mi navegador y les pregunto cual es el problema yo no quiero cambiar notificaciones a cambio de mi correo electrónico lo único por favor quiero solución si son tan amable. Desde ya muy agradecida.
The intention of the if was to preserve existing functionality. The old OnUserGesture() did not fire on scroll events, but UserInteraction did. At the time, the kGestureScrollBegin event had not been launched, so there was a timer on the mouse wheel scroll event to only send the first one (see https://codereview.chromium.org/1735833002). And hence the if statement to filter out the mouse wheel event

Now that we have the GestureScrollBegin event, this if statement should probably say GestureScrollBegin, not MouseWheel. Not sure if this has caused unintended bugs along the way.
Cc: dominickn@chromium.org
Owner: dcheng@chromium.org
dcheng: did you fix this in https://chromium-review.googlesource.com/1028484?
No, I didn't fix this (and I didn't make the original change, so it's hard for me to judge what the intent is).
Hmmm, D'oh, I can't read code properly. :)

I've sent a CL to fix this.
Cc: -dominickn@chromium.org
Owner: dominickn@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37a15052f10515874939931d9ed725e94bc94ee7

commit 37a15052f10515874939931d9ed725e94bc94ee7
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Jul 17 23:56:35 2018

Remove browser-initiated navigations from WebContentsObserver::DidGetUserInteraction.

Some types of user interactions are used as a signal to
ResourceDispatcherHostImpl. Prior to https://crrev.com/1735833002,
scrolling was not one of them, and part of that CL's work explicitly
filtered out MouseWheel events from being passed to
ResourceDispatcherHostImpl. It also changed user-initiated
navigations to call the new DidGetUserInteraction method with the
blink::WebInputEvent::Undefined type. However, only a limited subset of
user-initiated navigations were passed through this method:
Ctrl-R/refresh button reloads, omnibox navigations, and NTP/bookmark
navigations. Other user-initiated navigations such as back/forward
buttons were omitted.

https://crrev.com/1776843003 introduced wheel gesture events that
generalised touch and mouse scrolling into one concept.
https://crrev.com/1748553002 updated the user interaction code to use
the new GestureScrollBegin event in place of MouseWheel events, but did
not update the type check for forwarding to ResourceDispatcherHostImpl.

https://crrev.com/1964863002 introduced
NavigationHandle::IsRendererInitiated, which returns true if a
navigation was initiated by the renderer process, and false if it was
initiated by the browser process (i.e. it was user-initiated). This
is a broader scope that the existing user-initiated navigations
signaled through DidGetUserInteraction, namely including back/forward
navigations.

This CL improves on the user gesture handling in WebContentsImpl by:
 * removing user-initiated navigations from DidGetUserInteraction, and
   updating clients to listen to Did{Start/Finish}Navigation and check
   NavigationHandle::IsRendererInitiated. This includes more types of
   user-initiated navigations.
 * restoring the user gesture signal to ResourceDispatcherHostImpl back
   to how it was prior to https://crrev.com/1748553002 - listening to
   keydown, mouse click, taps, and certain user-initiated navigations.

BUG= 827659 

Change-Id: I639270b21446104f22c8ae9b8b939eb96af3cabd
Reviewed-on: https://chromium-review.googlesource.com/1125703
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575857}
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/download/download_request_limiter.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/download/download_request_limiter.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/download/download_request_limiter_unittest.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/engagement/site_engagement_helper.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/safe_browsing/safe_browsing_navigation_observer.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/ui/app_list/search/answer_card/answer_card_web_contents.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/ui/app_list/search/answer_card/answer_card_web_contents.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/ui/blocked_content/popup_opener_tab_helper.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/ui/blocked_content/popup_opener_tab_helper.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/content/public/browser/navigation_handle.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/content/public/browser/web_contents.h
[modify] https://crrev.com/37a15052f10515874939931d9ed725e94bc94ee7/content/public/browser/web_contents_observer.h

Labels: UserActivation
Status: Fixed (was: Started)

Sign in to add a comment