Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeyEvent rework #1040

Open
raphlinus opened this issue Jun 15, 2020 · 10 comments
Open

KeyEvent rework #1040

raphlinus opened this issue Jun 15, 2020 · 10 comments
Labels
architecture changes the architecture, usually breaking bug does not behave the way it is supposed to shell concerns the shell abstraction

Comments

@raphlinus
Copy link
Contributor

This is a tracking issue for redoing KeyEvent. Currently there are a number of things wrong with the implementation, which we're seeing as Control-X etc not working except as menu shortcuts (#1030), issues with numpad arrow keys (#318 and a number of related issues and PR's). Issue #357 rolls up a number of the numpad related issues.

I propose to rework KeyEvent to fix these. The following goals are in scope:

  • Key events should be as consistent as possible across platforms. This especially extends to the text field in keys with modifiers, scan codes, etc.

  • Export a reliable way for text widgets to determine whether the key event represents a potential text insertion. This will involve rethinking is_printable (see TextBox uses is_printable to filter text input which is unrelieable. #119 and Remove KeyCode.is_printable #1016).

  • Detailed information should be available and accurate. This includes tracking both left and right shift, as well as indicating whether the key is a repeat from being held down.

  • The Windows implementation should be UTF-16 aware, though key text in the astral plane is probably unlikely (see Support astral plane characters on Windows #174).

  • Expected behavior should be documented, including guidance about how and when to interpret keys as text insertion, and recognition of command modifiers.

I propose that we converge as much as possible toward the web KeyboardEvent mechanism. Note that the KeyboardEvent.keyCode field has extensive documentation and is likely a useful reference, though it is officially deprecated.

Also note, the issue of key events intersects in a very profound way with IME (input method editor), which is on our medium to long term roadmap but is unlikely to see work soon. Unfortunately, IME is the correct way to get important features such as "dead key" for Latin accents. In general, it's the goal of this work to not interfere with future development of IME, though I will take a look at whether there might be stopgap measures to get some of that functionality if well supported by the platform.

@raphlinus raphlinus added architecture changes the architecture, usually breaking bug does not behave the way it is supposed to shell concerns the shell abstraction labels Jun 15, 2020
@ratmice
Copy link
Collaborator

ratmice commented Jun 15, 2020

Given the convergence with the web KeyboardEvent mechanism,
I think it is worth looking at the keyboard-types crate (I believe is the basis for the KeyboardEvent implementation in servo).

@raphlinus
Copy link
Contributor Author

Very interesting. Poking around it, I see some things that are fairly well developed, but others that are strangely not. For example, there is no left shift / right shift distinction in modifiers, although there is a location that can be used to detect the press of the shift key itself (it feels to me that edge-triggered tracking of these is something of an antipattern). I suspect that there were long arguments about this and there is a reason, but if it's suboptimal I'm not sure I want to adopt it wholesale.

By comparison, Android's KeyEvent has a getMetaState() function, which happily reports left and right variants when queried.

I had a feeling this would be deeper water than I expected :)

@chris-morgan
Copy link

chris-morgan commented Jun 16, 2020

Probably also in scope for consideration: other input devices need to be able to know or query keyboard modifier state, so that things like Shift+click or Ctrl+scroll can work.

@xStrom
Copy link
Member

xStrom commented Jun 16, 2020

Probably also in scope for consideration: other input devices need to be able to know or query keyboard modifier state, so that things like Shift+click or Ctrl+scroll can work.

Yeah a cross-platform way to do this would be nice. For the mouse events we are currently using the platform provided modifier state, but that is differently defined on every platform. If I remember correctly then Windows is the only platform that tracks the right side keys in the mouse API.

@raphlinus
Copy link
Contributor Author

raphlinus commented Jun 18, 2020

I've done a lot of investigation, mostly on Windows so far, and want to update the issue. I have a notes doc containing a bunch of my notes, but will try to synthesize any firm findings into comments on this issue, so it's mostly of interest to those following along.

I did a full Windows implementation of KeyboardEvent from keyboard_types as raphlinus/win-win#5, and learned some things in the process. My primary model in this work was Mozilla, as the source was much easier to read than Chromium, and it takes some care to get things right. There are a number of challenges on Windows, not least of which is the fact that you get a sequence of WM_KEYDOWN followed by 0 or more WM_CHAR messages for each keypress, and these should be combined into one event. (More than 1 WM_CHAR is rare but can happen for non-BMP code points and also cases like getting a doubled spacing accent on the second press of a dead key).

I did not find an authoritative guide to key press handling on Windows. A very outdated guide is Using Keyboard Input which has so many things wrong with it that it's hard to know where to start. The blog Robust key message handling in Windows was interesting, but I don't think its advice of "Don’t try and correlate WM_CHAR and WM_KEYDOWN" is workable for our needs; in order to generate a KeyboardEvent, we absolutely do need information from both messages, and even aside from that we would need a reliable way to determine whether to do the handling in WM_KEYDOWN or wait for the WM_CHAR to come along. This is what is_printable was for, but it was a hack and didn't really work.

The solution I came up with is to use PeekMessage to determine whether there is a WM_CHAR in the queue. In my experiments, this should be quite reliable, even when the user is typing faster than the runloop can keep up; the WM_CHAR messages are inserted by TranslateMessage at the time the WM_KEYDOWN message is dispatched, so the Peek should only see WM_CHAR messages that "belong" to the same physical keypress. Even so, I implemented logic to detect that the scan code represents the same key. Basically, the strategy is to defer emitting the event until the last message in the sequence, which is detected by the absence of a peeked WM_CHAR (or, out of caution, a WM_CHAR with a different scan code). This is a lot simpler than the logic in Mozilla, which tries to do the processing in the WM_KEYDOWN handler, and goes through a considerable extremes to try to fetch the subsequent WM_CHAR messages.

Regarding whether to use keyboard_types, I am generally favorable. (And thanks for the suggestion, it's definitely worth investigating). I found two minor issues: first, its Modifiers struct does not distinguish between left and right shift/ctrl/alt keys (though these are distinguished both in Code and Location as edge-triggered information, just not level-triggered). Second, the Character variant of Key uses a String, and I think it would be better to use a small string such as TinyStr, to avoid allocation when cloning. But neither of these are showstoppers, and both can be improved later.

Next up is to do a similar investigation on mac. I hope it's easier, and in any case feel it should be possible to use the same Mozilla code as a guide, especially as I am now familiar with the way it works. I might ask for help on Gtk/X, as I'm sure there's a lot more interesting stuff there.

Another thing to explore is the interaction with accelerator processing. If there's an accelerator bound to Ctrl-Z, should that be code=KeyZ (ie the physical key to the right of the left shift on most keyboards), or key=Character("z") (which would be the key to the right of "T" on a German QWERTZ layout)? Should that be dispatched from the KeyboardEvent itself, or should we use the platform accelerator dispatching? In the latter case, does that mean we will need additional mapping from a spec written in terms of KeyboardEvent to the platform identifiers? This could be a big PR by the time I'm done with it :/

raphlinus added a commit that referenced this issue Jun 20, 2020
As of this checkpoint, we generate an keyboard-types KeyboardEvent,
but don't plumb it through as the actual druid-shell keyboard event.

Also, there is no dead-key handling at this point.

Work towards #1040
@raphlinus
Copy link
Contributor Author

Probably also in scope for consideration: other input devices need to be able to know or query keyboard modifier state, so that things like Shift+click or Ctrl+scroll can work.

MouseEvent (in both druid-shell and druid) already has a mods: KeyModifiers field. As part of this work, that changes to keyboard_types::Modifiers. Is there anything else that would need work here?

@cmyr cmyr mentioned this issue Jun 25, 2020
12 tasks
@raphlinus
Copy link
Contributor Author

More investigation. Apologies this is something of an unstructured braindump, right now I basically want to get my thoughts down. I have a reasonable implementation of the basics in #1049 and would like to land that, but also identify the areas for improvement.

macOS

The current #1049 has a reasonable implementation for macOS, but with a few places where things are basic. Essentially, I'm not doing any UCKeyTranslate at the moment, which has two consequences.

The first is that dead keys are not handled at all. The handling of dead keys is different on Windows and macOS. In the former case, the platform does process them and gives you the composed character in a WM_CHAR. But on macOS, the key event layer has no dead key processing, and applications are expected to implement NSTextInputClient. We have a choice here: do Windows-style dead key processing using UCKeyTranslate as an interim solution, or punt for now and schedule IME for the future. I'm inclined to the latter.

The other issue is the handling of combinations like Ctrl-"+", which is Ctrl-Shift-KeyMinus. I believe this should report Key::Character("+"), but it's not easy to derive that, as charactersIgnoringModifiers is "-". Browsers vary; Mozilla and Chrome report "-" but Safari shows "+". I haven't dug deeply into how the latter does its mapping (the code is probably in open source WebKit but it's hard to say for sure). I wouldn't be surprised if it were UCKeyTranslate based. See the attached notes document for other discrepancies between browsers, of which there are quite a few. In the case of Caps Lock I went with the Chrome/Safari handling (report both keyup and keydown depending on lock state) over Mozilla because I felt it was more useful.

Windows hotkeys

I'm very pleased with the generation of keyboard events in Windows in #1049; I think it's about as good as it can get. But there are issues around hotkeys. I've dug deepest into Windows but suspect there are similar issues on other platforms. Maybe we should have hotkeys as a separate issue; certainly I'd like to land the PR before fully solving them.

The problems are around non-Latin layouts and also switching layouts. Let's take the latter first because it's simpler. At menu creation time, there's a mapping to virtual key codes. The problem is that on keyboard layout change (WM_INPUTLANGCHANGE), the correct mapping can change. I'm not sure how serious this is - in researching the issue, I'm finding that most apps seem to shy away from hotkeys based on keys that move around a lot (like "/", "#", etc) and stick to ones that are stable in vk even if they move around physically.

Non-Latin mapping is trickier. If the keyboard is in a non-Latin layout, then there is no mapping for, say, Ctrl-Z. There are some choices here. One is to bring in a fallback based on virtual key, ie if the mapping of "Z" to vk fails, then choose VK_Z. Another is to do something similar but based on code, which presumes a US layout. I think more research is needed here.

Note that in one case these issues partially cancel each other out. If the app is started in a US layout, then the hotkeys will be assigned based on that. Then, switching into a non-Latin layout, those mappings are still valid.

We also have to think about dispatching command keys that are not assigned as menu hotkeys (some of this is done in text_input.rs now). I think that's also follow-on work.

X11

The biggest issue on X11 is discovering a keyboard map from the system and applying it. As is usual on Linux, there are several different conflicting mechanisms for this: /etc/default/keyboard is the main one, but it can apparently be overridden by gsettings and/or IBus (again in characteristic fashion, the documentation is partially stale and confusing). For now, I hardcode a US layout, which I think is a reasonable interim.

@PoignardAzur
Copy link
Collaborator

Another thing to explore is the interaction with accelerator processing. If there's an accelerator bound to Ctrl-Z, should that be code=KeyZ (ie the physical key to the right of the left shift on most keyboards), or key=Character("z") (which would be the key to the right of "T" on a German QWERTZ layout)?

I might be missing some deeper technical context, but if you're asking "which key should the user physically press to get the ctrl-z effect", the answer is definitely the second one, as in "the key with 'Z' written on it".

... I think. I know this is how major OSes handle AZERTY keyboards, and I have no reason to expect german keyboards are different.

I have no idea how shortcuts work for non-latin keyboards, though.

@PoignardAzur
Copy link
Collaborator

PoignardAzur commented Sep 30, 2020

Also, while I'm dispensing AZERTY trivia, dead key handling doesn't work on every Linux distribution I've tried (mostly gnome debian and KDE arch).

On Windows, to write a À character, you'd first input the ` dead key (altgr+è), then uppercase A. Doing so on Linux gives you the string `A.

To write À on Linux, the simplest way is to enter capslock mode, then press the à key. Doing so on Windows gives you 0 (0 is the normal result of pressing shift+à on an AZERTY layout).

Some relatively common letters like Ê are essentially impossible to type with an AZERTY keyboard on Linux.

(this was fun to type on an Android keyboard)

@pdenapo
Copy link

pdenapo commented May 28, 2023

Hardcoding the US keyboard is not reasonable.It breaks the usability of applications depending on Duid for non-US users!
You can find some information on how to detect the current keyboard layout in X11 here

https://unix.stackexchange.com/questions/12072/how-do-i-get-current-keyboard-layout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture changes the architecture, usually breaking bug does not behave the way it is supposed to shell concerns the shell abstraction
Projects
None yet
Development

No branches or pull requests

6 participants