8150709: Mac OSX and German Keyboard Layout (Y/Z)

[beldenfox]

This PR adds code to ensure that KeyCodeCombinations match KeyEvents as expected by more accurately mapping from a Mac key code to a Java key code based on the user’s active keyboard layout (the existing code assumes a US QWERTY layout). The new code first identifies a set of Mac keys which can produce different characters based on the user’s keyboard layout. A Mac key code outside that area is processed exactly as before. For a key inside the layout-sensitive area the code calls UCKeyTranslate to translate the key to an unshifted ASCII character based on the active keyboard and uses that to determine the Java key code.

When performing the reverse mapping for the Robot the code first uses the old QWERTY mapping to find a candidate key. If it lies in the layout-sensitive area the code then scans the entire area calling UCKeyTranslate until it finds a match. If the key lies outside the layout-sensitive area it’s processed exactly as before.

There are multiple duplicates of these bugs logged against Mac applications built with JavaFX.

https://bugs.openjdk.java.net/browse/JDK-8090257 Mac: Inconsistent KeyEvents with alternative keyboard layouts https://bugs.openjdk.java.net/browse/JDK-8088120 [Accelerator, Mac] CMD + Z accelerator is not working with French keyboard https://bugs.openjdk.java.net/browse/JDK-8087915 Mac: accelerator doesn't take into account azerty keyboard layout https://bugs.openjdk.java.net/browse/JDK-8150709 Mac OSX and German Keyboard Layout (Y/Z)

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed

Issue

• JDK-8150709: Mac OSX and German Keyboard Layout (Y/Z)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/425/head:pull/425
$ git checkout pull/425

Update a local copy of the PR:
$ git checkout pull/425
$ git pull https://git.openjdk.java.net/jfx pull/425/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 425

View PR using the GUI difftool:
$ git pr show -t 425

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/425.diff

[bridgekeeper[bot]]

:wave: Welcome back beldenfox! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge[bot]]

Webrevs

• 10: Full - Incremental (973eaaa9)
• 09: Full - Incremental (8f80e0e3)
• 08: Full - Incremental (690709b9)
• 07: Full - Incremental (3e6fc266)
• 06: Full - Incremental (47b4eb86)
• 05: Full - Incremental (99d24ae0)
• 04: Full - Incremental (1037be20)
• 03: Full - Incremental (fb449d93c393adb1117881d5bdffeca1e36d76f8)
• 02: Full - Incremental (26a6fb0d0ad7768fff6362bdb4e423db0d9cc984)
• 01: Full - Incremental (6238c49caab61276534396d7396900e0810d4535)
• 00: Full (b2254109262f39c9ab44413d55e41eb7ee7c4967)

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

I see that the Windows pre-submit test build failed. It's clearly not related to anything in this PR, so it can be ignored.

I'll review this PR later (hopefully next week some time), but I have a couple general comments:

1. Would it be possible to provide an automated test? Maybe not since it is sensitive to the keyboard layout.
2. For the related bugs, we can either close them as duplicates of this bug or use the /solves command to list them here. Generally, we would do the former in the case it really is a single fix, as this seems to be. That's what I'll do once this bug is integrated unless there is a good reason not to. Normally we would use the earliest of the bugs, but in this case, I don't think it matters, so I have no problem with your using the one you chose.

@tomsontom Since you were the one who filed JDK-8150709, and it's currently assigned to you, do you want to be the second reviewer on this?

[tomsontom]

