Issue metadata
Sign in to add a comment
|
CapsLock is mis-reported as F16 in most keydown/keyup events. |
||||||||||||||||||||
Issue descriptionChrome Version: 65.0.3325.56 (Official Build) dev (32-bit) (though this was reported as a regression in M63 and closed as Fixed while still being broken). OS: ChromeOS What steps will reproduce the problem? (1) Attach a USB keyboard, or configure the Search key to perform as CapsLock. (2) Open https://w3c.github.io/uievents/tools/key-event-viewer.html (3) Press and release the CapsLock key twice. What is the expected result? Expect to see four events, keydown+keyup pairs with |key|="CapsLock". What happens instead? Four events are generated, but all but one of them are mis-reported with |key|="F16".
,
Feb 9 2018
The DomKey F16 is first mapped to CapsLock, then in [1] the CapsLock is changed back to F16 when extracting layout results. I took a deeper look and found that the |xkb_keysym| for the key is XKB_KEY_XF86Launch7 in [2] and then converted to F16. [1] https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chromeos.cc?q=event_rewriter_chromeos.cc&dr&l=233 [2] https://cs.chromium.org/chromium/src/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc?q=xkb_keyboard_la&dr=C&l=751 A simple fix: https://chromium-review.googlesource.com/c/chromium/src/+/912239 The test [3] already covers this case, but since in the test KeyEvent::ApplyLayout() uses StubKeyboardLayoutEngine, while in real device it uses XkbKeyboardLayoutEngine which maps Caps Lock to F16. So the test passes, but the bug still exist in real device. [3] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/events/event_rewriter_unittest.cc?q=event_rewriter_uni&dr&l=1040
,
Feb 9 2018
,
Feb 9 2018
Re #2: The bug seems to be that the key is being reported as XKB_KEY_XF86Launch7, surely? Did you test with the physical CapsLock key, though, or with Search key configured to behave as CapsLock?
,
Feb 9 2018
In both situation, after EventRewriterChromeos::RewriteModifierKeys(), the |state| will be Caps Lock key. Its dom key is later overwritten here [1] to F16. So XKB_KEY_XF86Launch7 is reported for both situations. [1] https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chromeos.cc?q=event_rewriter_chromeos.cc&sq=package:chromium&dr&l=525
,
Feb 9 2018
Re #5: I'm sorry, but I still don't see where this is going wrong: - For a physical CapsLock key (i.e. external USB keyboard) XKB should be generating XKB_KEY_Caps_Lock. There should not be any rewriting required, and it should trivially resolve back to CapsLock even if we re-resolve the DomKey. - For Search configured as CapsLock we should either be: a) Rewriting the event so that its DomCode is that of CapsLock rather than OSKey - so that to any later stages the event _is_ a CapsLock key event. b) Rewriting only the VKEY and DomKey, as the last stage before we propagate the event to other parts of the system. I'm not sure from your description whether (1) the XKB layout table is generating the wrong value, (2) we're rewriting the event at some point other than (a) or (b), (3) we're rewriting things correctly, but then messing it up by doing some other post-fix for some other issue, or some combination of all three. :P
,
Feb 9 2018
Sorry about not being clear. Here is the details: 1. No matter it is Search key mapped to Caps Lock or external keyboard's Caps Lock key, the key event is successfully rewritten into correctly event after RewriteModifierKeys() [1] (Now the dom key is CapsLock). But later we mess the event by doing post-fix in SetMeaningForLayout() [2] which changes the dom key to F16. If you dive into the function and log in XkbKeyboardLayoutEngine [3], you would find |xkb_keysym| is XKB_KEY_XF86Launch7. Actually this is intended as mentioned here: [4]. [1] https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chromeos.cc?q=event_rewriter_chromeos.cc&sq=package:chromium&dr&l=496 [2] https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chromeos.cc?q=event_rewriter_chromeos.cc&sq=package:chromium&dr&l=525 [3] https://cs.chromium.org/chromium/src/ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc?q=xkb_keyboard_la&dr=C&l=725 [4] https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chromeos.cc?type=cs&q=XF86XK_Launch7&l=720
,
Feb 10 2018
Re #7: Based on this description, it seems we're going wrong at three points: 0. We have something broken in the XKB configuration, causing CapsLock to be mis-reported. 1. When we re-write events, we change the VKEY & DomKey but not the DomCode (i.e. we override the KeySym but not the XKB keycode). This means that if we _do_ re-map the event based on layout, it'll revert to pre-rewrite meaning. 2. We "post-fix" the already-rewritten event values back to the bogus values XKB reported. This is especially silly since we're doing it in the EventRewriterChromeOS itself, so it should really know better! IIUC this lead to the CapsLock regression when we started using the |sticky_keys_controller| to implement CapsLock, which meant that |flags & EF_CAPS_LOCK| changes whenever we process the CapsLock key events. This issue would only affect "sticky" modifier and numeric pad keys, since they are the only ones we rewrite _before_ the sticky keys controller call. So it looks like we should be passing the event back through https://chromium.googlesource.com/chromium/src/+blame/a22592852f3a26b711698a95933025967aa1db5a/ui/chromeos/events/event_rewriter_chromeos.cc#495 after the SetMeaningForLayout call, to re-rewrite it in the context of the new |flags| value. WDYT?
,
Feb 12 2018
#8 point 0: Long long ago the ChromeOS xkeyboard-config packaged was patched to report F16 in place of Caps Lock, in order to stop X11 changing its Caps state (in the usual case that the key is used for something else, i.e. Search). Now that X11 is gone, I suspect all these F16 special cases can be removed. crbug.com/146204
,
Feb 12 2018
Re #8, I made a CL for the solution you mentioned: https://chromium-review.googlesource.com/c/chromium/src/+/912239, is that what you mean?
,
Feb 12 2018
+kpschoedel@ Do you know why we need (Or in what use cases) to reapply the layout here if the flags after being rewritten is different than the original ones?
,
Feb 12 2018
,
Feb 13 2018
Re #11: In general the |key| associated with an event depends on (1) the physical key that was pressed (2) the layout that is currently configured and (3) the modifiers that are currently active. (e.g. the "2" key generates "2" under US English when un-modified but "@" when Shift is applied) The sticky_key_handler appears to be allowed to modify the |flags| associated with an event - perhaps because it needs to actually add the "sticky" modifiers? - which means it is actually changing the meaning of the key. (Technically it could also change the VKEY associated with the event, not just the |key|, since there are a few keys for which the modifiers actually affect the generated VKEY (e.g. the numeric pad keys, IIRC))
,
Feb 13 2018
Ideally we should propagate the information about user key settings, sticky keys, etc. down to Ozone and use that for layout, generate the correct event in the first place, and remove most of the horrible event rewriter code. I'm sad my Chrome time ran out before I could do that. (Event rewriting was *necessary* under X11 where we didn't have enough control over the original event.)
,
Feb 13 2018
Re #14: Do we need any special handling for Chrome-for-ChromeOS-under-Linux, which will still use X11? Or do we just use the stock Linux/X11 keyboard layout handling for that case? If the latter then cleaning up the "rewriter" logic by folding most of it into the layout engine SGTM.
,
Feb 13 2018
#15 — I hadn't know that existed, but yes, assuming it's the same code that used to be mainline ChromeOS and has the same requirements for preference-mapping and so on.
,
Feb 16 2018
Thanks for the explanation, wez@ I have a new patch set in https://chromium-review.googlesource.com/c/chromium/src/+/912239 (I am shadow gardening this week, sorry for late reply.) |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by w...@chromium.org
, Feb 9 2018Status: Assigned (was: Untriaged)