Chrome OS Accessibility: enable cursor compositing when screen magnifier is enabled.

Chrome OS Accessibility: enable cursor compositing when screen magnifier is enabled.

We should enable cursor compositng when screen magnifier is enabled so that the cursor
is easier to see.

BUG=217847
TEST=manually verify that cursor is magnified with screen magnifier on.
R=derat@chromium.org, oshima@chromium.org

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

[hshi1, 2014-02-10 20:39:39]

@oshima and @derat - PTAL. This CL intends to enable cursor compositing mode on
Chrome OS when screen magnifier is enabled. It expands the existing behavior
where we already enable cursor compositing mode for large-cursor mode and
high-contrast mode. Thanks!

[Daniel Erat, 2014-02-10 21:47:55]

lgtm

[hshi1, 2014-02-11 02:23:53]

+sadrul@ (OWNER of ui/aura)

The reason for the change in RootWindow::PreDispatchLocatedEvent() is to prevent
an infinite feedback loop when both screen magnifier and cursor compositing are
enabled. The sequence is like this:

a mouse move event calls ash::internal::CursorWindowController::UpdateLocation()
 which calls aura::Window::OnWindowBoundsChanged() which calls
aura::RootWindow::PostMouseMoveEventAfterWindowChange()
that generates a synthesized MOUSE_MOVE event that feeds back to
RootWindow::PreDispatchLocatedEvent and calling SetLastMouseLocation, that
triggers cursor window controller to update location again.

When screen magnifier is enabled with a non-integer scale factor, we'll see
mouse moving by itself due to accumulated errors in converting coordinates from
floating point to integers.

[sadrul, 2014-02-11 02:33:32]

Thanks for the detailed explanation. The change itself looks reasonable, but a
question below:

On 2014/02/11 02:23:53, hshi1 wrote:
> +sadrul@ (OWNER of ui/aura)
> 
> The reason for the change in RootWindow::PreDispatchLocatedEvent() is to
prevent
> an infinite feedback loop when both screen magnifier and cursor compositing
are
> enabled. The sequence is like this:
> 
> a mouse move event calls
ash::internal::CursorWindowController::UpdateLocation()
>  which calls aura::Window::OnWindowBoundsChanged() which calls
> aura::RootWindow::PostMouseMoveEventAfterWindowChange()

This sounds like a bug? The mouse-move event is synthesized to make sure that
the cursor gets updated (and the window under the cursor gets the correct hover
effect in the right location). This shouldn't apply for the cursor window,
right? Is there a way to not trigger this event for the cursor-window?


> that generates a synthesized MOUSE_MOVE event that feeds back to
> RootWindow::PreDispatchLocatedEvent and calling SetLastMouseLocation, that
> triggers cursor window controller to update location again.
> 
> When screen magnifier is enabled with a non-integer scale factor, we'll see
> mouse moving by itself due to accumulated errors in converting coordinates
from
> floating point to integers.

[oshima, 2014-02-11 04:48:44]

On 2014/02/11 02:33:32, sadrul wrote:
> Thanks for the detailed explanation. The change itself looks reasonable, but a
> question below:
> 
> On 2014/02/11 02:23:53, hshi1 wrote:
> > +sadrul@ (OWNER of ui/aura)
> > 
> > The reason for the change in RootWindow::PreDispatchLocatedEvent() is to
> prevent
> > an infinite feedback loop when both screen magnifier and cursor compositing
> are
> > enabled. The sequence is like this:
> > 
> > a mouse move event calls
> ash::internal::CursorWindowController::UpdateLocation()
> >  which calls aura::Window::OnWindowBoundsChanged() which calls
> > aura::RootWindow::PostMouseMoveEventAfterWindowChange()
> 
> This sounds like a bug? The mouse-move event is synthesized to make sure that
> the cursor gets updated (and the window under the cursor gets the correct
hover
> effect in the right location). This shouldn't apply for the cursor window,
> right? Is there a way to not trigger this event for the cursor-window?

here is how this happens:

1) You move the mouse
2) chrome gets mouse move, which can be outside of the cursor window (which
itself generates mouse events)
3) we move the cursor window such that the current mouse location overlaps with
the cursor window,
   which generates the mouse move event.

We talked about not synthesizing events for cursor window (and it's possible),
but I think this (skipping synthesized event) is still a right thing to do
because synthesized enter/leave events may not have a correct location and the
root location of a synthesized event may have an error like this.

> 
> 
> > that generates a synthesized MOUSE_MOVE event that feeds back to
> > RootWindow::PreDispatchLocatedEvent and calling SetLastMouseLocation, that
> > triggers cursor window controller to update location again.
> > 
> > When screen magnifier is enabled with a non-integer scale factor, we'll see
> > mouse moving by itself due to accumulated errors in converting coordinates
> from
> > floating point to integers.

[sadrul, 2014-02-11 13:42:57]

Mind adding a test for this case?

[hshi1, 2014-02-11 17:37:32]

On 2014/02/11 13:42:57, sadrul wrote:
> Mind adding a test for this case?

@sadrul and @oshima - my Patch Set #2 (don't update mouse location for
synthesized events) is breaking tests badly. I believe we can't simply drop the
synthesized mouse move events as sadrul pointed out.

Can you comment on Patch Set #3? I have modified
ash/display/mouse_cursor_event_filter.cc so that we do not forward synthesized
mouse move/drag events to the CursorWindowController. This breaks the infinite
feedback look and I can confirm it fixes the issue.

[oshima, 2014-02-11 18:08:22]

On 2014/02/11 17:37:32, hshi1 wrote:
> On 2014/02/11 13:42:57, sadrul wrote:
> > Mind adding a test for this case?
> 
> @sadrul and @oshima - my Patch Set #2 (don't update mouse location for
> synthesized events) is breaking tests badly. I believe we can't simply drop
the
> synthesized mouse move events as sadrul pointed out.

LastMouseCursor should contain the location of the real mouse cursor, so it
shouldn't be necessary to set the synthesized location.
I'll look into it separately.

> 
> Can you comment on Patch Set #3? I have modified
> ash/display/mouse_cursor_event_filter.cc so that we do not forward synthesized
> mouse move/drag events to the CursorWindowController. This breaks the infinite
> feedback look and I can confirm it fixes the issue.

lgtm

[hshi1, 2014-02-11 18:09:05]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-02-11 18:09:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/150403005/290001

[hshi1, 2014-02-11 19:00:15]

Committed patchset #3 manually as r250459 (presubmit successful).