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

Issue 706200 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 686839
issue 735710

Blocking:
issue 731255



Sign in to add a comment

Get MagnificationController to work for mushrome

Project Member Reported by sky@chromium.org, Mar 28 2017

Issue description

It uses CreateRootWindowTransformerForDisplay, which hasn't been ported yet. There are tests that trigger this too, see MagnifierKeyScrollerTest.Basic.
 

Comment 1 by sky@chromium.org, Jun 8 2017

Blocking: 731255

Comment 2 by sky@chromium.org, Jul 19 2017

In theory this should work in mushrome, but it currently doesn't. Need to investigate why.

Comment 3 by sky@chromium.org, Jul 25 2017

Owner: e...@chromium.org
Status: Assigned (was: Untriaged)

Comment 4 by e...@chromium.org, Aug 10 2017

I'm able to get the magnification to turn on in --mus, but there's a bunch of issues:

- There's a duplicate cursor that stays at 1x.
- (possibly related) the viewport doesn't reliably scroll when the cursor is moved.

The actual zooming in/out hotkeys and the enabling/disabling the feature work though!

Comment 5 by e...@chromium.org, Aug 11 2017

The duplicate cursor comes from //ash/display/cursor_window_controller.cc displaying the composited window which displays the cursor in magnification cases. The problem there is that it's not communicating that it's going to composite its own cursor image back to mus so that it can hide the platform cursor.

(I still don't understand the scrolling issue though.)

Comment 6 by e...@chromium.org, Aug 11 2017

Blockedon: 735710
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 18 2017

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

commit 8b679cacf3465630201734047fd8734d711a5624
Author: Elliot Glaysher <erg@chromium.org>
Date: Fri Aug 18 17:14:16 2017

events: Fix updating an Event's root location.

This patch was written originally by sadrul@; it's been modified just to
make unit tests pass.

Bug:  735710 ,  705690 ,  706200 ,  755780 
Change-Id: I676eba681899fe3ddd8b967382738c68449881eb
Reviewed-on: https://chromium-review.googlesource.com/619567
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495593}
[modify] https://crrev.com/8b679cacf3465630201734047fd8734d711a5624/ui/aura/mus/window_tree_client_unittest.cc
[modify] https://crrev.com/8b679cacf3465630201734047fd8734d711a5624/ui/events/event.cc
[modify] https://crrev.com/8b679cacf3465630201734047fd8734d711a5624/ui/events/event_unittest.cc

Comment 8 by e...@chromium.org, Aug 22 2017

After #7, this works a bit better. Zooming in/out and scrolling almost work reliably. However, there's still some big problems:

- The mus server side cursor is still displayed in addition to the window manager side cursor window. Given some of the problems below though, this should be the last things fixed.

- The responsiveness of the window manager side cursor window isn't great and there feels like a lag between moving the mouse and seeing it move. Unsure how much of this is caused by seeing the mus server cursor move immediately though. This will need measurement to really track the impact though.

- The window manager side cursor window stops being updated sometimes. The most obvious case is when over the shelf on the bottom: the mus side cursor keeps being updated, but the window manager side cursor does stays still. Whatever process updates the window manager side cursor doesn't appear to be happening.

- When the cursor goes over a chrome window, the window manager side cursor starts varying wildly from the mus server side cursor. It as if the root location of the mus side cursor is being used as the in-chrome-window coordinates as a base for the ash window manager side cursor. I assume there's another location()/root_location() problem here.

- When zoomed in, and trying to scroll the magnification window in a corner, there's a good chance that some sort of feedback loop will grab the cursor and make it dance around the corner scrolling point. The only way to get out of this state is to alt tab and kill the process (without using the mouse, since it looks like it's directly setting the mouse location).

Comment 9 by e...@chromium.org, Aug 22 2017

Reverting #7 fixes the chrome cursor location issue and the cursor not updating its position in the taskbar. 


Comment 10 by e...@chromium.org, Aug 23 2017

After sky's patch to WindowEventDispatcher, the bad position in chrome and the not updating in the taskbar are fixed.

Comment 11 by e...@chromium.org, Aug 23 2017

Most of the problems now seem to stem from the window server not dealing with root transforms correctly.

Experiment: Add logs for window targeting mus server side. Have zoom on, but at 1:1. Notice that all events are getting targeted correctly. Place post-it notes on your monitor at the corners of the chrome window on the ash desktop. Now zoom in quite a bit, move the mouse. Note that the server side window target reported follows the location of the physical post-it notes, instead of what the window is currently displaying.

