Port ScopedDisableInternalMouseAndKeyboardX11 to Ozone

Port ScopedDisableInternalMouseAndKeyboardX11 to Ozone

BUG=401598
TEST=Manual, see bug

Committed: https://crrev.com/62e31ac2ddce4ae5fbbc2f1275e7b0da9819a408
Cr-Commit-Position: refs/heads/master@{#311505}

[pkotwicz, 2014-12-17 03:39:40]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2014-12-17 04:17:40]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2014-12-17 04:29:44]

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

[pkotwicz, 2014-12-17 04:29:46]

spang@ PTAL

We need to block events in ozone instead of in EventRewriter because when we
disable the internal keyboard, we must also disable modifiers from the internal
keyboard

There is weird behavior if Ctrl is pressed when touch view is entered. This
weird behavior exists with X11 as well. It is possible to exit the weird state
by plugging in an external keyboard and pressing+releasing Ctrl

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_device_info.cc:242: return (HasAbsXY() ||
HasMTAbsXY()) && !IsMappedToScreen();
This function may return true for multi-touch mice. I need to find a multi-touch
mouse to test though

[spang, 2014-12-19 18:24:15]

Any reason not to do the filtering by installing an EventRewriter (or using the
existing CrOS one) and using ui::Event::device_id_ + DeviceDataManager to see if
its from an internal source?

That way doesn't add filtering policies to the platform API.

On 2014/12/17 04:29:46, pkotwicz wrote:
> spang@ PTAL
> 
> We need to block events in ozone instead of in EventRewriter because when we
> disable the internal keyboard, we must also disable modifiers from the
internal
> keyboard
> 
> There is weird behavior if Ctrl is pressed when touch view is entered. This
> weird behavior exists with X11 as well. It is possible to exit the weird state
> by plugging in an external keyboard and pressing+releasing Ctrl
> 
>
https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
> File ui/events/ozone/evdev/event_device_info.cc (right):
> 
>
https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
> ui/events/ozone/evdev/event_device_info.cc:242: return (HasAbsXY() ||
> HasMTAbsXY()) && !IsMappedToScreen();
> This function may return true for multi-touch mice. I need to find a
multi-touch
> mouse to test though

A

[pkotwicz, 2014-12-19 19:58:27]

The problem is that we need to block modifier keys as well.
For instance, I can attach an external mouse and click and simultaneously
accidentally press the Ctrl on a disabled internal keyboard.
The expected behavior would be to dispatch a normal click. Using an
EventRewriter, we would dispatch a Ctrl click which would be bad.

[spang, 2014-12-20 06:21:25]

On 2014/12/19 19:58:27, pkotwicz wrote:
> The problem is that we need to block modifier keys as well.
> For instance, I can attach an external mouse and click and simultaneously
> accidentally press the Ctrl on a disabled internal keyboard.
> The expected behavior would be to dispatch a normal click. Using an
> EventRewriter, we would dispatch a Ctrl click which would be bad.

Oh dear.. How are we disabling the effects of modifiers under X11?

[pkotwicz, 2014-12-21 02:03:21]

crbug.com/374002 has discussion on how this works under X11.

Under X11, modifiers from the blocked internal keyboard are ignored for touches
but not for an external mouse.

How it works on X11:
ui::EventTypeFromNative() returns ET_UNKNOWN when the event comes from a blocked
device.
WindowTreeHostX11::DispatchXI2Event() ignores XI2 events of type ET_UNKNOWN
In particular, if an XI2 event came from a blocked device,
EventFlagsFromNative() is not called, hence XModifierStateWatcher's state is
unchanged.

[pkotwicz, 2014-12-21 02:04:09]

For the sake of clarity: "Under X11, modifiers from the blocked internal
keyboard are ignored for touches
but not for an external mouse." means that we have a bug on X11

[spang, 2014-12-22 18:46:28]

spang@chromium.org changed reviewers:
+ kpschoedel@chromium.org

[spang, 2014-12-22 18:46:29]

+kpschoedel

[spang, 2014-12-22 18:56:16]

Thanks for the explanation.. didn't realize we were tracking the ctrl/shift
modifiers on x11 ourselves.

This feature seems unfortunate.. it's too bad there are side keys on the same
input device as the keyboard.

