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

Issue 740855 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression

Blocking:
issue 735193
issue 735761



Sign in to add a comment

Chrome top panel in Android N intercepts mouse events and messes up the y-coordinates

Reported by somcs...@gmail.com, Jul 11 2017

Issue description

Steps to reproduce the problem:
1. Connect USB Mouse/BT Mouse to the device.
2. Launch Chrome.
3. Open any website (Ex. www.google.com).
4. Tap Menu icon on the Right-Top.
Observe the behavior.

What is the expected behavior?
The main should display normally.

What went wrong?
Main page in background is also changing.

Did this work before? N/A 

Chrome version: 59.0.3071.125  Channel: n/a
OS Version: 8.0.0
Flash Version: 

Issue is not observed with the other browsers like Mozilla.
Issue observed in Nexus 6x (Android 7.0).
 

Comment 1 by somcs...@gmail.com, Jul 11 2017

web page display abnormally.zip
9.8 MB Download
Cc: prashanthpola@chromium.org
Labels: triage-te
Components: Blink>Input
Labels: -triage-te M-60
Owner: mustaq@chromium.org
Status: Assigned (was: Unconfirmed)
we are able to repro this issue in Chrome:59.0.3071.125 Device:Pixel XL/7.1.2

Bisect Info:

Good Build:58.0.2992.0
Bad Build:58.0.2993.0
Regression range:
https://chromium.googlesource.com/chromium/src/+log/58.0.2992.0..58.0.2993.0?pretty=fuller&n=10000

Good commit:446059
Bad commit:446060
Culprit CL:
https://chromium.googlesource.com/chromium/src/+/cbfc51271202a15afc7c6797f4eab4052c6a3867

Labels: -Type-Bug Type-Bug-Regression

Comment 5 by mustaq@chromium.org, Jul 20 2017

Cc: aelias@chromium.org dtapu...@chromium.org
Reproduces on 59.0.3071.125 on N5X with Android N.

However, Android M devices seem to work fine, tested on N7.

Upon a closer look, it seems that a mouse click on the address bar menu is also sent to the nearby element on the content.  E.g. on www.bbc.com/news, as if the search box is getting clicked.

Not sure what might be causing this only on Android N.  Any chance touch adjustment works differently on N?

Comment 6 by aelias@chromium.org, Jul 20 2017

Cc: amaralp@chromium.org jaebaek@chromium.org mdjones@chromium.org jinsuk...@chromium.org
I don't think it can be a touch adjustment problem, because that's Blink-side code.  The click ought to be getting absorbed by the URL-bar on the browser-side, and never seen by Blink at all.  cc'ing some folks who might have an idea where the problem is.

Comment 7 by mustaq@chromium.org, Jul 20 2017

Another reason it can't be touch adjustment is that none of these devices (N7, N5X) replace mouse events with touch! Thanks Dave.

Comment 8 by mustaq@chromium.org, Jul 20 2017

Components: UI>Browser>Mobile>NavPanel
Owner: ----
Status: Available (was: Assigned)
Summary: Chrome top panel in Android N intercepts mouse events and messes up the y-coordinates (was: The main page in background is also changing when tap the menu icon with USB Mouse/BT Mouse in chrome app.)
This looks like an issue with the top bar: any click near the bottom edge of the bar is sent to the content below with a y offset of +58px.  This happens along a 14 pixel wide strip (5px above and 9px below) around the bottom edge.

Repro:
- Open http://rbyers.github.io/eventTest.html, "Config" to log only mouse events.
- Click on the strip mentioned above.

Expected: no events should be logged for clicks in the top "Config" area.

Actual: clicks are logged with +58px offset in clientY. Check the clientY's of click and (the Blink-synthesized) mouseout events.

---

aelias@: Not sure what's the correct bug component I should use, used my best guess.  (Blink input is affected, so kept the old component.)

Comment 9 by aelias@chromium.org, Jul 20 2017

Blocking: 735761
The problem happens in Android M too except that the "interception area" is strictly above the widget boundary, and the pixel measures are different.
Cc: mustaq@chromium.org
Labels: -Pri-2 -Arch-x86_64 -M-60 M-61 OS-iOS Pri-1
Owner: jinsuk...@chromium.org
Status: Assigned (was: Available)
Jinsuk, can you take this on since it seems to be a browser input event forwarding problem?  I'd like to see it fixed in M61 (since it also probably causes  http://crbug.com/735761 )
Will do. I think  Issue 735193  also has to do with it.
Blocking: 735193
Labels: -OS-iOS
Made some observations:

- The mouse button event on address bar being passed to content also happens on M. Tested with N5/MMB29Q
- The issue didn't happen when the first CL that transformed touch to mouse events r431571 landed, but started happening when it was reinstated by the 'culprit' r446060 after it had been temporarily removed at r444116 I wonder what happened in the meantime...

