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

Issue 847500 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Keys get "stuck" in Crostini

Project Member Reported by npmeyer@google.com, May 29 2018

Issue description

Chrome version: 68.0.3437.0 (Official Build) dev (64-bit)
OS: Chrome

Repro steps:
1. Boot Pixelbook
2. Open crosh
3. Run `vmc start termina`
4. Run `run_container.sh --container_name=penguin --user npmeyer --shell`
5. Run `xev`
6. Press and hold "Alt+Up"
7. While still holding "Alt+Up", press and hold "Tab"
8. Release "Alt+Up"
9. Release "Tab"
10. Alt+Tab back to xev window.

Expected: xev window is back in foreground, no repeated key events are generated.

Actual: xev shows many repeated KeyPress and KeyRelease events for "Prior" (Alt+Up) as shown below. If the issue does not immediately reproduce, repeat steps 6-10 a few times until it reproduces.

KeyPress event, serial 35, synthetic NO, window 0x400001,
    root 0x2d8, subw 0x0, time 761573, (-617,-382), root:(705,470),
    state 0x0, keycode 112 (keysym 0xff55, Prior), same_screen YES,
    XLookupString gives 0 bytes: 
    XmbLookupString gives 0 bytes: 
    XFilterEvent returns: False

KeyRelease event, serial 35, synthetic NO, window 0x400001,
    root 0x2d8, subw 0x0, time 761599, (-617,-382), root:(705,470),
    state 0x0, keycode 112 (keysym 0xff55, Prior), same_screen YES,
    XLookupString gives 0 bytes: 
    XFilterEvent returns: False

KeyPress event, serial 35, synthetic NO, window 0x400001,
    root 0x2d8, subw 0x0, time 761599, (-617,-382), root:(705,470),
    state 0x0, keycode 112 (keysym 0xff55, Prior), same_screen YES,
    XLookupString gives 0 bytes: 
    XmbLookupString gives 0 bytes: 
    XFilterEvent returns: False

KeyRelease event, serial 35, synthetic NO, window 0x400001,
    root 0x2d8, subw 0x0, time 761625, (-617,-382), root:(705,470),
    state 0x0, keycode 112 (keysym 0xff55, Prior), same_screen YES,
    XLookupString gives 0 bytes: 
    XFilterEvent returns: False

 

Comment 1 by npmeyer@google.com, May 29 2018

Cc: reve...@chromium.org
Components: IO>Keyboard
Project Member

Comment 3 by bugdroid1@chromium.org, May 31 2018

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

commit 1addcee1a18a2793020618e3fc2ca532135820a5
Author: David Reveman <reveman@chromium.org>
Date: Thu May 31 00:54:53 2018

exo: Ignore synthetic key repeat events.

Exo clients should not see these events and instead handle
key repeat on the client side.

Bug:  847500 
Test: exo_unittests
Change-Id: I4d4c755dbe0a2fffe24f3ce85fd70a3608cfed7c
Reviewed-on: https://chromium-review.googlesource.com/1079768
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563095}
[modify] https://crrev.com/1addcee1a18a2793020618e3fc2ca532135820a5/components/exo/keyboard.cc
[modify] https://crrev.com/1addcee1a18a2793020618e3fc2ca532135820a5/components/exo/seat.cc

Labels: -Pri-2 Hotlist-Crostini-Platform Pri-1
Owner: reve...@chromium.org
Status: Assigned (was: Untriaged)

Comment 5 by vapier@chromium.org, Jun 21 2018

Components: OS>Systems>Containers
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 25 2018

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

commit 94a6f1a75f7fd8247443ba66fb6958e786626181
Author: David Reveman <reveman@chromium.org>
Date: Mon Jun 25 17:04:55 2018

exo: Fix key event rewrite support.

Use a map to track pressed keys. The key for the map is the physical
code that was pressed and the value is a potentially rewritten code.

This allows us to generate appropriate release events when physical
keys are released, while still allowing the event rewriting logic
to work as expected.

Note: Exo clients will always see a release for the same code as
the code that was generated when the physical key was pressed. This
is necessary in order to not break client side keyboard state
tracking and might not match the events that are generated internally.
That is because event rewrite logic is usually not rewriting both the
press and the release events, often just the press event.

