Do not show the mouse cursor when the display is rotated in TouchView

Do not show the mouse cursor when the display is rotated in TouchView

BUG=462883
TEST=Manual, see bug

Committed: https://crrev.com/3aa7a6eab4d102ae3bb54c943dca7fb546dfdd98
Cr-Commit-Position: refs/heads/master@{#322010}

[pkotwicz, 2015-03-03 03:56:33]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-03-03 03:58:35]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2015-03-03 04:00:14]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2015-03-03 05:07:42]

Patchset #1 (id:50003) has been deleted

[pkotwicz, 2015-03-03 05:11:26]

Patchset #2 (id:90001) has been deleted

[pkotwicz, 2015-03-03 05:13:45]

Patchset #2 (id:110001) has been deleted

[pkotwicz, 2015-03-03 20:50:36]

pkotwicz@chromium.org changed reviewers:
+ oshima@chromium.org

[pkotwicz, 2015-03-03 20:50:37]

Oshima, can you please take a look?

I decided against making WindowTreeHost::MoveCursorTo() dispatch a synthetic
mouse move on ChromeOS Ozone. We should strive to keep consistent behavior in
cross platform APIs

[oshima, 2015-03-04 00:12:53]

I'm fine with that too. lgtm

[pkotwicz, 2015-03-04 00:13:50]

pkotwicz@chromium.org changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2015-03-04 00:13:50]

Sadrul for ui/aura

[sadrul, 2015-03-04 00:53:49]

https://codereview.chromium.org/971753008/diff/130001/ash/display/display_con...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/971753008/diff/130001/ash/display/display_con...
ash/display/display_controller.cc:613: // cursor's visibility.
Can you update the comment to explain why we want a synth mouse-move to be
dispatched?

[pkotwicz, 2015-03-04 16:30:37]

Sadrul, can you please take another look?

I have updated the comment. We need to dispatch a synthetic mouse move to update
hover effects. We don't do this correctly either with or without this CL.
(Rotating the screen does leave stray hover effects). The mouse move needs to be
synthetic because we don't want to show the mouse cursor on rotation

[sadrul, 2015-03-05 09:42:37]

On 2015/03/04 16:30:37, pkotwicz wrote:
> Sadrul, can you please take another look?
> 
> I have updated the comment. We need to dispatch a synthetic mouse move to
update
> hover effects. We don't do this correctly either with or without this CL.
> (Rotating the screen does leave stray hover effects).

Two questions: (1) I think aura dispatches a synth mouse-move when a window is
transformed. Does that not work correctly in this case? (2) If dispatching the
event doesn't fix what it's intended to, why dispatch it?

> The mouse move needs to be
> synthetic because we don't want to show the mouse cursor on rotation

[pkotwicz, 2015-03-05 23:10:24]

Sadrul, can you please take another look? Thank you for your questions, they
made me dig deeper into the code.

(1) I think aura dispatches a synth mouse-move when a window is
> transformed. Does that not work correctly in this case?
- WindowEventDispatcher does not observe WindowTreeHost::window()
- WindowEventDispatcher::OnWindowBoundsChanged() clears
|WindowEventDispatcher::synthesize_mouse_move_|. aura::Window::SetBounds() is
called from TransformerHelper::SetRootWindowTransformer after the aura::Window's
transform is set
- Relying on WindowEventDispatcher::OnWindowTransformed() to synthesize a mouse
move when we are changing the screen position in
DisplayController::EnsurePointerInDisplays() is super subtle

(2) If dispatching the
> event doesn't fix what it's intended to, why dispatch it?
- We need to dispatch a synthetic mouse move to update the composited cursor's
location
- The behavior is worse if we do not dispatch the mouse move. If we do not
dispatch the synthetic mouse move, in addition to leaving stray hover effects,
there are no hover effects at the mouse's new position

[sadrul, 2015-03-06 14:27:01]

Thanks for digging into this!

On 2015/03/05 23:10:24, pkotwicz wrote:
> Sadrul, can you please take another look? Thank you for your questions, they
> made me dig deeper into the code.
> 
> (1) I think aura dispatches a synth mouse-move when a window is
> > transformed. Does that not work correctly in this case?
> - WindowEventDispatcher does not observe WindowTreeHost::window()
> - WindowEventDispatcher::OnWindowBoundsChanged() clears
> |WindowEventDispatcher::synthesize_mouse_move_|. aura::Window::SetBounds() is
> called from TransformerHelper::SetRootWindowTransformer after the
aura::Window's
> transform is set

Just to clarify, does this mean observing WTH::window() from WED wouldn't still
work?

