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

Issue 317496 link

Starred by 15 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2014
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

[Aura] Regression: Mouse pointer does not appear when Fullscreen is escaped

Reported by yfulgaon...@etouch.net, Nov 11 2013

Issue description

Chrome Version:33.0.1705.0 (Official Build 234138) canary
OS: Win 7 (Aero enabled), Win 8

What steps will reproduce the problem?
1. Launch Chrome and navigate to "http://scheib.github.com/HTMLMisc/PointerLockAndFullscreen.html"
2. Click on "Fullscreen and Lock pointer Target 1" under Target 1 section.
3. Allow to disable mouse pointer in Fullscreen mode and press escape (Esc) key.
4. Repeat step 2 and again press Esc key, observe.

Mouse pointer is not restored when Fullscreen mode is escaped.

Mouse pointer should be restored when Fullscreen mode is escaped.

This is an aura specific regression issue broken in M-32 and will soon provide narrow bisect info.

 
Below is the narrow bisect URL for above issue:

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=232650%3A232669

Note:Mouse pointer disappears for whole browser and never appears back, even when tab containing "http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html" link is closed. 
Owner: sky@chromium.org
Status: Assigned
Suspecting 232662

@sky@chromium.org --> Can you please take a look at the issue if your changes caused the problem? Please feel free to assign it to right developer if your changes are not causing the issue.
Labels: ReleaseBlock-Stable
Cc: vkomonduri@chromium.org
Labels: nov18
Uh oh, sounds bad. But @vkomonduri does this affect only touch machines? I think r232662 is mostly about touch machines, but I agree sky's statement about not needing the cursor hiding logic on Windows sounds suspicious here.
Issue 318122 has been merged into this issue.

Comment 6 by sky@chromium.org, Nov 12 2013

Status: Started
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 13 2013

------------------------------------------------------------------------
r234741 | sky@chromium.org | 2013-11-13T04:01:23.843558Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/corewm/compound_event_filter.cc?r1=234741&r2=234740&pathrev=234741

Gets mouse pointer to show again when existing fullscreen

My change at r232662 broke showing of the cursor when exiting
fullscreen. We always need to attempt to show the cursor, otherwise it
can get stuck hidden.

BUG= 317496 
TEST=see bug
R=ananta@chromium.org

Review URL: https://codereview.chromium.org/66333010
------------------------------------------------------------------------

Comment 8 by sky@chromium.org, Nov 13 2013

Labels: Merge-Requested

Comment 9 by kareng@google.com, Nov 13 2013

this jsut missed canary. let's see tomorrow and we'll merge.

Comment 10 by kareng@google.com, Nov 15 2013

does it look ok on canary? if so, we'd like ot merge today.

Comment 11 by sky@chromium.org, Nov 15 2013

I haven't seen any related issues come up.
Cc: mbollu@chromium.org
Still reproducible on 33.0.1710.0 canary - Win7 & Win8(classic mode).

When in Step 4: Repeat step 2 and don't move mouse. Press Esc key, observe.

Result: Mouse pointer is missing within chrome browser. Mouse pointer is observed outside chrome browser.

Comment 14 by sky@chromium.org, Nov 15 2013

The hiding with lack of mouse move was there before my changes. Will investigate.
Cc: jsc...@chromium.org
To be clear, this more limited case is a regression from non-Aura M31 right?

Justin (as proxy security guy) how worried should I be about the mouse pointer not reappearing after fullscreen is exited? I think its OK but I might be missing something.

Comment 16 by sky@chromium.org, Nov 16 2013

That's right, this is still a regression from 31. Fix landed @ 235500.
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 16 2013

------------------------------------------------------------------------
r235500 | sky@chromium.org | 2013-11-16T02:51:59.310187Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.h?r1=235500&r2=235499&pathrev=235500
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.cc?r1=235500&r2=235499&pathrev=235500

Fixes cursor hiding bug

OnCursorVisibilityChanged may be invoked multiple times with the same
value. Current windows implementation is effectively ref counted. We
don't want that, instead it should show/hide based on most recent
call.

BUG= 317496 
TEST=none
R=ananta@chromium.org

Review URL: https://codereview.chromium.org/70383014
------------------------------------------------------------------------
Labels: TE-Verified-33.0.1712.0 Te-Verified-M33
Works fine in - 33.0.1712.0 (Official Build 235613) canary

Comment 19 by kareng@google.com, Nov 18 2013

Labels: -Merge-Requested Merge-Merged
i merged this scott. Committed revision 235768
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 18 2013

