yhanada@, is this yours?

will look into this issue today

I found out that crrev.com/c/1112967 broke this behavior.
The CL assumes all calling of OnKeyEvent() is like following:
WillProcessEvent()
OnKeyEvent()
DidProcessEvent()

but, that is not true for synthesized events like arrow keys on Japanese input or arrow keys on virtual keyboard.
I can't come up with easy solution for that now.

Navigation keys on VK with Crostini also should be broken by the same reason.

Please avoid reverting crrev.com/c/1112967 as that's critical for having keyboard event handling being correct.

We can still support synthesized events. We just need a reliable way to ensure that each 'press' event will get a matching 'release' event. As without that, keyboard tracking on the client side will be broken.

WillProcessEvent() and DidProcessEvent() (that you are using for tracking key press/release) seems not be called for synthesized events because those events are not associated with any physical keyboard pressing. Therefore, we cannot use them simply to track pressed key set.
I haven't came up with a good way to track those states correctly.
Do you have any ideas or suggestions?

+cindyb as TPM for M-69

Hi Cindy,
crrev.com/c/1112967 is a critical fix for keyboard event handling on Crostini apps as mentioned by Davide, but it breaks the back button on the shelf( crbug.com/862140 ), navigation keys on virtual keyboard(this issue) and keyboard navigation with Chromevox on ARC++ apps( crbug.com/859797 ).

I'm trying to fix these issues on ARC++ apps without breaking Crostini, but still couldn't come up with a solution.

Which one should take priority? Can I revert the change to fix the issues on ARC++ at first?

Ideally we'd fix synthetic events properly for M69 but if that fails, you can always allow the old behavior for ARC++ only without breaking Crostini. ie. if the surface events are targeting is hosted in a remote_shell_surface or notification_surface then don't require the physical key code to be set.

I think #10 (something equivalent) is probably better short term fix for 69. We still have proper fix, otherwise crostini won't be able to receive synthesized key events.

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