> - Relying on WindowEventDispatcher::OnWindowTransformed() to synthesize a
mouse
> move when we are changing the screen position in
> DisplayController::EnsurePointerInDisplays() is super subtle
> 
> (2) If dispatching the
> > event doesn't fix what it's intended to, why dispatch it?
> - We need to dispatch a synthetic mouse move to update the composited cursor's
> location
> - The behavior is worse if we do not dispatch the mouse move. If we do not
> dispatch the synthetic mouse move, in addition to leaving stray hover effects,
> there are no hover effects at the mouse's new position

[sadrul, 2015-03-06 14:29:22]

On 2015/03/06 14:27:01, sadrul wrote:
> Thanks for digging into this!
> 
> On 2015/03/05 23:10:24, pkotwicz wrote:
> > Sadrul, can you please take another look? Thank you for your questions, they
> > made me dig deeper into the code.
> > 
> > (1) I think aura dispatches a synth mouse-move when a window is
> > > transformed. Does that not work correctly in this case?
> > - WindowEventDispatcher does not observe WindowTreeHost::window()
> > - WindowEventDispatcher::OnWindowBoundsChanged() clears
> > |WindowEventDispatcher::synthesize_mouse_move_|. aura::Window::SetBounds()
is
> > called from TransformerHelper::SetRootWindowTransformer after the
> aura::Window's
> > transform is set
> 
> Just to clarify, does this mean observing WTH::window() from WED wouldn't
still
> work?

The reason I slightly dislike exposing PostSynthesizeMouseMove() is that I feel
like it is an internal detail of how WED does its thing, and it doesn't feel
like a good thing to expose it to the public API.

> 
> > - Relying on WindowEventDispatcher::OnWindowTransformed() to synthesize a
> mouse
> > move when we are changing the screen position in
> > DisplayController::EnsurePointerInDisplays() is super subtle
> > 
> > (2) If dispatching the
> > > event doesn't fix what it's intended to, why dispatch it?
> > - We need to dispatch a synthetic mouse move to update the composited
cursor's
> > location
> > - The behavior is worse if we do not dispatch the mouse move. If we do not
> > dispatch the synthetic mouse move, in addition to leaving stray hover
effects,
> > there are no hover effects at the mouse's new position

[pkotwicz, 2015-03-06 15:36:51]

I don't mind exposing PostSynthesizeMouseMove() because it is trivial to
replicate the functionality. (i.e. writing a method which dispatches a synthetic
mouse move on a posted task is trivial). I can create a method in
DisplayController which does just that if you prefer.

[sadrul, 2015-03-08 01:21:07]

On 2015/03/06 15:36:51, pkotwicz wrote:
> I don't mind exposing PostSynthesizeMouseMove() because it is trivial to
> replicate the functionality. (i.e. writing a method which dispatches a
synthetic
> mouse move on a posted task is trivial). I can create a method in
> DisplayController which does just that if you prefer.

lolwut no

Hopefully it isn't actually trivial, but regardless, you shouldn't do that.

[pkotwicz, 2015-03-08 03:38:33]

Sadrul, which approach do you suggest?

[sadrul, 2015-03-08 03:57:04]

On 2015/03/08 03:38:33, pkotwicz wrote:
> Sadrul, which approach do you suggest?

From earlier comment:

> Just to clarify, does this mean observing WTH::window() from WED wouldn't
still
> work?

I would like for us to try observing WTH::window() too from WED, and try to make
that work.

[pkotwicz, 2015-03-08 16:35:22]

That sounds like a hacky solution. DisplayController::EnsurePointerInDisplays()
calls aura::Env::set_last_mouse_location(). It should be the one dispatching the
synthetic mouse move. We should not rely on WindowEventDispatcher (or anything
else) to do this

[pkotwicz, 2015-03-09 21:30:13]

We call aura::Window::MoveCursorTo() from RenderWidgetHostViewAura::LockMouse()
so we can't make moving the cursor an Ash/CrOS only thing as I previously stated

[pkotwicz, 2015-03-09 21:49:22]

The best solution might be to have DisplayController::EnsurePointerInDisplays()
call WindowEventDispatcher::OnCursorMovedToRootLocation() and have
WindowEventDispatcher::OnCursorMovedToRootLocation() dispatch a synthetic mouse
move

[pkotwicz, 2015-03-11 18:40:35]

Sadrul, Ping!

[sadrul, 2015-03-12 17:34:33]

On 2015/03/11 18:40:35, pkotwicz wrote:
> Sadrul, Ping!

