Invoke cursor-hiding logic on touch for Windows

Invoke cursor-hiding logic on touch for Windows

Continue to invoke cursor-hiding logic on touch
for Windows even though the OS hides the cursor
for us. The reason is that this code path is
needed in order to call
RenderWidgetHostViewAura::
    NotifyRendererOfCursorVisibilityState()
in order for the renderer to know to suppress
generation of synthetic mousemove events
while touch scrolling. This avoids unwanted
hover effects from being invoked when the
cursor is not visible.

To avoid regressions in behavior (such as
the one mentioned in
https://codereview.chromium.org/56053004),
make the cursor state global among all
root windows. This is done in
https://codereview.chromium.org/111043002/.

BUG=322250
TEST=To be added later (crbug.com/328549)
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240976

[tdanderson, 2013-11-22 02:03:35]

Scott, can you please take a look at my comment in DesktopRootWindowHostWin?

https://codereview.chromium.org/82463002/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/82463002/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:733:
::ShowCursor(true);
Adding this line will prevent the problem you described in
https://codereview.chromium.org/56053004 from occurring. If you tap on the
wrench menu and then tap on the page to dismiss the menu, the cursor becomes
visible when moved. Without this line, the cursor remains invisible when moved
within the browser window.

However, I am not entirely sure why this works.

[tdanderson, 2013-11-22 02:27:03]

https://codereview.chromium.org/82463002/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/82463002/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:733:
::ShowCursor(true);
On 2013/11/22 02:03:36, tdanderson wrote:
> Adding this line will prevent the problem you described in
> https://codereview.chromium.org/56053004 from occurring. If you tap on the
> wrench menu and then tap on the page to dismiss the menu, the cursor becomes
> visible when moved. Without this line, the cursor remains invisible when moved
> within the browser window.
> 
> However, I am not entirely sure why this works.

I should add that the |delegate_| in CursorManager is a
DesktopNativeCursorManager, which only knows about a single RootWindow. Ideally,
upon a cursor visibility change, I would like to iterate over all root windows
to inform their cursor clients of the change (instead of just a single root
window). But this doesn't seem possible given that we're operating in desktop
mode.

[sky, 2013-11-22 14:45:37]

Seems to me the bug is in the code assuming there is a single CursorClient.
CursorManager could keep around a static vector of all the CursorManagers and
notify each.

Also, write tests for what is happening.

https://codereview.chromium.org/82463002/diff/1/ui/views/corewm/compound_even...
File ui/views/corewm/compound_event_filter.cc (right):

https://codereview.chromium.org/82463002/diff/1/ui/views/corewm/compound_even...
ui/views/corewm/compound_event_filter.cc:74: bool ShouldHideCursorOnTouch() {
You can remove this whole function now.

[tdanderson, 2013-11-25 21:22:34]

@sky, can you please comment on the approach in Patch Set 2? (I will add tests
once the general approach gets your approval).

This change makes cursor state global for Desktop, while at the same time
maintaining the property that a DesktopNativeCursorManager interacts with only
one RootWindow.

An alternative could be to re-write DesktopNativeCursorManager so that it
interacts with all root windows (by perhaps storing a static vector of root
windows), but I do not know if this is desirable.

[scottmg, 2013-11-27 17:47:32]

On 2013/11/25 21:22:34, tdanderson wrote:
> @sky, can you please comment on the approach in Patch Set 2? (I will add tests
> once the general approach gets your approval).
> 
> This change makes cursor state global for Desktop, while at the same time
> maintaining the property that a DesktopNativeCursorManager interacts with only
> one RootWindow.
> 
> An alternative could be to re-write DesktopNativeCursorManager so that it
> interacts with all root windows (by perhaps storing a static vector of root
> windows), but I do not know if this is desirable.

This approach fixes https://code.google.com/p/chromium/issues/detail?id=312204
and makes https://codereview.chromium.org/83743004/ unnecessary.

[tdanderson, 2013-11-27 18:30:18]

On 2013/11/27 17:47:32, scottmg wrote:
> On 2013/11/25 21:22:34, tdanderson wrote:
> > @sky, can you please comment on the approach in Patch Set 2? (I will add
tests
> > once the general approach gets your approval).
> > 
> > This change makes cursor state global for Desktop, while at the same time
> > maintaining the property that a DesktopNativeCursorManager interacts with
only
> > one RootWindow.
> > 
> > An alternative could be to re-write DesktopNativeCursorManager so that it
> > interacts with all root windows (by perhaps storing a static vector of root
> > windows), but I do not know if this is desirable.
> 
> This approach fixes https://code.google.com/p/chromium/issues/detail?id=312204
> and makes https://codereview.chromium.org/83743004/ unnecessary.

I should also add that this *doesn't* actually fix the problem I was
experiencing (cursor permanently hidden after a tap on the wrench
menu followed by a tap on the web contents).

[sky, 2013-12-02 15:46:07]

Any reason why that is?

On Wed, Nov 27, 2013 at 10:30 AM,  <tdanderson@chromium.org> wrote:
> On 2013/11/27 17:47:32, scottmg wrote:
>>
>> On 2013/11/25 21:22:34, tdanderson wrote:
>> > @sky, can you please comment on the approach in Patch Set 2? (I will add
>
> tests
>>
>> > once the general approach gets your approval).
>> >
>> > This change makes cursor state global for Desktop, while at the same
>> > time
>> > maintaining the property that a DesktopNativeCursorManager interacts
>> > with
>
> only
>>
>> > one RootWindow.
>> >
>> > An alternative could be to re-write DesktopNativeCursorManager so that
>> > it
>> > interacts with all root windows (by perhaps storing a static vector of
>> > root
>> > windows), but I do not know if this is desirable.
>
>
>> This approach fixes
>> https://code.google.com/p/chromium/issues/detail?id=312204
>> and makes https://codereview.chromium.org/83743004/ unnecessary.
>
>
> I should also add that this *doesn't* actually fix the problem I was
> experiencing (cursor permanently hidden after a tap on the wrench
> menu followed by a tap on the web contents).
>
> https://codereview.chromium.org/82463002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tdanderson, 2013-12-03 00:13:30]

@sky and @scottmg,

Note that for the global cursor state I am going with the implementation in
https://codereview.chromium.org/92413002 rather than the one appearing in PS3 of
this CL.

After a bit more digging it looks like the cursor state confusion is due to two
mouse events being dispatched when I tap on the wrench menu. These were being
treated as "real" mouse events in CompoundEventFilter::OnMouseEvent() and
causing the cursor state to be set incorrectly. Synthetic mouse events should
never change the state of the cursor, so in PS4 I added a check for this in
CompoundEventFilter.

The first mouse event is coming from RootWindow::UpdateCapture(). Flagging this
as synthetic ensures it doesn't change the cursor state, which I did in PS4.

The second mouse event appears to be generated from the Windows OS and passes
through MenuController::Dispatch(const MSG& msg). This is a WM_MOUSEMOVE event
for which ui::IsMouseEventFromTouch() returns false, so it does not get flagged
as EF_FROM_TOUCH (even though it should be since it was as a result of tapping
on the wrench menu).

I have yet to figure out why this second message is being generated or why
ui::IsMouseEventFromTouch() is not returning true. This looks like it's the only
thing blocking me from resolving issue 322250 and at the same time making sure
that a touch-initiated menu will not permanently hide the cursor if the menu is
dismissed with touch (I observe this behavior happening even if I long-press to
show a context menu and then dismiss with touch).

[tdanderson, 2013-12-13 21:51:51]

@sky, can you PTAL? This is the follow-up to
https://codereview.chromium.org/111043002/.

[tdanderson, 2013-12-13 22:24:39]

On 2013/12/13 21:51:51, tdanderson wrote:
> @sky, can you PTAL? This is the follow-up to
> https://codereview.chromium.org/111043002/.

As discussed, test coverage is to be added later. Filed crbug.com/328549 to
track.

[sky, 2013-12-13 22:35:24]

https://codereview.chromium.org/82463002/diff/200001/ui/views/corewm/compound...
File ui/views/corewm/compound_event_filter.cc (right):

https://codereview.chromium.org/82463002/diff/200001/ui/views/corewm/compound...
ui/views/corewm/compound_event_filter.cc:74: bool ShouldHideCursorOnTouch() {
Is this function even needed anymore? I believe you're fixing the behavior I
added it for. If it is needed the comment needs to be updated. Also, I seem to
remember ifdefing some tests because of this.

[tdanderson, 2013-12-13 22:52:46]

It is needed because of Linux Aura. Removing this function introduces some
bugs, and since touch for Linux Aura is still in its early stages I was
going to discuss this with erg@ and flackr@ at a later time. Ideally it
will be removed in the future so we have consistency.

I am AFK until tomorrow/Sunday so I  will update the comment and tests at
that time.

Unless you have other concerns about this, would you mind giving an lgtm
with nits so I can get this in the CQ over the weekend?
On Dec 13, 2013 5:35 PM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/82463002/diff/200001/ui/
> views/corewm/compound_event_filter.cc
> File ui/views/corewm/compound_event_filter.cc (right):
>
> https://codereview.chromium.org/82463002/diff/200001/ui/
> views/corewm/compound_event_filter.cc#newcode74
> ui/views/corewm/compound_event_filter.cc:74: bool
> ShouldHideCursorOnTouch() {
> Is this function even needed anymore? I believe you're fixing the
> behavior I added it for. If it is needed the comment needs to be
> updated. Also, I seem to remember ifdefing some tests because of this.
>
> https://codereview.chromium.org/82463002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-12-13 23:18:18]

LGTM - but make sure you reinclude the appropriate set of tests for windows, I'm
pretty sure disabled a couple because of this.

[commit-bot: I haz the power, 2013-12-15 19:03:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/220001

[commit-bot: I haz the power, 2013-12-15 19:39:35]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-15 19:42:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/220001

[commit-bot: I haz the power, 2013-12-15 20:21:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-15 21:23:37]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-15 21:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-15 22:57:56]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-15 23:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-15 23:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-16 00:49:47]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 00:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-16 02:31:27]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 02:33:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-16 04:08:36]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 16:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-16 17:15:22]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 17:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/82463002/240001

[commit-bot: I haz the power, 2013-12-16 19:18:05]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[tdanderson, 2013-12-16 19:44:09]

Committed patchset #7 manually as r240976 (presubmit successful).