Issue metadata
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 descriptionSteps 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).
,
Jul 11 2017
,
Jul 14 2017
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
,
Jul 14 2017
,
Jul 20 2017
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?
,
Jul 20 2017
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.
,
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.
,
Jul 20 2017
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.)
,
Jul 20 2017
,
Jul 20 2017
The problem happens in Android M too except that the "interception area" is strictly above the widget boundary, and the pixel measures are different.
,
Jul 20 2017
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 )
,
Jul 20 2017
Will do. I think Issue 735193 also has to do with it.
,
Jul 20 2017
,
Jul 20 2017
,
Jul 21 2017
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.
,
Jul 21 2017
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?
,
Jul 24 2017
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.
,
Jul 24 2017
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.
,
Jul 24 2017
Correction to my first para above: I meant Toolbar instead of SwipeRecognizer above!
,
Jul 24 2017
#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.
,
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
,
Jul 25 2017
,
Jul 26 2017
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
,
Jul 26 2017
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
,
Jul 26 2017
,
Jul 26 2017
Issue 735761 has been merged into this issue.
,
Jul 26 2017
,
Aug 1 2017
Verified on Chrome:61.0.3163.27 Device:Pixel/7.1.2
,
Sep 19 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by somcs...@gmail.com
, Jul 11 20179.8 MB
9.8 MB Download