8360886: Cmd + plus shortcut does not work reliably #1837

beldenfox · 2025-06-30T16:58:00Z

The Mac platform code figures out where characters are on the keyboard as the user types. The character table is updated on every key press by calling a registerKeyEvent: in GlassApplication. This character table is used to resolve KeyCharacterCombination accelerators like Cmd + "+".

On a US English layout when the user types Cmd + "+" on the main keyboard they're actually typing Cmd + "=". There's special handling in macOS for this combination that can cause two NSEvents to be sent and so there's special handling in the Glass code so we don't process both events. When this special case is invoked registerKeyEvent: isn't being called to update the character table. This bug was introduced when code was consolidated in PR #1528.

The fix is a simple one-liner. I've added a test for this in the KeyboardTest.java program. It's an isolated test because it requires the Robot to send events to hold down a modifier while a character key is pressed. I also updated some obsolete comments and tweaked it to test all KeyCharacterCombinations since they should now work reliably on all platforms.

Progress

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

Issue

JDK-8360886: Cmd + plus shortcut does not work reliably (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 9ba898a9)
Michael Strauß (@mstr2 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1837

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1837.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-30T16:58:22Z

👋 Welcome back mfox! 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 · 2025-06-30T16:58:54Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2025-06-30T17:02:05Z

Webrevs

01: Full - Incremental (524b3f43)
00: Full (9ba898a9)

andy-goryachev-oracle · 2025-06-30T17:06:45Z

/reviewers 2

openjdk · 2025-06-30T17:07:29Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2025-06-30T17:10:46Z

tests/manual/events/KeyboardTest.java

+    // Cmd + "+" works as expected on the US English layout on the Mac. The
+    // OS special-cases this combination and so does JavaFX. In any case
+    // Shortcut + "+" is a very common shortcut and deserves a test.
+    private void testShiftedShortcut(Layout layout, Node focusNode, Logging log) {


Question: is it possible to convert this manual test into an automated headful one?

I could write a weaker system test. The problem sequence is CMD + EQUALS. There are two possible outcomes depending on the keyboard layout; either the EQUALS yields nothing or it generates a PRESSED event that matches against KeyCharacterCombination("=", KeyCode.SHORTCUT_DOWN). It's not quite the same test but it would have caught this bug. I can verify that it would work on all the keyboard layouts that Apple ships with macOS 15.

I'm less sure if the test would work cross-platform. It should but I don't have a good way of enumerating and testing all the layouts on Windows and Linux.

I was thinking more in terms of exhaustive checking of all the available keys (so we won't encounter another scenario when a bug is introduced similar to #1528, if that is possible).

I know we can't really control the keyboard layout, so perhaps what I am asking may not be possible since these tests might fail on other layouts.

So let me ask

is the problem limited to cmd+= on the US keyboard?

could we have a different behavior with a different kb layout (German? French?)

what happens when the user attaches an external USB or bluetooth keyboard?

The issue here is limited to cmd+= and cmd+period. These receive special treatment by macOS. To counter-act this they also receive special treatment in the Glass code.

The macOS special handling of cmd+= only applies to certain keyboard layouts (for example, English but not German). But that's not true for Glass, the work-around in glass applies to all layouts.

I will look into writing up a system test. I think I can put one together that tests more keys and works on all layouts. It would be nice not to rely solely on this manual test.

External USB or bluetooth keyboards shouldn't make a difference.

(In case you're curious, Apple wants to make sure you can invoke Cmd + "+" by holding down Cmd and pressing any key that has a "+" symbol printed on it. On a US English layout that means Cmd + "=" should also work. So they send us Cmd + "=" and if that doesn't trigger a shortcut they send us Cmd + "+". Glass tries to ensure that that second event doesn't turn into another KeyEvent. I think this only happens with keys where "+" is Shift + "=" which is why it happens on US English but not German.)

thank you for the detailed explanations!

andy-goryachev-oracle · 2025-06-30T21:33:27Z

SKARA warns about trailing period in the title. I've corrected the JIRA ticket, please fix the PR.

andy-goryachev-oracle · 2025-06-30T21:57:52Z

tests/manual/events/KeyboardTest.java

@@ -38,6 +39,7 @@
 import javafx.scene.control.TextArea;
 import javafx.scene.input.KeyCharacterCombination;
 import javafx.scene.input.KeyCode;
+import javafx.scene.input.KeyCombination;


I wonder if it is possible to add a panel to the test stage with explanations that are currently in the class-level javadoc? Specifically, the fact that you need to configure a particular layout.

I had some work done to simplify the boilerplate in #1747, though it did not make it yet.

andy-goryachev-oracle

The reproducer fails in master and runs correctly with the fix on macOS 15.5 M1.

beldenfox · 2025-06-30T23:04:14Z

SKARA warns about trailing period in the title. I've corrected the JIRA ticket, please fix the PR.

Title updated. Thanks for fixing the ticket.

…ormKeyEquivalent event.

beldenfox · 2025-07-07T16:56:36Z

Well, now I’ve gone and re-written this PR.

As stated earlier sometimes a single key press can lead to multiple calls to performKeyEquivalent. During testing I discovered that the way Glass checks for this doesn’t always work. Currently it tries to guess which key event might lead to a second one but there are cases where it guesses wrong (e.g. the Hebrew layout). I’ve decided to take a different approach, detecting and discarding the second event. The logic is:

Is the new key event not the current event (theEvent != NSApp.currentEvent)?
Are we still working on an earlier event (NSApp.currentEvent == lastKeyEvent)?

If both these conditions are true we can ignore the new key event.

I’ve reverted the changes to the manual test and added a system test. It covers a smattering of carefully selected keys including the combinations involved in this bug. Currently it’s restricted to macOS and Latin keyboards because the Hebrew keyboard on macOS is crazy. I will expand this to other platforms when I have time to do the testing. I’ve run this test on macOS using US English, French, German, Spanish, Hebrew, Devanagari, and a few other layouts.

(About that Hebrew layout. When you hold down Command all the keys start behaving like a US English layout, including the punctuation keys. So the key you press to type a period is entirely different from the key that generates Cmd+period.)

mstr2

Looks good.

beldenfox · 2025-07-14T18:26:34Z

Heads up, I just submitted a PR (#1848) that fixes this bug in an entirely different way. The fix in this PR is much simpler and safer (I would not be afraid to back port it) and so I think it should go forward even if it turns out to be an interim fix. In any case it might take a while for #1848 to get vetted and there's the non-zero chance it will get rejected. But if anyone reviews both you'll see the same area of code affected in GlassView3D.m (the performKeyEquivalent: handling).

Fixed Cmd + "+" accelerator (again)

9ba898a

openjdk bot added the rfr Ready for review label Jun 30, 2025

andy-goryachev-oracle reviewed Jun 30, 2025

View reviewed changes

andy-goryachev-oracle approved these changes Jun 30, 2025

View reviewed changes

beldenfox changed the title ~~8360886: Cmd + plus shortcut does not work reliably.~~ 8360886: Cmd + plus shortcut does not work reliably Jun 30, 2025

Reverted manual tests, added system test, changed check for fake perf…

524b3f4

…ormKeyEquivalent event.

mstr2 approved these changes Jul 14, 2025

View reviewed changes

8360886: Cmd + plus shortcut does not work reliably #1837

Are you sure you want to change the base?

8360886: Cmd + plus shortcut does not work reliably #1837

Uh oh!

Conversation

beldenfox commented Jun 30, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 30, 2025

Uh oh!

openjdk bot commented Jun 30, 2025

Uh oh!

mlbridge bot commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Jun 30, 2025

Uh oh!

openjdk bot commented Jun 30, 2025

Uh oh!

andy-goryachev-oracle Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

beldenfox Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

beldenfox Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Jun 30, 2025

Uh oh!

andy-goryachev-oracle Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Jun 30, 2025

Uh oh!

beldenfox commented Jul 7, 2025

Uh oh!

mstr2 left a comment

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Jul 14, 2025

Uh oh!

Uh oh!

beldenfox commented Jun 30, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 30, 2025 •

edited

Loading