Peter and I discussed offline about the various issues regarding MoveCursorTo,
the async events it can generate on X11, cursor visibility etc. And it's clear
that there are a lot of edge cases to worry about. I can't think of a good
solution for all of them. I am still particularly thrilled about this fix
either, so here's one last alternate workaround I would propose: would it make
sense for touchview to set the cursor on the WTHosts to kCursorNone to make sure
the possibly async events generated from MoveCursorTo doesn't make the cursor
visible again?

Also, this seems like a highly brittle area. Should we have some amount of test
coverage for the various edge-cases?

[oshima, 2015-03-12 19:00:19]

On 2015/03/12 17:34:33, sadrul wrote:
> On 2015/03/11 18:40:35, pkotwicz wrote:
> > Sadrul, Ping!
> 
> Peter and I discussed offline about the various issues regarding MoveCursorTo,
> the async events it can generate on X11, cursor visibility etc. And it's clear
> that there are a lot of edge cases to worry about. I can't think of a good
> solution for all of them. I am still particularly thrilled about this fix
> either, so here's one last alternate workaround I would propose: would it make
> sense for touchview to set the cursor on the WTHosts to kCursorNone to make
sure
> the possibly async events generated from MoveCursorTo doesn't make the cursor
> visible again?

I believe we do want to show the cursor if a user did move the cursor even in
touch view.

We show the mouse cursor only on physical mouse move, not synthetic one.
On ozone, isn't it possible to change MoveCursorTo to generate synthetic mouse
move event IF
it doesn't move the physical location, but generate real one if it does move its
physical location?
I think this matches the expectation.

> 
> Also, this seems like a highly brittle area. Should we have some amount of
test
> coverage for the various edge-cases?

Agreed. It should be easy to add more tests with the approach above.

[pkotwicz, 2015-03-12 19:34:38]

I tried sadrul@'s suggestion and it does not work. sadrul@, I can explain why it
doesn't work in more detail if you want.

I do not want WindowTreeHost::MoveCursorTo() to do something on Ozone and do
something different on other platforms. This is a price that we pay for having
cross platform APIs

