8311922: [macOS] right-Option key fails to generate release event #15432

DamonGuy · 2023-08-25T17:53:51Z

Previously, a new key combination involving the option key was added to Aqua LAF for JTextAreas. In doing so, some code was removed that created this regression. The regression caused the right option key on Mac OS to incorrectly show another KeyPressed event instead of a KeyReleased event when pressing and releasing the key. Additionally, the location of the key was 'standard' instead of 'right'. Adding back the key mask and its following code resolves the issue and doesn't cause any other CI tests to fail.

The headful test included displays the key events as they're pressed. After the changes, the test correctly shows the right option key's KeyPressed and KeyReleased events and shows the location as 'right'.

Progress

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

Issue

JDK-8311922: [macOS] right-Option key fails to generate release event (Bug - P3)

Reviewers

Harshitha Onkar (@honkar-jdk - Committer) ⚠️ Review applies to d1b3ae5e
Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15432/head:pull/15432
$ git checkout pull/15432

Update a local copy of the PR:
$ git checkout pull/15432
$ git pull https://git.openjdk.org/jdk.git pull/15432/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15432

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15432.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-25T17:55:19Z

👋 Welcome back dnguyen! 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 · 2023-08-25T17:57:31Z

@DamonGuy 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.

mlbridge · 2023-08-25T18:01:43Z

Webrevs

honkar-jdk · 2023-08-25T22:18:42Z

@DamonGuy Tested with the patch. Observed that the key code and extended modifier differ for left vs right option key. This is not the case for other left/right modifier keys such as Shift, Command. I think some extra flags (key masks) are being set in case of Right Option Key.

LEFT SHIFT KEY
-----------------
KEY PRESSED: 
 key code = 16 (⇧)
 extended modifiers = 64 (⇧)
 action key? NO
 key location: left
KEY RELEASED: 
 key code = 16 (⇧)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: left

RIGHT SHIFT KEY
-----------------
KEY PRESSED: 
 key code = 16 (⇧)
 extended modifiers = 64 (⇧)
 action key? NO
 key location: right
KEY RELEASED: 
 key code = 16 (⇧)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: right

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

LEFT OPTION KEY
------------------
KEY PRESSED: 
 key code = 18 (⌥)
 extended modifiers = 512 (⌥)
 action key? NO
 key location: left
KEY RELEASED: 
 key code = 18 (⌥)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: left

RIGHT OPTION KEY
-------------------
KEY PRESSED: 
 key code = 65406 (⌥)
 extended modifiers = 8704 (⌥+⌥)
 action key? NO
 key location: right
KEY RELEASED: 
 key code = 65406 (⌥)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: right

prsadhuk · 2023-08-28T05:25:55Z

I guess it was removed in as part of JDK-8267374 fix. Please see PR#9230 where it was concluded that we dont need ALT_GRAPH mapping for macos and RightOption key was mapped to ALT_GRAPH which makes Option key not work
#9230 (comment)
#9230 (comment)

Please run the regression test added/modified to see it does not regress those, mainly
test/jdk/javax/swing/JTextArea/TestAltUpDown.java

DamonGuy · 2023-08-28T21:18:17Z

@DamonGuy Tested with the patch. Observed that the key code and extended modifier differ for left vs right option key. This is not the case for other left/right modifier keys such as Shift, Command. I think some extra flags (key masks) are being set in case of Right Option Key.

LEFT SHIFT KEY
-----------------
KEY PRESSED: 
 key code = 16 (⇧)
 extended modifiers = 64 (⇧)
 action key? NO
 key location: left
KEY RELEASED: 
 key code = 16 (⇧)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: left

RIGHT SHIFT KEY
-----------------
KEY PRESSED: 
 key code = 16 (⇧)
 extended modifiers = 64 (⇧)
 action key? NO
 key location: right
KEY RELEASED: 
 key code = 16 (⇧)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: right

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

LEFT OPTION KEY
------------------
KEY PRESSED: 
 key code = 18 (⌥)
 extended modifiers = 512 (⌥)
 action key? NO
 key location: left
KEY RELEASED: 
 key code = 18 (⌥)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: left