Labels: merge-merged-1700
------------------------------------------------------------------------
r235768 | karen@chromium.org | 2013-11-18T18:33:35.317093Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.cc?r1=235768&r2=235767&pathrev=235768
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.h?r1=235768&r2=235767&pathrev=235768

Merge 235500 "Fixes cursor hiding bug"

> Fixes cursor hiding bug
> 
> OnCursorVisibilityChanged may be invoked multiple times with the same
> value. Current windows implementation is effectively ref counted. We
> don't want that, instead it should show/hide based on most recent
> call.
> 
> BUG= 317496 
> TEST=none
> R=ananta@chromium.org
> 
> Review URL: https://codereview.chromium.org/70383014

TBR=sky@chromium.org

Review URL: https://codereview.chromium.org/62333024
------------------------------------------------------------------------

Comment 21 by cpu@chromium.org, Nov 18 2013

Status: Fixed
Cc: rponnada@chromium.org
Labels: Needs-Feedback
Tested this issue on Windows7 using 32.0.1700.76 (Official Build 244343), Issue is not fixed, Mouse pointer is not restored when Fullscreen mode is escaped.

Please confirm whether it is merged into 1700 or not??
Status: Untriaged

Comment 24 by sky@chromium.org, Jan 13 2014

It was merged to 32 on Mon Nov 18. Could you run a bisect on 32 builds to see if it regressed or never worked?
Cc: ligim...@chromium.org
Labels: -TE-Verified-33.0.1712.0 -Te-Verified-M33
Status: Assigned
This issue is not fixed , now noticed that cursor disappears after multiple attempts.  
sorry for the update in comment #18.
Tried the bisect again , regression window is same as in the original report.

Good Build : 32.0.1690.0 (Official Build 232627) Aura
Bad Build : 32.0.1700.0 (Official Build 232870) Aura

CL
===
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=232650%3A232669

Assigning to sky@

Project Member

Comment 26 by bugdroid1@chromium.org, Jan 13 2014

------------------------------------------------------------------------
r244577 | sky@chromium.org | 2014-01-13T20:59:25.640070Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/ui/views/corewm/compound_event_filter.cc?r1=244577&r2=244576&pathrev=244577

Merge 234741 "Gets mouse pointer to show again when existing ful..."

> Gets mouse pointer to show again when existing fullscreen
> 
> My change at r232662 broke showing of the cursor when exiting
> fullscreen. We always need to attempt to show the cursor, otherwise it
> can get stuck hidden.
> 
> BUG= 317496 
> TEST=see bug
> R=ananta@chromium.org
> 
> Review URL: https://codereview.chromium.org/66333010

TBR=sky@chromium.org

Review URL: https://codereview.chromium.org/137393002
------------------------------------------------------------------------
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 13 2014

Labels: merge-merged-1700_72
------------------------------------------------------------------------
r244578 | sky@chromium.org | 2014-01-13T21:00:07.517363Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1700_72/src/ui/views/corewm/compound_event_filter.cc?r1=244578&r2=244577&pathrev=244578

Merge 234741 "Gets mouse pointer to show again when existing ful..."

> Gets mouse pointer to show again when existing fullscreen
> 
> My change at r232662 broke showing of the cursor when exiting
> fullscreen. We always need to attempt to show the cursor, otherwise it
> can get stuck hidden.
> 
> BUG= 317496 
> TEST=see bug
> R=ananta@chromium.org
> 
> Review URL: https://codereview.chromium.org/66333010

TBR=sky@chromium.org

Review URL: https://codereview.chromium.org/137183003
------------------------------------------------------------------------

Comment 28 by sky@chromium.org, Jan 15 2014

Status: Fixed
Status: Assigned
Still i am able to reproduce this issue (noticed that cursor disappears after multiple attempts) on Latest M32#32.0.1700.102 (Official Build 246481) m on Win7 64 bit.

Thank you.

Comment 30 by sky@chromium.org, Jan 23 2014

manoranjanr@chromium.org, what are the repro steps?
Below are the repro steps:
==========================
1. Launch Chrome and navigate to "http://scheib.github.com/HTMLMisc/PointerLockAndFullscreen.html"
2. Click on "Fullscreen and Lock pointer Target 1" under Target 1 section.
3. Allow to disable mouse pointer in Fullscreen mode and press escape (Esc) key.
4. Repeat step 2 and again press Esc key, observe.

Repeat step4 for more than 3 / 4 times will trigger the issue.

Thank you.

Comment 32 by Deleted ...@, Jan 28 2014

Clearly the complaint has been heard, and clearly nothing is being done about it. I like steppers in my scroll bar, so i'm moving back to firefox.

