Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1165223003:
Fix Ozone keyboard layout to handle German Neo 2. (Closed)
|
Created:
5 years, 6 months ago by kpschoedel Modified:
5 years, 6 months ago CC:
chromium-reviews, kalyank, jdduke+watch_chromium.org, ozone-reviews_chromium.org, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@x489769-rewriter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Ozone keyboard layout to handle German Neo 2.
1. Adds a DOM UI Events |key| for XKB's 'Shift Level 5'.
2. Sets EF_MOD3_DOWN while CapsLock is down regardless of its mapping,
to match X11 handling of ChromeOS-patched XKB configuration.
BUG=495277
Committed: https://crrev.com/70fdc72777d2385464ada40c3308f1244a9db7e3
Cr-Commit-Position: refs/heads/master@{#333948}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Local prefixed DomCode #
Total comments: 9
Patch Set 3 : review comments #
Total comments: 2
Messages
Total messages: 20 (5 generated)
kpschoedel@chromium.org changed reviewers: + spang@chromium.org, wez@chromium.org
The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165223003/1
https://codereview.chromium.org/1165223003/diff/1/ui/events/ozone/evdev/keybo... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/1/ui/events/ozone/evdev/keybo... ui/events/ozone/evdev/keyboard_evdev.cc:225: if (static_cast<int>(dom_code) == 0x070039) Oops — forgot to do something about this before uploading.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1165223003/diff/1/ui/events/ozone/evdev/keybo... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/1/ui/events/ozone/evdev/keybo... ui/events/ozone/evdev/keyboard_evdev.cc:225: if (static_cast<int>(dom_code) == 0x070039) On 2015/06/08 20:11:32, kpschoedel wrote: > Oops — forgot to do something about this before uploading. Done.
nit: Re CL description, DOM Level 3 is now DOM UI Events...
https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key_data.inc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key_data.inc:70: // Key Enum The DOM UI Events spec guidelines (see http://www.w3.org/TR/DOM-Level-3-Events/#keys-guidelines) don't allow for "non-standard" |key| values - do we need to file a spec bug to support this? https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:23: // same underlying data with an additional prefix. nit: Is there some easy way to fix that issue in dom_code.h instead? https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:231: // certain layouts work. crbug.com/495277 nit: Consider explicitly saying "even if it is re-mapped to e.g. Search or Control". OTOH, the X11 XKB behaviour sounds broken to me... ;)
https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key_data.inc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key_data.inc:70: // Key Enum On 2015/06/08 21:23:23, Wez wrote: > The DOM UI Events spec guidelines (see > http://www.w3.org/TR/DOM-Level-3-Events/#keys-guidelines) don't allow for > "non-standard" |key| values - do we need to file a spec bug to support this? Yes, that section seems clear. (The |key| document http://www.w3.org/TR/DOM-Level-3-Events-key/ alone seems ambiguous, though, with phrases “… implementations must support, at a minimum …” and “… key values not included here, which have become common since the publication of this specification.”) https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:23: // same underlying data with an additional prefix. On 2015/06/08 21:23:23, Wez wrote: > nit: Is there some easy way to fix that issue in dom_code.h instead? I believe we could rename the DomCode constants for letters from DomCode::KEY_X to just DomCode::X, since the conflicts are with preprocessor macros of the form KEY_X. (That would touch many files, mostly tests, so I'd rather make it a separate rename-only CL to simplify reviewing.) For the most part the DomCode and DomKey enum constants are direct conversions of the W3C strings, but there are already some exceptions arising from other name conflicts (DEL rather than DELETE and FN_LOCK rather than F_LOCK). https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:231: // certain layouts work. crbug.com/495277 On 2015/06/08 21:23:23, Wez wrote: > nit: Consider explicitly saying "even if it is re-mapped to e.g. Search or > Control". > > OTOH, the X11 XKB behaviour sounds broken to me... ;) Done. The pure-X11 behaviour is fine, but the ChromeOS hybrid model, with patched XKB configuration and modifier handling mixed between X11 and Chrome, is fragile, and I'll be glad when all devices are Freonized and some of the hacks can be undone.
https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:234: UpdateModifier(EF_MOD3_DOWN, down); Can you now remove the other hack for MOD3 in UpdateModifier()?
https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:234: UpdateModifier(EF_MOD3_DOWN, down); On 2015/06/08 22:43:09, spang wrote: > Can you now remove the other hack for MOD3 in UpdateModifier()? No, it's still required (in fact, the Neo layout is another reason it *is* required). Cleaning this up depends on removing or cleaning up the ChromeOS XKB patches, which is not easy until post-X11 ChromeOS ~~~~~~~~=[,,_,,]:3
On 2015/06/09 13:28:50, kpschoedel wrote: > https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... > File ui/events/ozone/evdev/keyboard_evdev.cc (right): > > https://codereview.chromium.org/1165223003/diff/40001/ui/events/ozone/evdev/k... > ui/events/ozone/evdev/keyboard_evdev.cc:234: UpdateModifier(EF_MOD3_DOWN, down); > On 2015/06/08 22:43:09, spang wrote: > > Can you now remove the other hack for MOD3 in UpdateModifier()? > > No, it's still required (in fact, the Neo layout is another reason it *is* > required). Cleaning this up depends on removing or cleaning up the ChromeOS XKB > patches, which is not easy until post-X11 ChromeOS ~~~~~~~~=[,,_,,]:3 Haha wow. lgtm
wez@chromium.org changed reviewers: + garykac@chromium.org
LGTM Adding garykac as reviewer to comment on the spec question, though. https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key_data.inc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key_data.inc:70: // Key Enum On 2015/06/08 22:30:02, kpschoedel wrote: > On 2015/06/08 21:23:23, Wez wrote: > > The DOM UI Events spec guidelines (see > > http://www.w3.org/TR/DOM-Level-3-Events/#keys-guidelines) don't allow for > > "non-standard" |key| values - do we need to file a spec bug to support this? > > Yes, that section seems clear. (The |key| document > http://www.w3.org/TR/DOM-Level-3-Events-key/ alone seems ambiguous, though, with > phrases “… implementations must support, at a minimum …” and “… key values not > included here, which have become common since the publication of this > specification.”) That does seem ambiguous - perhaps garykac@ can clarify & update the wording in whichever of the two docs is correct wrt what the working group agreed. https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... File ui/events/ozone/evdev/keyboard_evdev.cc (right): https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:23: // same underlying data with an additional prefix. On 2015/06/08 22:30:02, kpschoedel wrote: > On 2015/06/08 21:23:23, Wez wrote: > > nit: Is there some easy way to fix that issue in dom_code.h instead? > > I believe we could rename the DomCode constants for letters from DomCode::KEY_X > to just DomCode::X, since the conflicts are with preprocessor macros of the form > KEY_X. (That would touch many files, mostly tests, so I'd rather make it a > separate rename-only CL to simplify reviewing.) For the most part the DomCode > and DomKey enum constants are direct conversions of the W3C strings, but there > are already some exceptions arising from other name conflicts (DEL rather than > DELETE and FN_LOCK rather than F_LOCK). Acknowledged. https://codereview.chromium.org/1165223003/diff/20001/ui/events/ozone/evdev/k... ui/events/ozone/evdev/keyboard_evdev.cc:231: // certain layouts work. crbug.com/495277 On 2015/06/08 22:30:02, kpschoedel wrote: > On 2015/06/08 21:23:23, Wez wrote: > > nit: Consider explicitly saying "even if it is re-mapped to e.g. Search or > > Control". > > > > OTOH, the X11 XKB behaviour sounds broken to me... ;) > > Done. > > The pure-X11 behaviour is fine, but the ChromeOS hybrid model, with patched XKB > configuration and modifier handling mixed between X11 and Chrome, is fragile, > and I'll be glad when all devices are Freonized and some of the hacks can be > undone. Acknowledged.
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165223003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/70fdc72777d2385464ada40c3308f1244a9db7e3 Cr-Commit-Position: refs/heads/master@{#333948} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