Sure I started looking into this last week and found the place this translation is done in swing (there it is done differently but only works for keys a-z but not for other chars like #,...)

[beldenfox]

@kevinrushforth I have a manual app that can perform a simple test to verify that when a robot sends KeyCode.A through KeyCode.Z the system receives the characters 'a' to 'z'. On the Mac this sanity test was failing on German and French keyboards prior to these changes. The test is part of a key logger app I created.

I chose this bug because it has a straight-forward test attached and some recent comment activity.

@tomsontom Could you point me to the relevant code in Swing? I'm looking at the code but am getting lost in the layers.

[tomsontom]

https://github.com/openjdk/jdk/blob/8760688d213865eaf1bd675056eb809cdae67048/src/java.desktop/macosx/native/libawt_lwawt/awt/AWTEvent.m#L462

[beldenfox]

On the French keyboard the digits are shifted but I assumed we would still want those keys to map to KeyCode.0-9 as they do on Windows. My solution was to hard-code those key positions to be digits but this interferes with Dvorak layouts. I'm tweaking the implementation to fix this. The new approach will be to query both the shifted and unshifted characters on a key and favor digits and letters over everything else (and something over nothing).

(I've been writing Mac code to enumerate all the OS keyboard layouts and generate lists showing which Java key codes will change from the historic implementation. It also does some sanity checking.)

[tomsontom]

Maybe this is a very dumb idea but why are we not using the char to map back to the keycode? At least for everything in the ASCII-Range this would be ok not? We'd only have to check for special keys (like NUM-Keys, F-Keys, ...) first. So once we checked and the text is of length 1 we could do the mapping from the char.

[beldenfox]

The Function/arrow/number pad keys are handled using a hard-coded table since they don’t vary based on keyboard layout. Beyond that you’re asking exactly the right question.

JavaFX KeyCodes seem to be modeled on similar systems present in Windows and X11. On these platforms the system assigns a key code to each key which remains stable regardless of the modifier state. The system tries to ensure that codes for A-Z and 0-9 are assigned to keys even on keyboards that don’t generate Latin characters.

My implementation is conservative in that it’s trying to emulate the behavior of Windows and X11 and associate each key with a stable code including A-Z and 0-9. On the Mac that has no real utility when it comes to accelerator matching, we could use the character information in the event to generate the JavaFX key code and still match user’s expectations. But that would break other JavaFX assumptions, like that the key code is stable. Take the =/+ key on the US keyboard as an example. If Cmd is not held down it will generate = or + depending on the shift state and so vary between KeyCode.EQUALS and KeyCode.PLUS. And key events for non-Latin characters would have an UNDEFINED KeyCode until Cmd was held down and we got a useable ASCII character.

I have no idea if these deviations from the original design would be an issue. At this point I’m pretty sure that any problems would lie outside accelerator processing e.g. with the Robot.

P.S. What I would not do is what Swing is trying, namely to emulate Windows (stable codes, A-Z present, etc.) using just the NSEvent character information and heuristics. That’s generating all sorts of oddness under the hood. Better to generate UNDEFINED KeyCodes than bad guesses.

[beldenfox]

I've written a command line utility that runs through all the keyboard layouts available in macOS 11.2.3 and generates a report on how this PR would change the Java key codes for each layout. I used it to spot-check the behavior on various layouts and to flag potential trouble spots (like Lithuanian where digits are typed using a dead key.) I thought it would also be useful for this review. For example, I can now say that with this PR Meta-, (comma) will still be available on all layouts.

If the report would be of interest where should I put it? Add it to this PR, attach it to a comment in this thread, put it someplace public and link to it?

[kevinrushforth]

I guess it depends on how large the table is. Adding it as an inline table using markdown could be helpful, unless it is so large that it takes too long to scroll past it. In that case, I'd probably recommend attaching it to a comment in the PR.

[beldenfox]

I've attached a text document which details which Java key codes are lost and gained for each keyboard layout compared to the pre-PR code. It also flags some potential trouble spots with the word "Warning". Note that at the bottom of the file there's a long list of layouts which are unchanged. I've added it as a file to make it easier to diff against future versions if I need to tweak the algorithm some more (here's hoping I don't).

With the latest code tweak the Java key codes A to Z are available on all keyboards except Turkmen as there really is no way to type 'X' on that layout. The key codes Digit0 to Digit9 are available on all keyboards except Lithuanian where the digits require a dead-key to type.

GainsAndLosses.txt

[beldenfox]

The question was raised earlier why we can’t just generate the key code based on the typed character. That’s basically the Swing approach and I could evaluate what it would mean to adopt that algorithm in JavaFX. I know it produces some strange results under the hood but have reached the conclusion, based on the bugs I’ve seen, that for keys that generate printable characters we have a lot of leeway as long as Cmd+A through Cmd+Z work.

I haven’t seen any documentation on how a developer should set up an accelerator associated with a digit e.g. Cmd+2. Should they use a KeyCharacterCombination, a KeyCodeCombination, or are just not do it? Are there written guidelines on any of this?

[beldenfox]

I would like to get this review restarted. Sorry I let it lapse for so long.

I was asked to evaluate a Swing-style solution which determines the Java key code based on the keystroke’s character. If the character can’t be mapped to a key code the system falls back to the US QWERTY key code instead.

I’ve run some test code that loops through the keyboard layouts in macOS (all 189 of them) to sanity-check this approach. The executive summary is that it’s entirely work-able and in isolation would involve a lot less code than the current PR. I say "in isolation" because I’m also working on code to fix problems with KeyCharacterCombinations on the Mac and those fixes rely on some of the same machinery, particularly the calls to UCKeyTranslate, so even if we removed most of that code for this PR some of it would just re-appear in the next.

If you want I could put together a separate PR that uses the Swing-style solution for comparison purposes. To make it a fair comparison I would also incorporate the changes for the KeyCharacterCombination problems (I don’t have the bug numbers handy).

The one place where this approach falls down is the Robot. To send a key event using the Robot we have to map from a Java key code to a physical key. If the key code is determined based on data that’s only available while processing a keystroke it’s just not possible to map the other way reliably. Swing punts and assumes a US QWERTY layout as does the current JavaFX code. This has produced bad results for years but based on the bug database I'm not sure anyone has noticed. (The code I submitted with this PR assigns key codes without reference to key strokes and can work in both directions reliably.)

A comment in the Swing sources describes this approach as "hokey" and I agree. It relies on most keyboard layouts being similar to US QWERTY and that most of the system ignores non-letter and non-digit key codes (this algorithm often generates bogus codes). But for all it’s hokey-ness it gets the job done surprisingly well.

[tomsontom]

For reference this was the code I had around from my first iteration working on that problem

diff --git a/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m b/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
index 38a828a41e..8657519f13 100644
--- a/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
+++ b/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
@@ -247,6 +247,24 @@ jint GetJavaKeyCodeFor(unsigned short keyCode)
 
 jint GetJavaKeyCode(NSEvent *event)
 {
+	NSString *chars = [event charactersIgnoringModifiers];
+	
+	if ([chars length] > 0) {
+		NSInteger offset;
+	
+		unichar ch = [chars characterAtIndex:0];
+		if ([[NSCharacterSet letterCharacterSet] characterIsMember:ch]) {
+			NSLog(@"Letter char");
+			unichar lower;
+			lower = tolower(ch);
+			offset = lower - 'a';
+			if (offset >= 0 && offset <= 25) {
+				NSLog(@"Hello Char it is");
+				return com_sun_glass_events_KeyEvent_VK_A + offset;
+			}
+		}
+	}
+	
     return GetJavaKeyCodeFor([event keyCode]);
 }

[beldenfox]

Is there some reason you would prefer a Swing-style implementation over the approach I submitted in this PR? The Swing code breaks down if an accelerator calls for the Option modifier alone or in addition to Command. I'm still investigating work-arounds.

(Long story short, Command alters the character we receive in a good way, providing a low-ASCII character even on non-ASCII layouts like Cyrillic. Option alters the character in a bad way, often producing an arbitrary symbol. Prior to macOS 10.15 we can't toss one modifier without tossing the other short of using UCKeyTranslate.)

[tomsontom]

@beldenfox I did not say that the swing version is the way to go and in the end its @kevinrushforth call what route should be taken - I just wanted to show what my initial change would have been without saying it is better than what you are proposing which sounds like is more complete than what swing provides today.

[beldenfox]

@tomsontom I've added the Swing-style code as WIP PR #519. There are comments in the PR that probably should be in the code but I wanted to reduce the code diffs.

PR #519 (and Swing) tend to get codes for punctuation keys wrong on non-US layouts. That's not a big issue for accelerator processing since accelerators involving punctuation should use KeyCharacterCombinations which work even if the underlying key codes are wrong. (I can't seem to find documentation that tells developers how to choose between KeyCodeCombinations and KeyCharacterCombinations. Am I missing something?)

There are isolated instances where PR #519 assigns a letter code to a punctuation key. For example, on the French keyboard it assigns KeyCode.M to the comma/question mark key which is where the M key is on the US layout. This means an app that uses both M and one of those punctuation characters as accelerators might see both fire. This might be a non-issue, the same problem afflicts the current code and I didn't run across any bugs on that.

[johanvos]

@beldenfox Can you somehow add the test you talked about in a previous comment? It always helps to have a test that fails before and succeeds after. Let me know if you need help with this?

[beldenfox]

@johanvos I've attached my test app to this comment. The test I was referring to is invoked using the A-Z Test button down at the bottom. The test just sends KeyCodes A-Z using the Robot and verifies that the expected KeyCodes and characters come back.

KeyboardTest.txt

[christianheilmann]

@beldenfox @johanvos I would kindly ask for any updates on this issue? For our customers frequently used keycombinations are broken for all non us keyboards. What can we do to bring this PR to a finished state?

[kevinrushforth]

This is waiting on review from me (and others). I agree that it has been waiting for too long. It will take some time to review and, test, but I need to bump it up in priority.

What will help is additional reviewers / testers to ensure no regressions.

[beldenfox]

I’m not sure how to approach regression testing. MacOS 11 ships with 189 keyboard layouts and this PR changes the key assignments for 62 of them. On each layout there are just under 50 keys that might be affected. Some automated or manual sanity-checking tests would go a long way but the Robot code has bugs on every platform and I want to deal with the user-facing issues first (though this PR also fixes the Robot on Mac).

For manual testing I run a JavaFX app that dumps out info on each key event. There’s one attached to this e-mail thread under the name KeyboardTest.txt. I use the Mac Keyboard Viewer app to visualize layouts that don’t match what’s printed on my keyboard. It’s also helpful to compare against a Windows box if you have one.

When testing code changes I rely on a command line tool that runs through all the layouts and compares the results of the new algorithm against the old algorithm. This is useful for flagging issues that no amount of manual testing would be likely to find (ask me about Lithuanian digits some day) but I don’t really expect other testers to go that far.

If you are testing this keep in mind that there are no key codes corresponding to accented characters or other characters that bear diacritic marks. Those keys will be assigned KeyCode.UNDEFINED. And stay away from dead keys, the way they behave on the Mac is, um, complicated.

[beldenfox]

I can't build this branch anymore, something related to compiling CCTaskGroovy. I think I need to merge or rebase to the latest version of master. Which is preferred, merge or rebase?

[kevinrushforth]

I can't build this branch anymore, something related to compiling CCTaskGroovy. I think I need to merge or rebase to the latest version of master. Which is preferred, merge or rebase?

Merge, please.

[johanvos]

This looks ok, but I'm still a bit worried about possible regression. I understand that rigorous automated tests are hard in this case, but I wonder if we can have some basic tests at least. Is there an issue in the Robot implementation preventing this?

[beldenfox]

Let me start by saying that I already have two small changes to make to this PR. One is a fix for a bug in the Robot code. The other is a small tweak to the way dead keys are assigned key codes (I recently did a deep dive on dead keys). I will submit the changes in the next day or two.

The regression that concerns me the most is losing access to key codes that are likely to be bound to KeyCodeCombination shortcuts. This includes the fixed-function keys (like KeyCode.SPACE and KeyCode.TAB) as well as KeyCode.A through KeyCode.Z. The attached KeyboardTest app contains a manual test for most of these keys (it's the "A-Z test" button at the bottom).

With this PR the Robot implementation for letter keys will be good enough on all platforms that we can expand automated testing. But there's already a basic test for this (test.robot.javafx.scene.RobotTest) that fails on the Mac if the layout is French. I'm not sure why that failure was never reported. Expanding the test to cover more keys won't do any good if the test isn't run against non-US layouts and that requires manual setup.

On every platform the Robot implementation has bugs related to punctuation and symbols. The fixes aren't that deep and I'll be submitting PR's for them (this PR already fixes the Mac). Once those bugs are fixed the testing will still be limited and manual. You have to know the current layout to craft a useful test and there's no API for querying the layout much less forcing it to the one you want to test.

The KeyboardTest app has some limited tests specific to US, German, and French layouts that are useful for sanity checking. Unfortunately the attached version also include checks for KeyCharacterCombination matching which fail on the Mac (for now). I will tweak the app to make that part of the test optional. I was planning to submit this app as a manual test once the various platforms were in better shape.

[beldenfox]

I’ve updated the PR with three bug fixes that were in my queue. Sorry for not getting them in earlier.

GetGlassKey (used by the Robot) - The table only covers US QWERTY and is missing entries for codes like PLUS or NUMBER_SIGN. There was a bug that prevented these codes from working on any layout. That’s been fixed. The same routine was also returning a bogus result when asked to look up UNDEFINED. That’s also been fixed.

getJavaCodeForMacKey - The probe for the Option key character was intended to find letters. In some instances it could find a symbol and use that to determine the code. This could cause a key like Ä or È to be given a code like SLASH. That’s been fixed to better match Windows and Linux where these keys are UNDEFINED.

I’ve attached a sanity test for the Robot code. Every time you press a key it just sends the KeyCode to a Robot and verifies that a TYPED event with the same code is received.

RobotKeySanityTest.txt