Bug:  847500 
Test: exo_unittests --gtest_filter=KeyboardTest.OnKeyboardKey
Tbr: hidehiko@chromium.org
Change-Id: I23240304bd377e3855971edd079ade1b537a74f9
Reviewed-on: https://chromium-review.googlesource.com/1112967
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570074}
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/ash/system/message_center/arc/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/keyboard.cc
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/keyboard.h
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/keyboard_delegate.h
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/keyboard_unittest.cc
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/seat.cc
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/seat.h
[modify] https://crrev.com/94a6f1a75f7fd8247443ba66fb6958e786626181/components/exo/wayland/server.cc

Cc: tbuck...@chromium.org
Status: Fixed (was: Assigned)
I'm going to revert the CL in comment 6 tomorrow because it breaks all keyboard operations on ARC++ apps with ChromeVox and navigation keys on virtual keyboard.
Please see  crbug.com/859071  and  crbug.com/859797 .
yhanada@, are you still going to revert the CL in #6?

(It's causing  crbug.com/862140 )
I think we have two options for how to fix this:

1. Use a different wayland protocol that arc++ understands for synthetic events.
2. Use the standard protocol but force all synthetic events to always generate both a press and a release event so they don't break client side keyboard state tracking.
Cc: yhanada@chromium.org
Hmm, option 2 sounds impossible because if we have a rewriting rule like (Search+1 -> F1) and keys are pressed like (press Search -> press 1 -> release Search -> release 1), it seems very difficult to know press event was rewritten when 1 is released.

by the way, why do we need to keep consistency between key press events and key release events? As far as I know, there are some cases in X Window apps that no key release event is sent to an app which receives the key press event.
For example, If focus is switched by Alt+Tab, previously focused window sees only press events and new focused window sees only release event.
Other cases is connecting 2 keyboards and pressing some key at the same time. This will generate event sequence like (press X -> press X -> release X -> release X).
There's no way to track the keyboard state in a wayland application unless you have input focus. Which is good, as being able to do so is terrible from a security perspective. As a result, wayland protocol sends the currently pressed keys to the client as part of the "enter" event for input focus. We need to support that properly. That's what the physical code logic is about.

2 keyboards can be assigned to 2 different seats in order to allow separate tracking. If you map 2 keyboards to the same seat then the best we can do is ignore press events when a key is already pressed and ignore release events if a key is not pressed. The protocol is not designed to support multiple keyboards per seat well. Multiple seats is the way to support that in an ideal way.

Search+1 rewrite is handled by exo in a reasonable way I think. We store the rewritten code for the physical key when pressed, and when the physical key is released, we generate the same rewritten code independent of if the release event is rewritten the same way or not.
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 13

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

commit 421412e7cce9db9512c8e4b816323aa0eed0def5
Author: Yuichiro Hanada <yhanada@chromium.org>
Date: Fri Jul 13 09:07:50 2018

Send synthetic key events to ARC++ apps.

crrev.com/c/1112967 adds a check that sending key events is native or
not and synthetic key events were not sent to exo clients after the CL.
The check was introduced to make sure key press event and key release
event always have the same key code. It's needed for Crostini apps
tracking pressed key sets by observing key press/release events.
However, after the CL, all synthetic events (for example, the back
button on the shelf or keyboard navigation when ChromeVox is enabled)
were not sent to ARC++ apps anymore.
This CL is a quick hack for fixing this problem on ARC++ apps. We have
to find a cleaner way to fix all problems later.

Bug:  859071 ,  859797 ,  847500 ,  859797 ,  862140 
Test: Confirmed the back button on the shelf works on ARC++ apps.
Change-Id: Ia1da441cca8bc43687327b69cccf16fb8692046b
Reviewed-on: https://chromium-review.googlesource.com/1133102
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574866}
[modify] https://crrev.com/421412e7cce9db9512c8e4b816323aa0eed0def5/components/exo/keyboard.cc
[modify] https://crrev.com/421412e7cce9db9512c8e4b816323aa0eed0def5/components/exo/keyboard.h
[modify] https://crrev.com/421412e7cce9db9512c8e4b816323aa0eed0def5/components/exo/notification_surface.cc

Sign in to add a comment