Comment 33 by sky@chromium.org, Jan 29 2014

mrpaulyman, scrollbar steppers are being added back here: 333499.

Comment 34 by sky@chromium.org, Jan 29 2014

Status: Started
This still happens on trunk too (34.0.1810.0).

Comment 35 by sky@chromium.org, Jan 29 2014

Owner: tdander...@chromium.org
Since this happens on trunk and Terry has done the more work on this, I'm passing to him.
Taking a look now. I'll update the bug with findings.
Cc: ananta@chromium.org sky@chromium.org
Patch: https://codereview.chromium.org/151563004
Labels: Iteration-99
Project Member

Comment 39 by bugdroid1@chromium.org, Feb 6 2014

------------------------------------------------------------------------
r249310 | tdanderson@chromium.org | 2014-02-06T09:29:39.096610Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.cc?r1=249310&r2=249309&pathrev=249310
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/widget/desktop_aura/desktop_root_window_host_win.h?r1=249310&r2=249309&pathrev=249310

Make the member |is_cursor_visible_| in DesktopWindowTreeHostWin static.

On Windows we can have multiple root windows and the implementation
of ::ShowCursor() is based on a counter, so making this member static
ensures that ::ShowCursor() is always called exactly once whenever the
cursor visibility state changes. This prevents an incorrectly hidden
(or incorrectly shown) cursor because the number of calls to
::ShowCursor() is no longer dependent on the number of instantiated
root windows at the time when a request to change the cursor visibility
is made.

BUG= 317496 

Review URL: https://codereview.chromium.org/151563004
------------------------------------------------------------------------
Cc: kareng@google.com rbyers@chromium.org
Labels: -M-32 M-33
I have verified that r249310 has fixed the problem in 34.0.1827.0 (Official Build 249550) canary.

No more merges into 32 are being allowed, so I'm updating this to be 33.

I am hesitant to request a merge of this into 33 because in order for me to have confidence this will correctly fix the issue, I would also have to merge in r240827. This second patch was previously reverted from 33 (see r243665) and given its complexity it doesn't seem like a good idea to try and re-land in 33 at this point.

Tom, Ananta, or Scott: do you think the severity of this issue would justify a merge request of both r240827 and r249310 into 33?
Cc: wiltzius@chromium.org
+Tom (please see comment #40)
I don't think we want to merge r240827. Does r249310 fix this, other than in touch situations without r240827? Perhaps it'd be worth building 33, patching just r249310 in and checking that. https://sites.google.com/a/chromium.org/dev/developers/how-tos/get-the-code#Working_with_release_branches

(It does seem relatively bad given that it affects the whole browser, not just the tab, but I don't know how often and where fullscreen + pointerlock is used.)
r240827 isn't just for touch scenarios, and this issue doesn't actually have anything to do with touch. 

Without r240827, my concern is that in the process of exiting fullscreen (and thus releasing pointer lock) we could be in a situation where the root window for which we are trying to show the cursor has been destroyed, which means the cursor would remain hidden. r240827 ensures that the request to show the cursor will always reach DesktopWindowTreeHostWin::OnCursorVisibilityChanged(), and r249310 ensures a correct number of calls to the Windows ::ShowCursor() / ::HideCursor() functions.

I am in the process of building 33 with just r249310 to see what happens but am running into a few road blocks, so stay tuned.
I am unable to reproduce this in M33, even before applying r240827 or r249310. 

yfulgaonkar@ or smokana@: Can you please check if you can get this issue to reproduce in 33 and provide bisect/version information?

My version:
33.0.1750.88 (Developer Build 250193) Win8 desktop
Labels: -Needs-Feedback
Above issue was reproducible till build 33.0.1711.3 and it seems to be fixed after 33.0.1712.0
Cc: tdander...@chromium.org agautam@chromium.org scheib@chromium.org
 Issue 319460  has been merged into this issue.
Cc: msrchandra@chromium.org
Issue 335857 has been merged into this issue.
Labels: -M-33 M-34
Status: Fixed
Here's where we stand on this issue:

* It is still reproducible on M32, but merges into 32 have been cut off so there isn't anything we can do about that.

* It does not reproduce in M33, so I'm updating this to be an M34 bug. Unfortunately even after looking through the logs I am not sure what fixed and then re-broke this again. (If anyone does observe this reproducing in M33, please re-open this bug and I will investigate.)

* This was reproducing in M34, but I have verified that r249310 has fixed the problem in 34.0.1827.0 (Official Build 249550) canary, so I'm marking this as Fixed.
 Issue 332027  has been merged into this issue.

Sign in to add a comment