RFR: 8150709: Mac OSX and German Keyboard Layout (Y/Z) [v7]

Wed Apr 5 13:33:29 UTC 2023

On Tue, 21 Mar 2023 22:49:34 GMT, Martin Fox <duke at openjdk.org> wrote:

>> 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)
>
> Martin Fox has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added manual cross-platform keyboard handling test

I tested on Mac Ventura the manual test. Fails without the patch for all languages, passes with it for all of them.
I've left some comments, but the PR looks good.

buildSrc/mac.gradle line 156:

> 154:         "-framework", "AppKit",
> 155:         "-framework", "ApplicationServices",
> 156:         "-framework", "Carbon",

Change license header copyright to 2023

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 34:

> 32: #import "GlassKey.h"
> 33: 
> 34: #import <Carbon/Carbon.h>

Change license header copyright to 2023

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 195:

> 193:     }
> 194: 
> 195:     if (keyCode >= 0x00 && keyCode <= 0x32)

Add a comment? like `// Null to [Space]`

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 195:

> 193:     }
> 194: 
> 195:     if (keyCode >= 0x00 && keyCode <= 0x32)

out of curiosity, why not?
` if ((keyCode >= 0x00 && keyCode <= 0x32) || (keyCode >= 0x5D && keyCode <= 0x5F))`

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 198:

> 196:         return YES;
> 197: 
> 198:     if (keyCode >= 0x5D && keyCode <= 0x5F)

same: `'' to ']'`

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 330:

> 328:         // A handful of keyboards (Azeri, Turkmen, and Sami variants) can only access
> 329:         // critical letters like Q by using the Option key in conjunction with Cmd.
> 330:         // In this API the Cmd flag suppresses the Option flag so we ommit Cmd.

typo s/ommit/omit

modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 473:

> 471:     // If the QWERTY key is in the layout sensitive area search the other keys in that
> 472:     // area. We may not find a key so returning NO is possible.
> 473:     for (unsigned short trialKey = 0x00; trialKey <= 0x7E; ++trialKey)

I see this as a last resource, in case everything else failed so far. But I wonder if this could be slow? 
Also given that keyCode in sensitive layout is true only for [0x00 - 0x32] and 0x5D, 0x5F, maybe you could improve this?

tests/manual/events/KeyboardTest.java line 78:

> 76:  * KeyCharacterCombinations don't work reliably on most platforms (for now).
> 77:  */
> 78: 

As discussed before, please add a comment on how to enable Terminal access to Accessibility features.

tests/manual/events/KeyboardTest.java line 458:

> 456:         GERMAN("German", KeyListBuilder.germanKeys()),
> 457:         LATIN("Latin", KeyListBuilder.latinKeys()),
> 458:         NON_LATIN("non-Latin", KeyListBuilder.nonLatinKeys());

How hard would be adding another languages to this test, like Spanish? I see that you already commented that the robot can't access dead keys (so no possible test for `ñ` for instance...)?

tests/manual/events/KeyboardTest.java line 458:

> 456:         GERMAN("German", KeyListBuilder.germanKeys()),
> 457:         LATIN("Latin", KeyListBuilder.latinKeys()),
> 458:         NON_LATIN("non-Latin", KeyListBuilder.nonLatinKeys());

Out of curiosity, I tested Cantonese keyboard with Non-Latin, got this printout:

2023-04-05 15:25:55.539 java[8997:135623] +[CATransaction synchronize] called within transaction

not sure what it means or if it is relevant?

-------------

PR Review: https://git.openjdk.org/jfx/pull/425#pullrequestreview-1372819730
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158482688
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158482779
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158460512
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158463612
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158462598
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158468865
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158477269
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158480033
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158517055
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1158519520