My suggestion: (from comment #24)
"The best solution might be to have DisplayController::EnsurePointerInDisplays()
call WindowEventDispatcher::OnCursorMovedToRootLocation() and have
WindowEventDispatcher::OnCursorMovedToRootLocation() dispatch a synthetic mouse
move"

I really really really want to add tests for this. I think that the best that I
can do is test the implementation of ui::TestWindow on Ozone. Maybe having a
test which tests that the bug does not exist when using the ui::TestWindow
implementation is still valuable?

[oshima, 2015-03-12 19:49:37]

On 2015/03/12 19:34:38, pkotwicz wrote:
> I tried sadrul@'s suggestion and it does not work. sadrul@, I can explain why
it
> doesn't work in more detail if you want.
> 
> I do not want WindowTreeHost::MoveCursorTo() to do something on Ozone and do
> something different on other platforms. This is a price that we pay for having
> cross platform APIs

I consider my suggestion is expected and right behavior, and it's just that X11
doesn't provide a good way to implement it.
In X11, we can use XQueryPointer with caveat that it synchronous operation to
Xserver, and async issue remains. (but this won't be
an issue for rotate case)

> 
> My suggestion: (from comment #24)
> "The best solution might be to have
DisplayController::EnsurePointerInDisplays()
> call WindowEventDispatcher::OnCursorMovedToRootLocation() and have
> WindowEventDispatcher::OnCursorMovedToRootLocation() dispatch a synthetic
mouse
> move"
> 
> I really really really want to add tests for this. I think that the best that
I
> can do is test the implementation of ui::TestWindow on Ozone. Maybe having a
> test which tests that the bug does not exist when using the ui::TestWindow
> implementation is still valuable?

[pkotwicz, 2015-03-12 20:11:47]

My understanding is that Windows does not provide a good way of dispatching a
synthetic mouse move either from WindowTreeHost::MoveCursorTo()

[pkotwicz, 2015-03-17 18:40:12]

Patchset #4 (id:170001) has been deleted

[pkotwicz, 2015-03-17 18:42:34]

Sadrul, can you please take another look? I have have implemented the solution
described in comment #24.

[pkotwicz, 2015-03-19 03:08:05]

Sadrul, Ping!

[sadrul, 2015-03-20 16:58:07]

This needs tests

[pkotwicz1, 2015-03-20 17:42:50]

tldr: I would rather add tests in a separate CL.

I can't think of tests which are easy to write (i.e. which do not require moving
WindowTreeHost::MoveCursorTo() out of WindowTreeHost) The reason that it is
difficult with the current architecture is that there is no easy way of
replacing WindowTreeHost::MoveCursorTo() with a test implementation on all
platforms.

I think that it would be easier to have a stub implementation for moving the
cursor if we added CursorManager::MoveCursorTo() to the CursorManager API and
AshNativeCursorManager owned an instance of CursorMover. CursorMover would be a
new class whose responsibility is to move the cursor. Every platform would have
a different version of CursorMover. For tests, we can replace
AshNativeCursorManager's CursorMover with a test implementation.

If we decide to make the test implementation for moving the cursor asynchronous
(TestCursorMover::MoveCursorTo()) this would still be clunky. The test would
need to wait till TestCursorMover::MoveCursorTo() dispatched the move.
Alternatively the test could call RunAllPendingInMessageLoop() which sky@ does
not like - see https://codereview.chromium.org/891213002/)

We have many tests which test what happens to the cursor position, cursor scale
factor and cursor rotation when the display configuration changes. For instance,
we have a test which checks that the cursor is rotated when the display is
rotated. (AshNativeCursorManagerTest.SetDeviceScaleFactorAndRotation) We do not
have any tests which check for the side effects of
WindowTreeHost::MoveCursorTo() dispatching a mouse move. The closest thing we
have is CursorWindowControllerTest.MoveToDifferentDisplay - which I committed
last week

[sadrul, 2015-03-20 17:54:50]

On 2015/03/20 17:42:50, pkotwicz22 wrote:
> tldr: I would rather add tests in a separate CL.
> 
> I can't think of tests which are easy to write (i.e. which do not require
moving
> WindowTreeHost::MoveCursorTo() out of WindowTreeHost) The reason that it is
> difficult with the current architecture is that there is no easy way of
> replacing WindowTreeHost::MoveCursorTo() with a test implementation on all
> platforms.
> 
> I think that it would be easier to have a stub implementation for moving the
> cursor if we added CursorManager::MoveCursorTo() to the CursorManager API and
> AshNativeCursorManager owned an instance of CursorMover. CursorMover would be
a
> new class whose responsibility is to move the cursor. Every platform would
have
> a different version of CursorMover. For tests, we can replace
> AshNativeCursorManager's CursorMover with a test implementation.
> 
> If we decide to make the test implementation for moving the cursor
asynchronous
> (TestCursorMover::MoveCursorTo()) this would still be clunky. The test would
> need to wait till TestCursorMover::MoveCursorTo() dispatched the move.
> Alternatively the test could call RunAllPendingInMessageLoop() which sky@ does
> not like - see https://codereview.chromium.org/891213002/)
> 
> We have many tests which test what happens to the cursor position, cursor
scale
> factor and cursor rotation when the display configuration changes. For
instance,
> we have a test which checks that the cursor is rotated when the display is
> rotated. (AshNativeCursorManagerTest.SetDeviceScaleFactorAndRotation) We do
not
> have any tests which check for the side effects of
> WindowTreeHost::MoveCursorTo() dispatching a mouse move. The closest thing we
> have is CursorWindowControllerTest.MoveToDifferentDisplay - which I committed
> last week

We have a few aura unit-tests that call MoveCursorTo(). Can you not add a check
there that we receive the synthesized-mouse move afterwards?

We are switching the behaviour here, from explicitly cancelling any pending
dispatch of a synthesized mouse-move, to requesting a dispatch of synthesized
mouse-move. It will be a bit disappointing if this doesn't break some tests
(also: please send out tryjobs when you feel a CL is ready for review), but we
should try hard to add tests to make sure this behaviour doesn't accidentally
change.

[pkotwicz, 2015-03-23 02:38:57]

Patchset #5 (id:210001) has been deleted

[pkotwicz, 2015-03-23 02:39:17]

Patchset #4 (id:190001) has been deleted

[pkotwicz, 2015-03-23 14:41:29]

Patchset #4 (id:230001) has been deleted

[pkotwicz, 2015-03-23 16:22:16]

Sadrul, can you please take another look? I have added a test as you requested

[sadrul, 2015-03-24 07:56:27]

lgtm

[pkotwicz, 2015-03-24 14:39:42]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-03-24 14:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/971753008/#ps250001
(title: " ")

[commit-bot: I haz the power, 2015-03-24 14:40:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/971753008/250001

[commit-bot: I haz the power, 2015-03-24 16:03:06]

Committed patchset #4 (id:250001)

[commit-bot: I haz the power, 2015-03-24 16:03:44]

Patchset 4 (id:??) landed as
https://crrev.com/3aa7a6eab4d102ae3bb54c943dca7fb546dfdd98
Cr-Commit-Position: refs/heads/master@{#322010}