RIGHT OPTION KEY
-------------------
KEY PRESSED: 
 key code = 65406 (⌥)
 extended modifiers = 8704 (⌥+⌥)
 action key? NO
 key location: right
KEY RELEASED: 
 key code = 65406 (⌥)
 extended modifiers = 0 (no extended modifiers)
 action key? NO
 key location: right

Thanks for catching this. I believe the edit I just pushed fixes this. My test now shows the correct key code and location on my end.

DamonGuy · 2023-08-28T21:19:16Z

I guess it was removed in as part of JDK-8267374 fix. Please see PR#9230 where it was concluded that we dont need ALT_GRAPH mapping for macos and RightOption key was mapped to ALT_GRAPH which makes Option key not work #9230 (comment) #9230 (comment)

Please run the regression test added/modified to see it does not regress those, mainly test/jdk/javax/swing/JTextArea/TestAltUpDown.java

I see the comments. I removed the code that was removed again and instead edited the existing NSAlternateKeyMask. Fix now shows the correct key code as well. TestAltUpDown passes as well as my new test.

alisenchung · 2023-08-28T22:09:26Z

why was the keycode wrong with the previous fix? and why do we need to manually set leftAltKeyPressed = YES? should this be done in a different part of the code?

honkar-jdk · 2023-08-29T00:27:58Z

src/java.desktop/macosx/native/libawt_lwawt/awt/AWTEvent.m

@@ -291,7 +291,7 @@
        //kCGSFlagsMaskAppleLeftAlternateKey,
        //kCGSFlagsMaskAppleRightAlternateKey,
        58,
-        0,
+        61,


Looks like the only change required to make right option key working is adding in the rightKeyCode (61) here. I don't think it is required to revert changes related toleftAltKeyPressed.

This seems correct. I updated it to only include the key code. Re-ran my test and TestAltUpDown, and both passed still.

Updated changes look good.

DamonGuy · 2023-08-29T17:12:21Z

why was the keycode wrong with the previous fix? and why do we need to manually set leftAltKeyPressed = YES? should this be done in a different part of the code?

I originally added back the code that was removed. There seemed to be 2 instances of NSAlternateKeyMask afterwards, so the key code value was incorrect. This must've been the case before the regression as well I'd assume. leftAltKeyPressed has been removed as well since it doesn't seem necessary to fix this particular issue.

test/jdk/java/awt/event/KeyEvent/OptionKeyEventTest.java

honkar-jdk

LGTM. Manual test looks good.

I guess this test cannot be automated using Robot since the VK_ALT defaults to the left option key.

DamonGuy · 2023-08-29T19:56:12Z

LGTM. Manual test looks good.

I guess this test cannot be automated using Robot since the VK_ALT defaults to the left option keypress.

Right. I looked into automating this and making the test headless, but I didn't see a way around this. Thanks

openjdk · 2023-09-20T18:34:17Z

@DamonGuy 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:

8311922: [macOS] right-Option key fails to generate release event

Reviewed-by: honkar, prr

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 539 new commits pushed to the master branch:

54028e7: 8316562: serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java times out after JDK-8314829
7c991cc: 8296246: Update Unicode Data Files to Version 15.1.0
a021dbc: 8316149: Open source several Swing JTree JViewport KeyboardManager tests
455cfae: 8315880: change LockingMode default from LM_LEGACY to LM_LIGHTWEIGHT
9e00949: 8316427: Duplicated code for {obj,type}ArrayKlass::array_klass
c43ebd3: 8315981: Opensource five more random Swing tests
e30e356: 8316461: Fix: make test outputs TEST SUCCESS after unsuccessful exit
242eeae: 8286757: adlc tries to build with /pathmap but without /experimental:deterministic
b275bdd: 8308995: Update Network IO JFR events to be static mirror events
e1870d3: 8316411: compiler/compilercontrol/TestConflictInlineCommands.java fails intermittent with force inline by CompileCommand missing
... and 529 more: https://git.openjdk.org/jdk/compare/6864441163f946d0bec7380a2a120e31b812a6dc...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.

DamonGuy · 2023-09-21T16:40:38Z

/integrate

openjdk · 2023-09-21T16:42:24Z

Going to push as commit 83b01cf.
Since your change was applied there have been 564 commits pushed to the master branch:

542b300: 8315954: getArgumentValues002.java fails on Graal
0637900: 8316433: net.dll should delay load winhttp.dll
90bcdbd: 8316581: Improve performance of Symbol::print_value_on()
8350268: 8316453: [JVMCI] Using Xcomp on jargraal must eagerly initialize JVMCI
3809d69: 8316240: Open source several add/remove MenuBar manual tests
38bf119: 8310228: Improve error reporting for uncaught native exceptions on Windows
349723c: 8315739: Missing null check in os::vm_min_address
8cbe42b: 8316421: libjava should load shell32.dll eagerly
378bcd5: 8316595: Alpine build fails after JDK-8314021
b3d75fe: 8310874: Runthese30m crashes with klass should be in the placeholders during verification
... and 554 more: https://git.openjdk.org/jdk/compare/6864441163f946d0bec7380a2a120e31b812a6dc...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-21T16:42:35Z

@DamonGuy Pushed as commit 83b01cf.

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

Add back key mask and logic for right option. Add new test

7259188

openjdk bot added the rfr Pull request is ready for review label Aug 25, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Aug 25, 2023

Remove duplicate alt key mask. Add value to other mask.

d992ec3

honkar-jdk suggested changes Aug 29, 2023

View reviewed changes

Remove extraneous code

5f82777

honkar-jdk reviewed Aug 29, 2023

View reviewed changes

test/jdk/java/awt/event/KeyEvent/OptionKeyEventTest.java Outdated Show resolved Hide resolved

Move curly brace

d1b3ae5

honkar-jdk approved these changes Aug 29, 2023

View reviewed changes

Modified test for easier use

0aa8d4a

prrace approved these changes Sep 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 20, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 21, 2023

openjdk bot closed this Sep 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8311922: [macOS] right-Option key fails to generate release event #15432

8311922: [macOS] right-Option key fails to generate release event #15432

DamonGuy commented Aug 25, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 25, 2023

openjdk bot commented Aug 25, 2023

mlbridge bot commented Aug 25, 2023 •

edited

Loading

honkar-jdk commented Aug 25, 2023 •

edited

Loading

prsadhuk commented Aug 28, 2023 •

edited

Loading

DamonGuy commented Aug 28, 2023

DamonGuy commented Aug 28, 2023

alisenchung commented Aug 28, 2023

honkar-jdk Aug 29, 2023 •

edited

Loading

DamonGuy Aug 29, 2023

honkar-jdk Aug 29, 2023

DamonGuy commented Aug 29, 2023

honkar-jdk left a comment •

edited

Loading

DamonGuy commented Aug 29, 2023

openjdk bot commented Sep 20, 2023

DamonGuy commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

8311922: [macOS] right-Option key fails to generate release event #15432

8311922: [macOS] right-Option key fails to generate release event #15432

Conversation

DamonGuy commented Aug 25, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 25, 2023

openjdk bot commented Aug 25, 2023

mlbridge bot commented Aug 25, 2023 • edited Loading

Webrevs

honkar-jdk commented Aug 25, 2023 • edited Loading

prsadhuk commented Aug 28, 2023 • edited Loading

DamonGuy commented Aug 28, 2023

DamonGuy commented Aug 28, 2023

alisenchung commented Aug 28, 2023

honkar-jdk Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

DamonGuy Aug 29, 2023

Choose a reason for hiding this comment

honkar-jdk Aug 29, 2023

Choose a reason for hiding this comment

DamonGuy commented Aug 29, 2023

honkar-jdk left a comment • edited Loading

Choose a reason for hiding this comment

DamonGuy commented Aug 29, 2023

openjdk bot commented Sep 20, 2023

DamonGuy commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

DamonGuy commented Aug 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 25, 2023 •

edited

Loading

honkar-jdk commented Aug 25, 2023 •

edited

Loading

prsadhuk commented Aug 28, 2023 •

edited

Loading

honkar-jdk Aug 29, 2023 •

edited

Loading

honkar-jdk left a comment •

edited

Loading