Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1427743003:
Alt remapping doesn't work when Shift is pressed on ChromeOS (Closed)
DescriptionAlt remapping doesn't work when Shift is pressed on ChromeOS
When the Alt key is mapped to ESC or disabled from the settings, pressing
Shift+Alt will generate a KeySym value of |XKB_KEY_Meta_L/R|. This will
result in the EventRewriter skipping the rewriting of the Alt key completely.
On ChromeOS, the Meta key is not used, and hence we keep it as Alt.
BUG=541468
TEST=tested manually on a link device as well as ChromeOS Linux build.
Committed: https://crrev.com/8a9bfea2cfaf6a748389909207d8e837146c9519
Cr-Commit-Position: refs/heads/master@{#357385}
Patch Set 1 #
Messages
Total messages: 16 (3 generated)
afakhry@chromium.org changed reviewers: + kpschoedel@chromium.org
kpschoedel@ How about this way of fixing the bug? This keeps the behavior the same for both real device builds and CrOS Linux builds.
kpschoedel@chromium.org changed reviewers: + wez@chromium.org
On 2015/10/28 23:04:01, afakhry wrote: > kpschoedel@ How about this way of fixing the bug? This keeps the behavior the > same for both real device builds and CrOS Linux builds. LGTM; I think that's a reasonable way on ChromeOS to catch any XKB layout case that returns Meta. +Wez for OWNER review.
On 2015/10/29 at 03:03:49, kpschoedel wrote: > On 2015/10/28 23:04:01, afakhry wrote: > > kpschoedel@ How about this way of fixing the bug? This keeps the behavior the > > same for both real device builds and CrOS Linux builds. > > LGTM; I think that's a reasonable way on ChromeOS to catch any XKB layout case that returns Meta. > > +Wez for OWNER review. From the bug & CL description it wasn't clear to me why this only affects Alt when configure as Escape, or disabled? IIUC your change will mean that Shift+Alt generates DomKey::META under Linux, but DomKey::ALT under CrOS with Alt left behaving as Alt?
On 2015/10/29 17:32:10, Wez wrote: > On 2015/10/29 at 03:03:49, kpschoedel wrote: > > On 2015/10/28 23:04:01, afakhry wrote: > > > kpschoedel@ How about this way of fixing the bug? This keeps the behavior > the > > > same for both real device builds and CrOS Linux builds. > > > > LGTM; I think that's a reasonable way on ChromeOS to catch any XKB layout case > that returns Meta. > > > > +Wez for OWNER review. > > From the bug & CL description it wasn't clear to me why this only affects Alt > when configure as Escape, or disabled? > > IIUC your change will mean that Shift+Alt generates DomKey::META under Linux, > but DomKey::ALT under CrOS with Alt left behaving as Alt? Yes, it affects the *rewriting* of Alt when it is set to be mapped to something else from the keyboard settings page. ESC or disabled are the ones that have testable reproducible bugs like Shift+Alt[=>ESC] --> Should bring up the task manager but it doesn't, because the event rewriter doesn't see Alt, but rather Meta. Similarly Shift+Alt[=>Disabled] should *not* switch to the next IME, but it does also since the rewriter doesn't see Alt to rewrite it (or remove it).
On 2015/10/29 at 17:53:56, afakhry wrote: > On 2015/10/29 17:32:10, Wez wrote: > > On 2015/10/29 at 03:03:49, kpschoedel wrote: > > > On 2015/10/28 23:04:01, afakhry wrote: > > > > kpschoedel@ How about this way of fixing the bug? This keeps the behavior > > the > > > > same for both real device builds and CrOS Linux builds. > > > > > > LGTM; I think that's a reasonable way on ChromeOS to catch any XKB layout case > > that returns Meta. > > > > > > +Wez for OWNER review. > > > > From the bug & CL description it wasn't clear to me why this only affects Alt > > when configure as Escape, or disabled? > > > > IIUC your change will mean that Shift+Alt generates DomKey::META under Linux, > > but DomKey::ALT under CrOS with Alt left behaving as Alt? > > Yes, it affects the *rewriting* of Alt when it is set to be mapped to something else from the keyboard settings page. ESC or disabled are the ones that have testable reproducible bugs like Shift+Alt[=>ESC] --> Should bring up the task manager but it doesn't, because the event rewriter doesn't see Alt, but rather Meta. Similarly Shift+Alt[=>Disabled] should *not* switch to the next IME, but it does also since the rewriter doesn't see Alt to rewrite it (or remove it). Why are we still re-writing based on the DomKey rather than the DomCode? If we must rewrite based on DomKey, could the rewriter simply understand that Alt & Meta are the same key?
On 2015/10/29 18:53:32, Wez wrote: > Why are we still re-writing based on the DomKey rather than the DomCode? This preserves historical behaviour, in which AltGraph is untouched by the modifier settings, rather than moving with AltRight. (I don't personally think that was the right choice, but I expect people are used to it.)
On 2015/10/29 19:02:32, kpschoedel wrote: > On 2015/10/29 18:53:32, Wez wrote: > > Why are we still re-writing based on the DomKey rather than the DomCode? > > This preserves historical behaviour, in which AltGraph is untouched by the > modifier settings, rather than moving with AltRight. (I don't personally think > that was the right choice, but I expect people are used to it.) I can add a case to the event rewriter for DomKey::Meta here https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... for ChromeOS only, but this has the potential of having other similar bugs wherever XKeySymToDomKey() is used and that Shift+Alt == DomKey::Meta is not expected.
On 2015/10/29 at 20:25:35, afakhry wrote: > On 2015/10/29 19:02:32, kpschoedel wrote: > > On 2015/10/29 18:53:32, Wez wrote: > > > Why are we still re-writing based on the DomKey rather than the DomCode? > > > > This preserves historical behaviour, in which AltGraph is untouched by the > > modifier settings, rather than moving with AltRight. (I don't personally think > > that was the right choice, but I expect people are used to it.) > > I can add a case to the event rewriter for DomKey::Meta here https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... for ChromeOS only, but this has the potential of having other similar bugs wherever XKeySymToDomKey() is used and that Shift+Alt == DomKey::Meta is not expected. I'd argue that any such call-sites are simply buggy, and should be fixed! I'd prefer that we re-write based on the DomCode, not the DomKey, though I appreciate that that would mean a special-case to work-around AltGr. Kevin, I'll defer to you on this one, if you think that this is the cleanest solution?
On 2015/10/29 21:19:30, Wez wrote: > Kevin, I'll defer to you on this one, if you think that this is the cleanest > solution? I think there are no really clean solutions, only workarounds, because there's an impedance mismatch between Chrome[OS]'s Windows-inspired shortcuts and the X-derived layouts. This solution at least has the pragmatic advantage that it matches the established conversion to VKEYs in |KeyboardCodeFromXKeysym()|. I'd support separately changing EventRewriter to use DomCode, preferably without an AltGr special case, or, if there's a special case, with a distinct AltGr setting. I suspect the current scheme was not explicitly chosen, just implemented with only the plain US layout in mind. A decision on that would probably be slow, though. The other option I can think of is modifying the CrOS xkeyboard-config to not generate XK_Meta on Shift+Alt. (IMO that's a unwise mapping, since it depends on the keypress order, which doesn't match my mental model of modifiers.)
On 2015/10/30 at 15:56:54, kpschoedel wrote: > On 2015/10/29 21:19:30, Wez wrote: > > Kevin, I'll defer to you on this one, if you think that this is the cleanest > > solution? > > I think there are no really clean solutions, only workarounds, because there's an impedance mismatch between Chrome[OS]'s Windows-inspired shortcuts and the X-derived layouts. This solution at least has the pragmatic advantage that it matches the established conversion to VKEYs in |KeyboardCodeFromXKeysym()|. OK, SGTM. > I'd support separately changing EventRewriter to use DomCode, preferably without an AltGr special case, or, if there's a special case, with a distinct AltGr setting. I suspect the current scheme was not explicitly chosen, just implemented with only the plain US layout in mind. A decision on that would probably be slow, though. Agreed; worth discussing w/ dtapuska@, though. > The other option I can think of is modifying the CrOS xkeyboard-config to not generate XK_Meta on Shift+Alt. (IMO that's a unwise mapping, since it depends on the keypress order, which doesn't match my mental model of modifiers.) That's always the case with DOM UI Events' |key| values (e.g. "a" versus "A" in keyup depending on whether Shift is released before or after the A key), and sometimes the case with other Windows VKEY values. In this case, though, I could see an argument for aiming for compatibility with Windows layouts rather than Linux. LGTM
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427743003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427743003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/8a9bfea2cfaf6a748389909207d8e837146c9519 Cr-Commit-Position: refs/heads/master@{#357385} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
