Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273506: java Robot API did the 'm' keypress and caused /awt/event/KeyEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12 #8320

Closed
wants to merge 2 commits into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Apr 20, 2022

Clear the kCGEventFlagMaskSecondaryFn flag if it is set before posting keyboard events to the system queue.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

  • JDK-8273506: java Robot API did the 'm' keypress and caused /awt/event/KeyEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8320

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

Using diff file

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

…yEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12

Clear the kCGEventFlagMaskSecondaryFn flag before posting the keyboard event to the system queue.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 20, 2022

👋 Welcome back kizune! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 20, 2022
@openjdk
Copy link

openjdk bot commented Apr 20, 2022

@azuev-java The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Apr 20, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 20, 2022

Webrevs

@JB-Dmitry
Copy link
Contributor

Just a note - with the proposed fix, some generated events will be different from those generated by a real keyboard - namely events for arrow keys and F1-F12 keys, as those have 'inherent' Fn modifier set, regardless of whether Fn key was pressed during event generation. This won't be observable from Java applications, as Java APIs don't expose Fn modifier in KeyEvent-s, but can theoretically make some difference when interacting with other apps (I don't know about any problematic cases currently though). If this can be considered a problem, it's possible to improve the fix by not clearing Fn flag for key events 'inherently' having that Fn modifier. Simple approach might just hard-code them, and a more robust one (but a bit more complicated as well) can determine the set of affected keycodes on Robot startup, by examining flags set by CGEventCreateKeyboardEvent in a 'clear' event source state.

@azuev-java
Copy link
Member Author

Just a note - with the proposed fix, some generated events will be different from those generated by a real keyboard - namely events for arrow keys and F1-F12 keys

As far as i can tell they are not different, the Quartz layer adds these flags back for the composite keys so functionally there is no difference even when we interact with the external applications. The problem was that Quartz code does not clear them when events are processed and that is what caused the issue on AWT Robot side of things.

@JB-Dmitry
Copy link
Contributor

I assumed that whatever keys we set on an event being posted using CGEventPost, are delivered unchanged to the 'receiving' applications. But I did a test, and that doesn't seem to be the case. You're right, Quartz (or some other subsystem) does set those flags before delivering the events. Sorry for the noise.
Interestingly, extending the fix to NumPad flag (that 'sticks' in the global state similarly) doesn't work in the same way - the flag isn't being added automatically for arrow keys' KeyUp event by Quartz, but that's out of scope.

@kevinrushforth kevinrushforth self-requested a review April 21, 2022 15:49
@mrserb
Copy link
Member

mrserb commented Apr 22, 2022

I am not sure do we need to discard the possibility to press the shortcuts via the FN key, if the user may press them then why the robot cannot do the same? Looks like the behavior of this key is similar to the capslock which can be toggled on/off and may have a similar issue? If yes then we can press that button in the test twice to set an initial state(similar to how could fix the same issue for the capslock).

@azuev-java
Copy link
Member Author

azuev-java commented Apr 23, 2022

I am not sure do we need to discard the possibility to press the shortcuts via the FN key, if the user may press them then why the robot cannot do the same? Looks like the behavior of this key is similar to the capslock which can be toggled on/off and may have a similar issue? If yes then we can press that button in the test twice to set an initial state(similar to how could fix the same issue for the capslock).

Ok, starting from the beginning - there is no possibility to press the Function key with Robot, the corresponding key code (OSX_Function - 0x3F) has not mapped to any of the keys available to Robot. Function key does not act like a caps lock - it acts like a shift. Just like a shift, it can only be fixated by the accessibility feature "sticky keys". Once Function key is released the global state should be changed in a way as if this key is not active. We do not issue key press or key release event for the Function key. The reason of why global state is being changed this way when we issue key press/key release sequence for a totally unrelated key (OSX_DownArrow - 0x7D) is unknown, it happens in the native code outside of our control. All we can do is to eliminate the consequences of this action that started to cause problems in Mac OS X 12.

@mrserb
Copy link
Member

mrserb commented Apr 26, 2022

Ok, starting from the beginning - there is no possibility to press the Function key with Robot, the corresponding key code (OSX_Function - 0x3F) has not mapped to any of the keys available to Robot. Function key does not act like a caps lock - it acts like a shift. Just like a shift, it can only be fixated by the accessibility feature "sticky keys". Once Function key is released the global state should be changed in a way as if this key is not active. We do not issue key press or key release event for the Function key. The reason of why global state is being changed this way when we issue key press/key release sequence for a totally unrelated key (OSX_DownArrow - 0x7D) is unknown, it happens in the native code outside of our control. All we can do is to eliminate the consequences of this action that started to cause problems in Mac OS X 12.

That description is very helpful, thank you. Based on that it looks like this is a bug in the macOS, did we report it to the Apple?

@azuev-java
Copy link
Member Author

azuev-java commented Apr 27, 2022

That description is very helpful, thank you. Based on that it looks like this is a bug in the macOS, did we report it to the Apple?

No, we did not. I probably will but it is unlikely it will be addressed quickly so fixing this issue on our side seems to be required anyways.

@mrserb
Copy link
Member

mrserb commented May 3, 2022

d not. I probably will but it is unlikely it will be addressed quickly so fixing this issue on our side seems to be required anyways.

Please add a "feedback id" to the JBS, so it will be possible to refer that macOS bug later.

@openjdk
Copy link

openjdk bot commented May 3, 2022

@azuev-java This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273506: java Robot API did the 'm' keypress and caused /awt/event/KeyEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12

Reviewed-by: serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 323 new commits pushed to the master branch:

  • af1ee1c: 8283684: IGV: speed up filter application
  • 7a48351: 8280568: IGV: Phi inputs and pinned nodes are not scheduled correctly
  • 64b5b2b: 8282828: CDS uncompressed oops archive is not deterministic
  • 45ca81f: 8285915: failure_handler: gather the contents of /etc/hosts file
  • 3420a1a: 8286013: Incorrect test configurations for compiler/stable/TestStableShort.java
  • fbcd874: 8285979: G1: G1SegmentedArraySegment::header_size() is incorrect since JDK-8283368
  • 50a4df8: 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry
  • f973b78: 8286028: Some -Xlint keys are missing in javac man page
  • 9d8c3bf: 8285745: Re-examine PushbackInputStream mark/reset
  • 41de506: 8285507: revert fix for JDK-8282704 now that JDK-8282952 is fixed
  • ... and 313 more: https://git.openjdk.java.net/jdk/compare/4451257b1432e4180a16757aafca6141b8063772...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 3, 2022
@azuev-java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 3, 2022

Going to push as commit 39e50c2.
Since your change was applied there have been 328 commits pushed to the master branch:

  • 3cbf769: 8285977: Add links to IEEE 754 specification
  • 4434c7d: 8265360: several compiler/whitebox tests fail with "private compiler.whitebox.SimpleTestCaseHelper(int) must be compiled"
  • ffca23a: 8284490: Remove finalizer method in java.security.jgss
  • 0f62cb6: 8285921: serviceability/dcmd/jvmti/AttachFailed/AttachReturnError.java fails on Alpine
  • 6fcd322: 8279622: C2: miscompilation of map pattern as a vector reduction
  • af1ee1c: 8283684: IGV: speed up filter application
  • 7a48351: 8280568: IGV: Phi inputs and pinned nodes are not scheduled correctly
  • 64b5b2b: 8282828: CDS uncompressed oops archive is not deterministic
  • 45ca81f: 8285915: failure_handler: gather the contents of /etc/hosts file
  • 3420a1a: 8286013: Incorrect test configurations for compiler/stable/TestStableShort.java
  • ... and 318 more: https://git.openjdk.java.net/jdk/compare/4451257b1432e4180a16757aafca6141b8063772...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 3, 2022
@openjdk openjdk bot closed this May 3, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 3, 2022
@openjdk
Copy link

openjdk bot commented May 3, 2022

@azuev-java Pushed as commit 39e50c2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants