RFR: JDK-8302618: [macOS] Problem typing uppercase letters with java.awt.Robot when moving mouse [v5]

[Alexey Ivanov]

On Thu, 6 Jul 2023 00:04:08 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> **Problem:**
>> 
>> On macOS, Robot erroneously produces lowercase letter when mouse is moved (manually) in unison with Robot's keyEvents. This issue was originally logged by a developer of an on-screen accessibility keyboard  - TouchBoard. Originally reported at https://github.com/adoptium/adoptium-support/issues/710
>> 
>> The issue is reproducible on JDK versions 22 to 11, but works fine on JDK-8. (details below) 
>> 
>> This issue is not restricted to the Shift modifier key and causes problems with other modifier keys as well and in some scenarios without any external mouse movement.
>> 
>> - This works correctly on JDK-8 up to JDK-9+129 when Accessibility APIs (AXUIElementCreateSystemWide/ AXUIElementPostKeyboardEvent) were used. Later on it was changed to CGEvents. 
>> 
>> - With the present code, the issue occurs at [CRobot.m#L295](https://github.com/openjdk/jdk/blob/ac6af6a64099c182e982a0a718bc1b780cef616e/src/java.desktop/macosx/native/libawt_lwawt/awt/CRobot.m#L295.)  The flags gets reset or cleared when mouse is moved physically in unison with Robot's key events.
>> 
>> - The physical mouse movement causes the event flags to be reset. 
>> 
>> **Impact:**
>> 
>> Modifier keys don't work as expected when using Robot with any simultaneous physical mouse movement and in case of TouchBoard, this behavior breaks the usability of the on-screen a11y keyboard. There is no known workaround for this particular use case except for reverting to JDK-8. More details on this use case [here.](https://github.com/adoptium/adoptium-support/issues/710#issuecomment-1594103280)
>> 
>> **Proposed Fix:**
>> 
>> - In order to avoid resetting of the CGEventFlags here [CRobot.m#L295](https://github.com/openjdk/jdk/blob/ac6af6a64099c182e982a0a718bc1b780cef616e/src/java.desktop/macosx/native/libawt_lwawt/awt/CRobot.m#L295.), the CGEvent flag state is obtained in `initRobot` (stored in initFlags) which is later used within `CRobot_keyEvent`.
>> 
>> - The incoming keyCode is used to determine whether it is a modifier key and the corresponding modifierFlagMask is either added or cleared from the initFlags based on whether the modifier key was pressed or released.
>> 
>> - Finally, only the required and known flag bits from initFlag are copied over to local flag which is used in `CGEventSetFlags()`.
>> 
>> **Testing:**
>> 
>> The newly added test - RobotModifierMaskTest.java tests for Shift, Caps, Control, Option and Command keys.
>> The test is run in 2 modes - as automated and manual test.
>> 
>> CASE 1 :...
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor test changes

I admit I don't understand the Objective-C code, yet it couple of points caught my attention. I mostly looked at the test as I did the previous time.

src/java.desktop/macosx/native/libawt_lwawt/awt/CRobot.m line 313:

> 311:                     if (keyPressed) {
> 312:                      initFlags ^= flagMaskValue;
> 313:                     }

Suggestion:

                    if (keyPressed) {
                        initFlags ^= flagMaskValue;
                    }

Shouldn't it get indented?

src/java.desktop/macosx/native/libawt_lwawt/awt/CRobot.m line 322:

> 320: 
> 321:             CGEventFlags flags = CGEventSourceFlagsState(kCGEventSourceStateHIDSystemState);
> 322:             flags  = (initFlags & allModifiersMask) | (flags & (!allModifiersMask));

Should `!allModifiersMask` be `~allModifiersMask`? That is _bitwise_ NOT instead of _logical_ NOT.

Suggestion:

            flags = (initFlags & allModifiersMask) | (flags & (!allModifiersMask));

test/jdk/java/awt/Robot/RobotModifierMaskTest.java line 78:

> 76:             move the mouse (without clicking/dragging).
> 77:             When ready click on the "Start" button to run the test.
> 78:             """;

And start moving the mouse?

test/jdk/java/awt/Robot/RobotModifierMaskTest.java line 102:

> 100:                             throw new RuntimeException("Test instruction frame closed");
> 101:                         }
> 102:                     }

Instead of busy-waiting, you can use `CountDownLatch` to start the test. You should also release the latch if the instruction frame is closed.

test/jdk/java/awt/Robot/RobotModifierMaskTest.java line 237:

> 235:         jFrame.getContentPane().add(jButton, BorderLayout.PAGE_END);
> 236: 
> 237:         jFrame.setSize(560,200);

Suggestion:

        jFrame.setSize(560, 200);

test/jdk/java/awt/Robot/RobotModifierMaskTest.java line 250:

> 248:         jFrame.getContentPane().add(pane, BorderLayout.CENTER);
> 249: 
> 250:         jFrame.setSize(450,100);

Suggestion:

        jFrame.setSize(450, 100);

test/jdk/java/awt/Robot/RobotModifierMaskTest.java line 261:

> 259:             boolean condition = expectedResult.equals(EXPECTED_RESULT_CTRL)
> 260:                                 ? (jTextArea.getCaretPosition()
> 261:                                      != Integer.parseInt(EXPECTED_RESULT_CTRL))

You pass different expected result strings but here you always compare `expectedResult` to `EXPECTED_RESULT_CTRL`. Is it intentional? Do I miss anything?

I haven't run the test.

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

PR Review: https://git.openjdk.org/jdk/pull/14744#pullrequestreview-1522945218
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258848971
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258864126
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258876531
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258875171
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258877726
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258877487
PR Review Comment: https://git.openjdk.org/jdk/pull/14744#discussion_r1258879793