A quick fix I can think of is remove the BUTTON_PRESS/RELEASE cases in CVC.onGenericMotionEvent and let onMouseEvent generate synthetic button events in pair with DOWN/UP. But there may be a side effect. I'm hoping to find a root cause here.

Thanks jinsukkim@ for pin-pointing the events that are "leaking" into the content.  Unfortunately we can't replace PRESS/RELEASE with DOWN/UP since only the former provides the info about the last button changed (through getActionButton()) which is crucial for Blink.  This was discussed in the first CL we attempted (crrev.com/2054193002).

The correct fix would be to drop/handle the new events (PRESS/RELEASE) in the top widget (don't know what it is called, Navpanel?).  I suspect those events are just falling though some event filtering logic there that has no default-type check.  Should we try a debug build in case there is a DCHECK?

mustaq@: oh I didn't mean 'replacing PRESS/RELEASE with DOWN/UP' but generating synthetic PRESS/RELEASE events upon DOWN/UP for mouse event flow rather than relying on real PRESS/RELEASE. Since DOWN/PRESS - RELEASE/UP are always supposed to come in pairs, and it is only PRESS/RELEASE that are being leaked now not DOWN/UP, this will work.

It indeed is a quick fix but it has a positive side effect that it let us go without a workaround like r467470 which was written to deal with some hardware not generating button events properly. Not that this should be the prioritized way for that reason... I have a fix which I think is more correct https://crrev.com/c/582053 though I still haven't found the real culprit to revert (or base the fix on).

mdjones@ could you shed some light on this? Dead code you removed https://codereview.chromium.org/2560043002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java seemed to have played the role of preventing the leakage we're observing here. Do you know some history on how it was used? I think my fix basically does the same thing against mouse events on toolbar, but I'd like to know how it was handled, in case there was a better or more general way.

Your CL above looks like a proper fix to me: since the SwipeRecognizer is not ready to handle PRESS/RELEASE events, it shouldn't see them.

Re synthetic PRESS/RELEASE: in the content layer, we need the "last button changed" info for MouseEvent/PointerEvent. DOWN/UP events don't provide the info, and it would be error-prone to maintain extra states in CVC for that.
Correction to my first para above: I meant Toolbar instead of SwipeRecognizer above!
#17: The toolbar view changed locations in the view hierarchy. That code was used to check if a touch event was inside the bounds of the toolbar before android would normally do it (because of the hierarchy order). Now the toolbar is a child of our root coordinator layout, the same view that holds the web contents. Android's normal touch event checking now works in an order where we do not need to manually check the view.

If a view is intercepting/using a motion event, it should not be propagated to the underlying page. It seems like there is a mistake somewhere in the toolbar code.
Project Member

Comment 21 by bugdroid1@chromium.org, Jul 24 2017

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

commit ddd3b95b86917f5c0e61563689762219bad18d87
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Jul 24 23:03:41 2017

Consume mouse button events on toolbar

A regression was letting mouse button click events on toolbar
to be passed down to content layer, and causing the content
hidden behind the toolbar react to the events as well, while
they should have triggered corresponding action on the toolbar
only. This CL stop the leak.

BUG= 740855 

Change-Id: I3e0e3ede01ba73d589b3d3e3fa3a55b2577c0957
Reviewed-on: https://chromium-review.googlesource.com/582053
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489123}
[modify] https://crrev.com/ddd3b95b86917f5c0e61563689762219bad18d87/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java

Labels: Merge-Request-61
Project Member

Comment 23 by sheriffbot@chromium.org, Jul 26 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, Jul 26 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2011da09b129626b86c8f5f3720825f5d41665f9

commit 2011da09b129626b86c8f5f3720825f5d41665f9
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Jul 26 04:37:55 2017

Consume mouse button events on toolbar

A regression was letting mouse button click events on toolbar
to be passed down to content layer, and causing the content
hidden behind the toolbar react to the events as well, while
they should have triggered corresponding action on the toolbar
only. This CL stop the leak.

BUG= 740855 
NOTRY=true
NOPRESUBMIT=true
TBR=jinsukkim@chromium.org

(cherry picked from commit ddd3b95b86917f5c0e61563689762219bad18d87)

Change-Id: I3e0e3ede01ba73d589b3d3e3fa3a55b2577c0957
Reviewed-on: https://chromium-review.googlesource.com/582053
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489123}
Reviewed-on: https://chromium-review.googlesource.com/585509
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#52}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/2011da09b129626b86c8f5f3720825f5d41665f9/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java

Status: Fixed (was: Assigned)
 Issue 735761  has been merged into this issue.
Cc: rlanday@chromium.org
 Issue 735193  has been merged into this issue.
Verified on Chrome:61.0.3163.27 Device:Pixel/7.1.2 
Cc: ajit...@samsung.com
 Issue 766463  has been merged into this issue.

Sign in to add a comment