Closed Bug 981262 Opened 6 years ago Closed 6 years ago

Hatchak keyboard layout doesn't work in Firefox

Categories

(Core :: DOM: UI Events & Focus Handling, defect, minor)

All
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: alexhenrie24, Assigned: alexhenrie24)

Details

Attachments

(1 file)

This is an edge case bug in KeymapWrapper::GetDOMKeyCodeFromKeyPairs that is exposed by the Hatchak keyboard layout: https://github.com/willghatch/hatchak

The function KeymapWrapper::GetDOMKeyCodeFromKeyPairs is only used in 3 places, all in KeymapWrapper::ComputeDOMKeyCode:

1. If the key is a modifier key (for example, Ctrl), GetDOMKeyCodeFromKeyPairs maps the modifier key to a DOM key code

2. If the key is an unprintable key (for example, F3), GetDOMKeyCodeFromKeyPairs maps the unprintable key MINUS MODIFIERS to a DOM key code

3. If the key is an unprintable key and step 2 failed, GetDOMKeyCodeFromKeyPairs maps the unprintable key with modifiers to a DOM key code

In the Hatchak keyboard layout, Tab becomes AltGr and Tab+Space (now AltGr+Space) becomes Tab. When ComputeDOMKeyCode reaches step 2, it strips the AltGr from the key event and GetDOMKeyCodeFromKeyPairs returns NS_VK_SPACE (causing a space to be typed). This makes it impossible to use Tab on the Hatchak keyboard layout.

I propose changing GetDOMKeyCodeFromKeyPairs to map GDK_space to 0 instead of NS_VK_SPACE. This will cause ComputeDOMKeyCode to proceed to step 3 which will properly map AltGr+Space to NS_VK_TAB.

Note that in any keyboard layout except Hatchak, when the space key is pressed steps 1-3 are skipped and GetDOMKeyCodeFromKeyPairs is never called. So there's no reason for GDK_space to be in GetDOMKeyCodeFromKeyPairs, and having it there breaks this edge case.

I hope that explanation makes sense. If it doesn't, let me know.
Attached patch Proposed patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=5f02fb0be80c
Assignee: nobody → alexhenrie24
Status: NEW → ASSIGNED
Attachment #8388040 - Flags: review?(roc)
Comment on attachment 8388040 [details] [diff] [review]
Proposed patch

Review of attachment 8388040 [details] [diff] [review]:
-----------------------------------------------------------------

Go Alex! But Masayuki knows this better than I do.
Attachment #8388040 - Flags: review?(roc) → review?(masayuki)
Comment on attachment 8388040 [details] [diff] [review]
Proposed patch

It seems that this doesn't compute NS_VK_SPACE if spacebar doesn't cause text input? E.g., Shift + Spacebar, Alt + Spacebar and/or Ctrl + Spacebar?

And KeyboardEvent.key works as you expected?
I did some more tests:

Both with and without the patch, and on both the QWERTY and the Hatchak keyboards, ComputeDOMKeyCode returns NS_VK_SPACE on Space, Shift+Space, Ctrl+Space, and Alt+Space.

Both with and without the patch, and on both the QWERTY and the Hatchak keyboards, Space and Shift+Space always produce character input; Ctrl+Space and Alt+Space never do.

Both with and without the patch, and on both the QWERTY and the Hatchak keyboards, KeyboardEvent.key returns " " on Space, Shift+Space, Ctrl+Space, and Alt+Space.

Both with and without the patch, on the Hatchak keyboard, KeyboardEvent.key returns "Tab" on Alt+Space (which surprised me), but Tab only moves to the next element with the patch applied.

So the patch appears to be working perfectly.
Correction: Both with and without the patch, on the Hatchak keyboard, KeyboardEvent.key returns "Tab" on Tab+Space (which surprised me), but Tab only moves to the next element with the patch applied.
Comment on attachment 8388040 [details] [diff] [review]
Proposed patch

Okay, thank you for the test!
Attachment #8388040 - Flags: review?(masayuki) → review+
Requesting checkin of attachment 8388040 [details] [diff] [review]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f118cc958ddd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Thank you!
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.