I like the idea of doing more of this in CrOS because it's quite hardware
specific.. I might look into that more. I believe it might be possible to unmap
keys and then remap them, and this would be transparent to chrome.

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_converter_evdev_impl.cc (right):

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_converter_evdev_impl.cc:108: KeyboardCode key_code =
KeyboardEvdev::KeyboardCodeFromEvdevKey(input.code);
I think this the key filter should probably use DomCode? KeyboardCode has some
weirdness to it, we probably want to avoid adding uses of it.

[kpschoedel, 2014-12-22 19:17:56]

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_converter_evdev_impl.cc (right):

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_converter_evdev_impl.cc:108: KeyboardCode key_code =
KeyboardEvdev::KeyboardCodeFromEvdevKey(input.code);
On 2014/12/22 18:56:16, spang wrote:
> I think this the key filter should probably use DomCode? KeyboardCode has some
> weirdness to it, we probably want to avoid adding uses of it.

Yes, assuming the comment in 
scoped_disable_internal_mouse_and_keyboard_x11.cc correctly captures the
motivation:
  // Allow the accessible keys present on the side of some devices to continue
  // working.
KeyboardCode depends on layout for painful Windows-compatibility reasons, so (in
principle) the physical side keys could return any VKEY (and vice versa).
DomCode is the platform-independent physical key, so the caller should probably
use that; currently the implementation can map DomCode to/from evdev scan codes
with
  DomCode dom_code = KeycodeConverter::NativeKeycodeToDomCode(evdev_code +
kXkbKeycodeOffset);
  unsigned int evdev_code = KeycodeConverter::DomCodeToNativeKeycode(dom_code) -
kXkbKeycodeOffset;

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ke...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/806693009/diff/40001/ui/events/ozone/evdev/ke...
ui/events/ozone/evdev/keyboard_evdev.cc:17: int ModifierFromEvdevKey(unsigned
int code) {
The base version here is fatally out of date now.

[pkotwicz, 2014-12-23 01:50:19]

Patchset #2 (id:60001) has been deleted

[pkotwicz, 2014-12-23 01:53:09]

kpschoedel@ can you please take another look?

I have rebased the CL and have switched to using ui::DomCode.

[kpschoedel, 2014-12-25 18:14:17]

Non-owning LGTM for the keystroke parts, with one suggestion.

https://codereview.chromium.org/806693009/diff/80001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_converter_evdev_impl.cc (right):

https://codereview.chromium.org/806693009/diff/80001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_converter_evdev_impl.cc:118:
KeyboardEvdev::EvdevCodeToNativeCode(input.code));
NativeKeycodeToDomCode() is a slow operation; since allowed_keys_ is part of
evdev-specific code could you make it hold evdev codes and thus convert just
once?

[spang, 2015-01-02 22:49:57]

lgtm

https://codereview.chromium.org/806693009/diff/80001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_converter_evdev_impl.cc (right):

https://codereview.chromium.org/806693009/diff/80001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_converter_evdev_impl.cc:61: if (ignore_events_)
Add a TODO(spang): Re-implement this by releasing buttons & temporarily closing
the device.

That'll fix the buttons stuck down issue, as well as some relatively minor
issues with respecting the event boundaries (SYN_REPORT) when we stop or resume
processing.

[pkotwicz, 2015-01-13 22:25:48]

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

[pkotwicz, 2015-01-13 22:25:49]

oshima@ can you take a look at the changes in ash/

[oshima, 2015-01-13 23:16:37]

ash lgtm with nits and q.

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc
(right):

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc:24:
->HideCursor();
I assume you just want to hide as a initial state for touch view and that's why
it's not locking?

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h
(right):

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
nit: 2015

[pkotwicz, 2015-01-14 16:24:12]

Patchset #4 (id:120001) has been deleted

[pkotwicz, 2015-01-14 16:24:38]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-01-14 16:25:19]

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

[pkotwicz, 2015-01-14 16:36:45]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2015-01-14 16:46:04]

Patchset #5 (id:160001) has been deleted

[pkotwicz, 2015-01-14 16:46:14]

Patchset #4 (id:140001) has been deleted

[pkotwicz, 2015-01-14 16:52:21]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-01-14 16:52:46]

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

[commit-bot: I haz the power, 2015-01-14 17:52:20]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-01-14 17:53:19]

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

[pkotwicz, 2015-01-14 18:01:42]

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc
(right):

https://codereview.chromium.org/806693009/diff/100001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc:24:
->HideCursor();
Yes, moving an external mouse while in maximized mode will make the cursor
reappear.