Issue metadata
Sign in to add a comment
|
Regression: Unable to Min/Max/Close browser via tap-touch.
Reported by
aiman.an...@etouch.net,
Oct 22
|
||||||||||||||||||||||
Issue descriptionChrome Version: 72.0.3588.0 (Official Build) Revision 56533f367ba451d6545ab0045e8e11f288b36326-refs/branch-heads/3588@{#1} (32/64-bit). OS: Windows 10 (Touch Device) only. Steps to reproduce: 1. Launch chrome, Try to tap-touch on Minimize/Maximize/Close button in Chrome tab strip. 2. Observe . Actual Result: Unable to Min/Max/Close browser via tap-touch. Expected Result: Should be able to Min/Max/Close browser via tap-touch. This is Regression Issue broken in M-72, and below is the bisect info. Good Build: 72.0.3584.0 (Revision:600617) Bad Build: 72.0.3585.0 (Revision:601013) Narrow Bisect: CHANGE-LOG URL: https://chromium.googlesource.com/chromium/src/+log/f17b24b1e2bb0488361cce031afd8ede9ff41455..1c621415aa17d8f1d3de440798ad19714b07df8c?pretty=fuller&n=10000 Suspecting: r600658 or r600662 ? tasak/sunnyps@: Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. Note: 1. Issue is Win 10 (Touch Device) specific and is not seen on Win(7,8,8.1,10), Mac(10.12.6,10.13.1,10.13.6,10.14.1) and Linux(14.04 LTS) OS. 2. Unable to provide 'per-revision' bisect as it shows "RuntimeError : We don't have enough builds to bisect..." error message for above range. 3. Hence provided suspect through 'Chromium bisect'. Kindly refer the attached screen-cast. Thank You!
,
Oct 23
Can you attach chrome://gpu from the affected device? Might be the same root cause as issue 871257 which is that Windows passes touch events to the intermediate D3D window from GPU process instead of browser's window. My change makes it more likely that direct composition is not used which should prevent the above kind of bug, but maybe not using direct composition causes this bug?
,
Oct 23
It's probably not tasak's change.
,
Oct 23
Hi, w.r.t. comment #2 attaching chrome://gpu details from Win 10 Touch Device. Kindly refer. Thank You!
,
Oct 23
According to chrome://gpu, direct composition is not being used. I reproduced this on a Lenovo Yoga Tab by disabling direct composition with --disable-direct-composition-layers. The minimize/maximize/close buttons do not respond to touch, but double tapping on the title bar maximizes and restores, and the rest of Chrome UI is responsive to touch. With direct composition enabled, the minimize/maximize/close buttons work as well. With direct composition, Spy++ thinks the intermediate D3D window is over buttons, but without direct composition it's the Chrome browser parent window (Chrome_WidgetWin_1). So it looks like the D3D window causes touch events on the buttons to be dropped (like the other bug), but that makes the buttons do the right thing (via default message handler probably?). So yes, my CL caused this regression because it disabled direct composition in many cases, but we don't want to revert it because we'd be using direct composition where we don't need to, and that's a riskier code path IMO.
,
Oct 23
I found an interesting post while reading the product forum for the other bug: "ruifung said: Based on what is said in https://bugs.chromium.org/p/chromium/issues/detail?id=871257 I worked around it by starting chrome with --disable-d3d11, which appears to fix the issue for me. While this does result in the minimize/maximize/close buttons no longer accepting touch input, I worked around it by setting #windows10-custom-titlebar in chrome://flags to disabled. I'm using a Surface Go 128GB. Chrome 70" +bsep@ - looks like custom title bar makes window controls not work with touch when direct composition is disabled
,
Oct 23
I'm seeing interesting results on a Lenovo Yoga Touch 710: 1) Default (direct composition enabled): window buttons work on touch 2) --disable-direct-composition-layers: window buttons do not work at all on touch (don't change visual state) 3) chrome://flags set windows10-custom-titlebar to Disabled: window buttons do not work but change visual state on touch 4) chrome://flags as above and --disable-direct-composition-layers: window buttons work on touch So it looks like window buttons work on touch = direct composition xnor custom titlebar
,
Oct 23
From Ross about spy++ issue. > I debugged that too… I see spy++ calls WindowFromPoint, and that function is returning > the legacy window. Spy++ must be doing > something else after the fact, but I can’t tell > what. Certainly misleading… > > > Hi Ross, > > > > We have one related issue. When we debugging this issue, we use spy++ find tool to > > find the Chrome window, it select the D3D window, but spy++ shows D3D window is below > > legacy Chrome window. Is it expected? MSDN only mention the spy++ shows window by z-order. https://docs.microsoft.com/en-us/visualstudio/debugger/windows-view?view=vs-2017 So if you seeing the order is: + Toplevel: class=Chrome_WidgetWin_1 +- Child 1: Chrome Legacy Window +- Child 2: Intermediate D3D Window or no D3D window Then the z-order is correct.
,
Oct 23
I've seen a few of these go by. I have bug 894023 for "caption buttons don't work if hardware acceleration is off" which I haven't had time to look at. I also have a smattering of "caption buttons don't work at all" bugs for older hardware (e.g. bug 845832). I'm guessing their touch drivers are weird, but I haven't dug into it that much. Those are mostly for Dell Inspiron 15R though. Is this one of those issues? If so, this doesn't need to be RBS.
,
Oct 24
Looks like a duplicate of those other issues. Disabling hardware acceleration has the side-effect of disabling direct composition which is what actually breaks the window buttons. We disabled direct composition in many cases (non Intel Skylake+ devices) so more users will see this bug now so it might be higher priority.
,
Oct 24
I see. I'll bump up 894023 on my todo list then, or feel free to investigate yourself. Let me know if you have any questions. The buttons are supposed to work in all cases of course, but custom titlebar is rarely off on Windows 10 and touch devices are rare on Windows 7.
,
Oct 24
We probably want to dupe the other way tho, since there's more info here.
,
Oct 24
Can you provide a few pointers for how we handle window button interactions? Where are the message handlers for the browser parent (Chrome_WidgetWin_1) and child (Chrom Legacy Window) windows?
,
Oct 24
Chrome_WidgetWin_1 Message Handler: https://cs.chromium.org/chromium/src/ui/views/win/hwnd_message_handler.h Chrom Legacy Window Message Handler: https://cs.chromium.org/chromium/src/content/browser/renderer_host/legacy_render_widget_host_win.h
,
Oct 25
,
Oct 25
,
Oct 25
We have a fix for the touch scrolling issue (crrev.com/c/1299342) but it makes this bug happen 100% of the time.
,
Oct 25
-> bsep@ since this is a known UI bug
,
Oct 26
,
Oct 31
Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Nov 5
@Gentle Ping! Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Nov 7
Haven't started yet, been too busy. Hopefully will soon
,
Nov 8
Issue 894023 has been merged into this issue.
,
Nov 9
I looked at this today. I tried to reproduce the bug in both the simulator and on a Surfacebook: * Building ToT always reproduces, regardless of whether I pass in --disable-direct-composition-layers or --enable-direct-composition-layers, or toggle hardware acceleration. * Canary seems to reproduce inconsistently... sometimes it works and sometimes it doesn't. It sends to reproduce more often than not, but it certainly was confusing. * Stable always works. I ran a bisect and it came up with r600662 as above, which we already knew about. I attached Spyxx and it claims that it's sending WM_POINTERDOWN to Stable, but WM_NCPOINTERDOWN to Canary. So we did something that makes Windows think we can handle WM_NCPOINTERDOWN, but we don't. Honestly, we SHOULD be handling it, but no one has had time to implement it. I'll play around with it more tomorrow, but we may have to revert sunnyps@'s patch until we get around to doing Windows 10 touch properly... I'll also check and make sure bug 894032 is actually a dupe; I'm not convinced it is, since the direct composition flags above didn't seem to do anything.
,
Nov 9
FWIW that CL has been effectively reverted on ToT with https://chromium-review.googlesource.com/c/chromium/src/+/1318808 We started using WS_EX_TRANSPARENT for the GPU process child window which makes Windows pass input events to sibling/parent windows. So now one of the browser windows will be processing the event which is where the bug is. We'd have to revert using that window style, but we did that because touch scrolling was completely broken without that (Windows wouldn't pass touch messages to the browser window). We decided that the min/max/close button bug is less severe than touch scrolling being broken.
,
Nov 9
Okay, so it sounds like we need to roll forward. Hopefully it won't be too much work to support WM_NCPOINTER* events.
,
Nov 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbdbb023f2619c745abcd8bcab3039688b8eded5 commit fbdbb023f2619c745abcd8bcab3039688b8eded5 Author: Bret Sepulveda <bsep@chromium.org> Date: Wed Nov 14 20:25:58 2018 Add support for WM_NCPOINTER* messages. A recent change caused the caption buttons to stop responding to touch. When I investigated, the change caused Windows to send WM_NCPOINTERDOWN and WM_NCPOINTERUP in that area (which we don't handle), where previously it was sending WM_POINTERDOWN and WM_POINTERUP. Since the change in question is fixing a different critical issue, this patch implements handling for WM_NCPOINTERDOWN, WM_NCPOINTERUP, and WM_NCPOINTERUPDATE, by delegating to the existing touch event handler. This patch also modifies the workaround frin crrev.com/c/1260507, since the type of messages being translated into ui::ET_TOUCH_RELEASED events is now different. Bug: 897662 Change-Id: I6fd052211b91a9a78834573deac830883972092b Reviewed-on: https://chromium-review.googlesource.com/c/1330838 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#608108} [modify] https://crrev.com/fbdbb023f2619c745abcd8bcab3039688b8eded5/ui/views/win/hwnd_message_handler.cc [modify] https://crrev.com/fbdbb023f2619c745abcd8bcab3039688b8eded5/ui/views/win/hwnd_message_handler.h [modify] https://crrev.com/fbdbb023f2619c745abcd8bcab3039688b8eded5/ui/views/win/pen_event_processor.cc
,
Nov 14
,
Nov 15
Hi, Retested the above issue on Win 10(Touch Device) using latest canary #72.0.3611.0 and issue is fixed. Now, able to Min/Max/Close browser via Tap-touch. Please refer the attached screen-cast for reference. Thank You!
,
Nov 16
***UI Mass Triage*** |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rbasuvula@chromium.org
, Oct 22