New issue
Advanced search Search tips

Issue 912514 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression
fi



Sign in to add a comment

Regression: Browser minimizes automatically after tap-touch on reload icon.

Project Member Reported by aim...@virtusa.com, Dec 6

Issue description

Chrome Version: 73.0.3632.0 (Official Build)914f5b8f721ff5a734bb22a96363191c20c8c3f2-refs/branch-heads/3632@{#1} (32/64-bit)

OS: Windows 10 (Touch Device).

Steps to reproduce:
1. Launch chrome, Minimize chrome once by tap-touch on 'minimize' icon in tab-strip.
2. Maximize chrome by tap-touch on chrome in task-bar.
3. Now tap-touch on reload icon and observe.
4. Observe.

Actual Result: Browser minimize automatically after tap-touch on reload icon.
Expected Result: Browser should not minimize automatically.

This is Regression Issue broken in M-72, and will soon update other info:
Good Build: 72.0.3623.0 (Revision:611017)
Bad Build: 72.0.3624.0 (Revision:611430)

Note:
1. Issue is Win 10 (Touch Device) specific and is not seen on Win(7,8,8.1,10), Mac(10.13.1,10.13.6,10.14.2) and Linux(14.04 LTS) OS.
2. Issue is also seen on Dev #72.0.3626.7 build.

Kindly refer the attached screen-cast.

Thank You!

 
Actual Result.mp4
1.1 MB View Download
Expected Result.mp4
815 KB View Download
Status: Untriaged (was: Unconfirmed)
As this being a Non-Regression issue, changing the status to Untriaged so that the issue would get addressed.

Thank You!
Status: Unconfirmed (was: Untriaged)
Please ignore comment#1, Bisect results will update soon.Thank You!
Labels: hasbisect-per-revision
Owner: davidbienvenu@chromium.org
Status: Assigned (was: Unconfirmed)
Summary: Regression: Browser minimizes automatically after tap-touch on reload icon. (was: Regression: Browser minimize automatically after tap-touch on reload icon.)
Update:

You are probably looking for a change made after 611321 (known good), but no later than 611322 (first known bad).

CHANGE-LOG URL:

The script might not always return single CL as suspect as some perf builds might get missing due to failure.

https://chromium.googlesource.com/chromium/src/+log/7cecfe61811dcc3ae80fda771a6026e13627ac31..04f0abb991b6ff444114dd775141fbbc04ad1403

Suspect: https://chromium.googlesource.com/chromium/src/+/04f0abb991b6ff444114dd775141fbbc04ad1403

davidbienvenu: 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.

Thank You!

Status: Started (was: Assigned)
It's very possible that's caused by my change. Perhaps touch restore doesn't generate the same event(s) as mouse click restore (SC_RESTORE). I'll investigate.
Cc: bsep@chromium.org
From what I can tell with spy++, touch minimize and touch restore do generate the same WM_SYSCOMMAND SC_MINIMIZE/SC_RESTORE messages as using the mouse. And, the key to recreating this is using touch to minimize chrome. Once chrome is restored, touching anywhere in non web content area causes the minimize button to be highlighted, and then chrome is minimized. It doesn't matter if chrome is restored via the mouse or touch; it just matters that chrome is minimized with touch. Once you get in this bad state, it's difficult to get out of it. I have gotten out of this state, but I'm not sure how - it might have been moving the window. The fact that touching anywhere in the non web content area causes the minimize button to be highlighted is very weird - is that Windows doing that, or does Chrome handle the minimize button with the custom titlebar?
#5: Chrome handles the custom minimize button (see GlassBrowserFrameView::ButtonPressed), but it's pretty standard. I wonder if the first touch caches the touch location, and it isn't updated when the browser minimizes/restores. I have no idea if that code exists though; I'm just spitballing. If it is something like that, I can see how dragging the window (which would be a different gesture) would get you out of that state.

You might be able to get insight into what part of the code is going wrong by turning off custom titlebar (chrome://flags/#windows10-custom-titlebar), which makes Windows do the button handling.
@bsep, thanks. Turning off the custom titlebar does make the problem go away. And making a different gesture with the custom titlebar on also clears the state - e.g., minimize with touch, restore, and touch down in the titlebar, w/o releasing - the minimize button gets highlighted, but if you drag your finger a little bit, the highlighting goes away and we're back to being in a good state. How this is related to my change of hiding the root window and compositor on minimize and showing on restore is a mystery, though. 
Cc: sky@chromium.org
@sky, do you have any insight into what code specifically could be causing this issue? The Window::CleanupGestureState calls when the sys command minimize is handled happen before the code I added to hide the root window is called, at least when I minimize with the mouse. I.e., after the call to DefWindowProc in HWNDMessageHandler::OnSysCommand. So it's hard to see how hiding the root window is preventing the clearing of the touch gesture.
David, did you verify it was your patch that caused this?

HWNDMessageHandler::OnTouchEvent() processes touches async. Is it possible we're trying to process a touch *after* the window is hidden, which is causing the GestureRecognizer (or related classes) to be in a bad state? Did you try inspecting the state of the GestureRecognizer when the touch after minimizing is processed?
Labels: Hotlist-DesktopUIConsider
Yes, I did a build with my change commented out and it didn't have the problem. It seems likely we're trying to process a touch after the window is hidden, but there is code that allows certain touch events to be processed even if the window is hidden. One thing I've noticed is that when I restore Chrome after a touch minimize, the minimize button highlights briefly, as if the button state is still selected. fwict, gesture recognizer does get cleaned up on minimize. I'll keep digging. One weird thing is that all the hittests are for the correct coordinates, but the minimize button still gets the events. I tried to instrument the various capture/lock pieces of code, but I haven't found the problem yet.
I would be inclined to make sure the async touch handling in HWNDMessageHandler early outs if the window is hidden.
The issue is that we don't process the gesture end event, if the root window is not visible, which leaves the root_view holding onto the minimize button as its gesture_handler_.  A lot of different things cause RootView to clear its gesture_handler_, but none of them were getting triggered by minimizing the window. In particular RootView::VisibilityChanged() clears the gesture_handler_, but that wasn't getting called. A simple fix is to make DesktopWindowTreeHostWin::HandleWindowMinimizedOrRestored() call HandleVisiblityChanged, so that all the interested parties get notified about the visibility change. I've verified that fixes the issue.

Comment 14 Deleted

Hi,

Just to update,

I retested the above bug and I am able to reproduce this issue on latest Canary build #73.0.3665.0 on Win 10 Touch Device.

Attaching screen-cast for reference.

Thank You!
Canary Behaviour.mp4
1.4 MB View Download
The fix for this issue hasn't been submitted yet. This specific issue was introduced in M72. We plan on rolling back a CL to fix this issue for M72 today, and hope to submit a fix for M73 soon. 
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 15

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

commit a82f115f1eb907a8ed70bd87f82a45951ec62d26
Author: David Bienvenu <davidbienvenu@chromium.org>
Date: Tue Jan 15 13:59:47 2019

Make DesktopNativeWidgetAura::Minimize clear root view event handlers.

This fixes the issue with the root view holding onto a gesture handler on window minimize
which sends the event to the previous gesture handler.

Bug:  912514 
Change-Id: I25775378197fb56cca6ad1c8232e792d83720ba8
Reviewed-on: https://chromium-review.googlesource.com/c/1388170
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622842}
[modify] https://crrev.com/a82f115f1eb907a8ed70bd87f82a45951ec62d26/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
[modify] https://crrev.com/a82f115f1eb907a8ed70bd87f82a45951ec62d26/ui/views/widget/root_view.cc
[modify] https://crrev.com/a82f115f1eb907a8ed70bd87f82a45951ec62d26/ui/views/widget/root_view.h
[modify] https://crrev.com/a82f115f1eb907a8ed70bd87f82a45951ec62d26/ui/views/widget/widget_interactive_uitest.cc

Labels: fi
Status: Fixed (was: Started)
Fixed on head; should have been fixed on m72 branch since Monday.

Sign in to add a comment