On Thu, 6 Jul 2023 00:04:08 GMT, Harshitha Onkar <[email protected]> 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
