Did some preliminary research on this.
i.
LockCursor() is part of the pointer events' LockMouse()/UnlockMouse() calls (see implementation in RenderWidgetHostViewAura). It prevents the cursor manager from changing the various cursor properties until unlocked, when it applies all the latest state for everything. Something should work if we just call the current window’s cursor manager and issue LockCursor()/UnlockCursor() calls. But I’m not sure what it should do given how we changed the semantics of cursors. In mus, every window has its own cursor, instead of there being a single toplevel cursor state.
Currently in ash, if you LockCursor(), this is a desktop wide effect because there’s one CursorManager on the one toplevel aura root window, meaning ash can. But the code on desktop (at least desktop x11), if you LockCursor(), this only applies to the current set of chrome windows. In the current NativeWidgetMus implementation, it only applies to the current process.
But most of the places in ash where we call LockCursor(), we want a global effect! DefaultWindowResizer wants to lock the cursor to a resize one for the duration of the resize. ScreenshotController wants to set the cursor to the window selection cursor globally (!) and lock it for the duration of selecting a window to screenshot, and then hide the cursor while the screenshot is taken. LockStateController wants to globally hide the cursor from the point where a user clicks on the shutdown button to the computer actually shutting down.
(This isn’t a problem for the desktop usage of LockCursor(), such as RWHVA::LockMouse(). The cursor gets hidden on the current window, and if there’s a mouse movement over the window edge, it warps the mouse position back to the center of the window. I don’t think we even need to implement anything for this case.)
ii.
IsMouseEventsEnabled() is another place where the current ash code looks at the state of the global Root Window’s CursorManager for information. And it’s another place where all the non-ash usage is safe to be per-process. (IsMouseEventsEnabled() is also mostly already implemented in native_widget_mus.cc; )
(The entire set of EnableMouseEvents()/DisableMouseEvents()/IsMouseEventsEnabled() might also be misnamed; I don’t see anything that really actually affects the generation of mouse events, only things at the edges which interpret events. i.e. ImmersiveFullscreenController:: UpdateLocatedEventRevealedLock(), ShelfLayoutManager::CalculateAutoHideState(), View::IsMouseHovered(). Several of these checks are really a sort of “mouse cursor visibility plus” checks for touch events.)
Summary: Implement aura::client::CursorClient for mushrome/mustash (was: ash wm needs equivalent of aura::client::CursorClient)
I'm changing the title to better reflect what I think should happen here.
WmShell exposes a couple of functions that come from CursorClient, e.g. LockCursor, UnlockCursor and IsMouseEventsEnabled. These functions should be removed and code should go back to using CursorClient. In mushrome/mash custom mus specific CursorClients should be installed. The mus specific cursor client will end up talking over mojom to mus.
Summary: Reevaluate usage of aura::client::CursorClient in mushrome/mustash (was: Implement aura::client::CursorClient for mushrome/mustash)
I think the problem is one step higher.
The CursorClient interface assumes that it's setting the properties of a single
global cursor for the whole screen. This isn't how mus works, with a separate
client/non-client cursor set on a per mus window basis, with cursor switching
happening automatically on the server.
Calls to the cursor manager in ash fall into a few categories:
- The calls in ScreenshotController which try to set the cursor in a global
way. This is a one off and would require a new SetGlobalOverride() cursor.
- Calls which are trying to hide/lock the cursor globally. (This can be handled
by just exposing Show/Hide/Lock/Unlock methods to the window manager, per the
bug description.)
- Calls which are superfluous. (Everything about window resizing; this is
handled by mus.)
I wrote a patch which exposes {Lock,Unlock,Show,Hide}Cursor to the client
(https://codereview.chromium.org/2857963003/), and with the addition of a new
interface to set a global override cursor, this should be enough to
reimplemenet (or ignore) the above usages.
The big problem is that's not the majority of what CursorManager/
NativeCursorManager do. CurosrManager is another way for ash to inject
behaviour into Chrome.
CurosrClient is set globally on a root window. In pre-mus-mash land, it was set
and then took all cursor changing calls from chrome. It did this to do a bunch
of additional
- To control the size of the cursor. CursorClient::SetCursorSet() is called by
chrome accessibility code to make the cursor larger.
- To disable the native cursor and to use an image in an aura::Window as the
cursor instead. (This is also some sort of accessibility feature). I have no
idea how you go and make this performant in a mus world. Given that cursor
data is no longer data a single piece of global state, but
- Global boradcast of these state changes to all aura window hosts.
These things which are currently the responsibility of
CursorManager/AshNativeCursorManager need to be rethought.
(I haven't looked at what it would take to implement IsMouseEventsEnabled() in
detail; it seems to be a weird hanger-on to this interface.)
The last part of this bug that still needs to be addressed is IsMouseEventsEnabled(). In #4, I suggested that it was mostly implemented and later, suggested that it had nothing to do with whether mouse events were dispatched.
I was wrong. This all needs a major rethink. The state is used in WindowEventDispatcher::PreDispatchMouseEvent() to filter non-synthesized mouse events out. This code is in aura, and the window server does not appear to do anything like this; fixing this probably involves window manager calls to the current EnableMouseEvents()/DisableMouseEvents(), along with changing event dispatch in the window server itself.
(This might not be a problem for mushrome, since it's keeping everything in one process? I'll need further research here.)
Comment 1 by sky@chromium.org
, Aug 24 2016