Rendering appears to properly using the root window transform. However, ui server side event targeting does not; the wrong target is being selected in the window server instead of in the ash window manager (which is where I originally thought the bug was). This is really weird, since //services/ui/ws/window_finder.cc appears to compensate for the ServerWindow's gfx::Transform!

Comment 12 by e...@chromium.org, Aug 25 2017

OK, so after sky@'s patches, and some minor changes, I'm able to get all
positioning and scrolling working in the magnification controller under --mus.

However, the minor changes are wrong.

What's going on is that the WindowEventDispatcher is synthesizing incorrect
mouse move events during WindowEventDispatcher::OnCursorMovedToRootLocation().
If you just comment out the line that posts a synthesized mouse move, mouse
movement and scrolling work, albeit at the expense of hover states working, so
this isn't a real solution.

One theory I'm investigating is that this is an issue with the shared memory
cursor position. Namely, every correct movement happens during an
MusMouseLocationUpdater::OnEventProcessingStarted() block. Every wrong jump
happens after a grab of the GetCursorScreenPoint() path in
last_mouse_location().

Let's call the point on the physical display physical space. Let's call the
point in the conceptual screen logical space.[1] When unzoomed in, physical
space equals logical space, and there is no problem. While the
MagnificationController is zoomed in, last_mouse_location() returns its
positions in logical space during event handling, as set by
MusMouseLocationUpdater. (ie: Even if viewport is zoomed in to the bottom half
of the logical display, but the cursor is near the top of the physical display,
the y coordinate will be a large number.) But outside of event handling, the
location returned from last_mouse_location() is the physical location, which
gets passed to it by CursorLocationManager::OnMouseCursorLocationChanged().

The minor (wrong) changes work by preventing the situation where we read from
last_mouse_location() outside of event processing, but this breaks some hover
states since the synthesized events really are needed. The right fix is so
last_mouse_location() always returns logical space, though I don't know exactly
where the transforms should go.

[1] Why invent terminology here? Because I'm not sure we're consistently using
screen and root location correctly consistently!
Cc: sadrul@chromium.org
I think the magnifier thingy tries to move the cursor (i.e. call WindowTreeHost::MoveCursor...()), right? Do we do that correctly with mus (i.e. are we using the right location when we send the request to mus? Are we synthesizing more move events because of the cursor move? Are all of those happening at the right place at the right time?)? If we stop moving the cursor, it would break the behaviour of maginifier, but does that address the hover issues? If it does, then the cursor-moving code is probably where the problem is.

Comment 14 by e...@chromium.org, Sep 8 2017

Cc: e...@chromium.org
Owner: sky@chromium.org
sky@, this was the patch I had before. https://chromium-review.googlesource.com/c/chromium/src/+/627482
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 12 2017

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

commit 4063fb956d6fb807e41d799691f2631b185511d3
Author: Scott Violet <sky@chromium.org>
Date: Tue Sep 12 17:40:26 2017

chromeos: make cursor location account for root transform

This is needed to properly convert pixels to DIPs, otherwise things
like the the magnification controller don't work.

BUG= 706200 
TEST=covered by test

Change-Id: Ie5c841c75080f239b0b3e01469ca0af1435e0756
Reviewed-on: https://chromium-review.googlesource.com/662057
Reviewed-by: Elliot Glaysher <erg@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501327}
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/cursor_location_manager.h
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/cursor_location_manager_unittest.cc
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/test_utils.cc
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/test_utils.h
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/4063fb956d6fb807e41d799691f2631b185511d3/services/ui/ws/window_manager_state_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Sep 12 2017

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

commit 42ea92d0031db4479473dd58fb1f665b911bff43
Author: Elliot Glaysher <erg@chromium.org>
Date: Tue Sep 12 18:41:33 2017

Hide the mus cursor when using window manager side cursor windows.

Bug:  706200 
Change-Id: Iceaa1b1bac05b348274ddd78040ded6e4a4de42e
Reviewed-on: https://chromium-review.googlesource.com/641377
Commit-Queue: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501342}
[modify] https://crrev.com/42ea92d0031db4479473dd58fb1f665b911bff43/ash/wm/native_cursor_manager_ash_mus.cc

Comment 17 by e...@chromium.org, Sep 12 2017

Status: Fixed (was: Assigned)

Comment 18 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 19 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment