8278924: [Linux] Robot key test can fail if multiple keyboard layouts are installed

[beldenfox]

The Robot implementation on Linux did not consult the current layout when mapping from a KeyCode to a hardware code. Internally it retrieved results for all the layouts but just picked the first one it saw leading to random effects. Though not part of the original bug report, the code also ignored the shift level when choosing which result to pick. On a French layout the dollar sign is on two keys (AltGr 4 is the second one) and the code could choose either one. Same is true for pound.

This PR consults the current layout and only on shift level 0 which is the same level used in get_glass_key to figure out which KeyCode to assign when generating a KeyEvent.

---

Progress

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

Issue

• JDK-8278924: [Linux] Robot key test can fail if multiple keyboard layouts are installed

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 718

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/718.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

• 02: Full - Incremental (ca9f5bf7)
• 01: Full - Incremental (51fcd047)
• 00: Full (ad88f044)

[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).

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[kevinrushforth]

@tsayao or @andy-goryachev-oracle or @jperedadnr would one of you also be able to review this?

[tsayao]

@beldenfox Could you point out which tests were failing? test.robot.javafx.embed.swing.SwingNodeJDialogTest is getting stuck for me.

[beldenfox]

@tsayao The best place to start is with the manual test that I added to PR #425 (KeyboardTest.java). That test works on Windows, Mac, and Linux and uses a Robot to throw a whole slew of platform key events at the system and then verify that the right JavaFX KeyEvents come through on the other side.

The primary motivation for this PR is to pave the way for future PR's. Accelerators involving punctuation and symbols aren't working at all well on Linux (see JDK-8273743) and having a working Robot in hand will be extremely helpful in testing the fixes. The manual test in #425 can also be configured to test KeyCharacterCombinations (the component that's broken) but for now you can ignore all that.

Unfortunately the manual test itself is a big chunk of code that needs to be reviewed but it is the first comprehensive test written for JavaFX keyboard handling. I wish I could make it shorter (it looks more complicated than it is) but the only way to test the keyboard system is to press a lot of keys.

BTW, the bot that kicked this PR has lousy timing. I'll be out of town for most of the coming week and will be away from my Linux box.

[tsayao]

Without the fix:

With the fix:

[tsayao]

@beldenfox I've done some research to understand your code. It seems gdk_keymap_get_entries_for_keyval will return multiple entries for hardware keys (as many as the layouts installed). So your change queries which one corresponds to the current layout. It seems correct to me, but I'll leave some suggestions.

To observe the problem using pure gdk:

#include <gdk/gdk.h>

int main(int argc, char *argv[]) {
    GdkKeymap *keymap;
    guint keyval = GDK_KEY_a;  // Example keyval
    guint upper_keyval = gdk_keyval_to_upper(keyval);
    GdkKeymapKey *keys;
    gint n_keys;
    gint group;

    // Initialize GDK
    gdk_init(&argc, &argv);

    // Get the default keymap
    keymap = gdk_keymap_get_for_display(gdk_display_get_default());

    // Get the keycodes for the uppercase character
    gdk_keymap_get_entries_for_keyval(keymap, upper_keyval, &keys, &n_keys);

    // Print the keycodes
    for (int i = 0; i < n_keys; i++) {
        printf("Keycode: %d\n", keys[i].keycode);
    }

    // Free resources
    g_free(keys);
    g_object_unref(keymap);

    return 0;
}

gcc -o testkey testkey.c `pkg-config --cflags --libs gdk-3.0`

[beldenfox]

For anyone else looking at this code: the GTK documentation on how groups and levels work doesn't match the behavior seen on X11 systems. This whole area is sketchily documented. A quick summary:

The keymap includes information on all of the keyboard layouts the user has installed. A call like gdk_keymap_get_entries_for_keyval returns results that span all the installed layouts and it's up to the client to sift through them to find the entries relevant to the current layout. The layout is specified in the group field which is an index into the user's list of configured layouts with 0 at the top (as presented in the UI). The level field indicates the modifier state (0 for unshifted, 1 for shifted, etc.)

Under certain circumstances all of the results may be in group 0 even if that's not the active layout. I've seen this happen when the current layout is non-Latin (e.g. Russian) or when querying values on the numeric keypad. So if all the results are in group 0 the group fields should be ignored.

If the user configures a non-Latin layout GTK (or XKB) will append an additional U.S. QWERTY layout/group under the hood. Presumably this is added to ensure there's always a Latin layout around to resolve accelerators like Ctrl+C.

[beldenfox]

The results returned from gdk_keymap_get_entries_for_keyval match the way the keys events are delivered from the OS. The numlock state can affect the level and some keys (like ones on the numeric keypad) always appear on group 0 even if that's not the current group. When searching through the results we have to use translate_keyboard_state to find the correct group and level to match against. Code has been updated along with a few minor fixes.

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[beldenfox]

Leaving a comment so this pull request isn't closed. Working Robot code is vital for testing other parts of keyboard handling, like #694.

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.