8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts #1848

beldenfox · 2025-07-14T17:39:50Z

The Mac platform code sends a KeyEvent into the scene graph and if the event is not consumed it gets sent on to the system menu. But ComboBox and Spinner always consume the original event and fire a copy which the system menu ignores.

In the past the platform code sent key events to the system menu even if the scene graph consumed them. This caused various bugs and was fixed in PR #1528 leading to this issue.

One could argue that a ComboBox or Spinner shouldn’t consume all key events but one could also argue that the system menu shouldn’t behave so differently from a standard MenuBar which will respond to any KeyEvent that reaches the top-level Scene no matter where it came from.

This PR installs an event dispatcher which forwards KEY_PRESSED events on to the platform so any event bubbling through the dispatch chain can trigger the system menu. The dispatcher is placed by the top-level (non-popup) Window such that it’s the last dispatcher encountered while bubbling.

In this PR once the key event reaches the GlassSystemMenu it passes the JavaFX key code and modifiers into the Mac platform code. This isn’t enough information to construct an NSEvent to pass to the main menu. Instead the code uses the code and modifiers to verify that the originating key down NSEvent (which it retained) should be sent on to NSApp.mainMenu.

(There are other ways this could work. GlassSystemMenu could take the KeyEvent and perform its own accelerator matching entirely inside Java. This would match the way the standard MenuBar finds accelerators instead of using Apple’s matching algorithm. This PR is the more conservative approach, basically just shifting the timing of system menu matching without changing how it’s done.)

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-8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1848

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…tform code

…before sending it to NSApp.mainMenu. Various bits of cleanup and correct handling of the popup redirector.

…to sysmenuinplatform

bridgekeeper · 2025-07-14T17:40:30Z

👋 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-07-14T17:41:32Z

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

openjdk · 2025-07-14T17:41:50Z

⚠️ @beldenfox This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

mlbridge · 2025-07-14T17:45:24Z

Webrevs

01: Full - Incremental (411701c6)
00: Full (f5b05059)

kevinrushforth · 2025-07-14T17:47:59Z

/reviewers 2

openjdk · 2025-07-14T17:48:22Z

@kevinrushforth
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).

hjohn · 2025-07-14T19:09:53Z

modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java

@@ -901,7 +901,6 @@ WindowEventDispatcher createInternalEventDispatcher() {
        return new WindowEventDispatcher(new PopupEventRedirector(this),
                                         new WindowCloseRequestHandler(this),
                                         new EventHandlerManager(this));
-


You should remove this unrelated change.

hjohn · 2025-07-14T19:12:21Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/delegate/MenuBarDelegate.java

@@ -31,4 +31,8 @@ public interface MenuBarDelegate {
    // removes a submenu at {@code pos} which delegate is {@code menu} parameter
    public boolean remove(MenuDelegate menu, int pos);
    public long getNativeMenu();
+    // Returns true if the key event was processed
+    default public boolean handleKeyEvent(int code, int modifiers) {


This should be public default or just default as everything is always public in an interface anyway.

hjohn

Java part looks like a simple enough change. I can't comment on the native code.

hjohn · 2025-07-14T19:14:53Z

modules/javafx.graphics/src/main/java/com/sun/javafx/stage/WindowEventDispatcher.java

+            if (supported == SupportedState.TRUE && event.getEventType() == KeyEvent.KEY_PRESSED && (event instanceof KeyEvent)) {
+               Toolkit.getToolkit().getSystemMenu().handleKeyEvent((KeyEvent)event);
+            }


Suggested change

if (supported == SupportedState.TRUE && event.getEventType() == KeyEvent.KEY_PRESSED && (event instanceof KeyEvent)) {

Toolkit.getToolkit().getSystemMenu().handleKeyEvent((KeyEvent)event);

}

if (supported == SupportedState.TRUE && event.getEventType() == KeyEvent.KEY_PRESSED && event instanceof KeyEvent ke) {

Toolkit.getToolkit().getSystemMenu().handleKeyEvent(ke);

}

Or perhaps the instanceof is even superfluous, as you already verify it is a KEY_PRESSED event.

hjohn · 2025-07-14T19:26:28Z

One could argue that a ComboBox or Spinner shouldn’t consume all key events

They definitely shouldn't, but I think that's how the poor man's focus/event delegation is currently implemented in these controls.

andy-goryachev-oracle · 2025-07-15T17:59:00Z

On a surface, this looks like a rather intrusive change (I can't comment on the merits of this PR yet).

Would it make sense to attempt fixing event consumption bugs in the ComboBox and Spinner first?
And perhaps also look at fixing the dispatching of consumed events such as JDK-8303209 or JDK-8229467 (the latter has been by @mstr2 , I think prematurely)?

beldenfox · 2025-07-15T20:21:15Z

Would it make sense to attempt fixing event consumption bugs in the ComboBox and Spinner first?

When a Spinner is the focus owner it receives all KeyEvents but KeyEvents still need to be delivered to its editor. There's no good way of doing that now short of consuming the original event and firing a copy. To fix this we would have to introduce new API's (like some form of PR #1632).

And perhaps also look at fixing the dispatching of consumed events such as JDK-8303209 or JDK-8229467 (the latter has been by @mstr2 , I think prematurely)?

Those bugs have to do with how consumption is signaled which isn't relevant. All that's relevant is that the original event is being consumed, not how we find out about it.

JavaFX handles events using dispatch chains and currently the system menu doesn't participate in that machinery. This causes it to work very differently from a standard MenuBar (which places its accelerators in the Scene which is in the chain). One could argue that it's a bug that the system menu doesn't respond to KeyEvents the way a standard MenuBar would.

Another way to look at it: do we want to outlaw what ComboBox and Spinner are doing? It's not illegal to create a KeyEvent and fire it at a control. It's not illegal to consume a KeyEvent that originated in Glass. Things only fail when the system menu is in effect and, again, only because it doesn't participate in dispatch.

Because I agree with you, this PR is intrusive. I'm not fan and I wrote it. I think the system menu needs to participate in event dispatch and I haven't found any clean, tidy way of doing that.

andy-goryachev-oracle · 2025-07-15T21:13:54Z

To fix this we would have to introduce new API's (like some form of PR #1632).

I am not a fan of #1632, I think it's an attempt to band-aid over a fundamental design issue, but this is a different topic altogether.

Why would a Spinner or ComboBox consume the key events that it is not supposed to? Ok, let's assume the outer control (Spinner or ComboBox) gets the event and fires a copy to the inner control (TextField). This is perfectly legal, but now it must either consume the event if the secondary event is consumed. More importantly, it should not consume the event if the secondary event is not consumed, isn't that the issue?

So it it a responsibility of the entity that creates the secondary event to either consume the original event or let it go, or am I mistaken?

hjohn · 2025-07-16T01:40:54Z

Another way to look at it: do we want to outlaw what ComboBox and Spinner are doing? It's not illegal to create a KeyEvent and fire it at a control. It's not illegal to consume a KeyEvent that originated in Glass. Things only fail when the system menu is in effect and, again, only because it doesn't participate in dispatch.

Good point, it's not and shouldn't be strictly illegal. I only commented earlier that I think they shouldn't be doing it but currently have little recourse.

It should be perfectly fine to create your own KeyEvent or MouseEvent, and IMHO such events should be able to do anything that system created events can do, including firing system menus. That ship may have sailed though as I think there's already data in these events that users can't create or replicate.

mstr2 · 2025-07-16T02:47:21Z

Another way to look at it: do we want to outlaw what ComboBox and Spinner are doing? It's not illegal to create a KeyEvent and fire it at a control. It's not illegal to consume a KeyEvent that originated in Glass. Things only fail when the system menu is in effect and, again, only because it doesn't participate in dispatch.

A control can't just fire an event "to the inner control" using the usual Event.fireEvent(). These events are routed through the scene graph starting from the root, so an outside observer will see two key events, not one. There's no way of knowing that the two events are not actually two separate user inputs.

I don't know what "outlaw" means in this case. Throw an exception when user code tries to fire input events? No, probably not. But in my view we should consider it as a control bug, not as a legitimate use of the event system.

beldenfox · 2025-07-16T16:37:03Z

But in my view we should consider it as a control bug, not as a legitimate use of the event system.

If we decided to fix this in controls and discard this PR as unnecessary I would not shed any tears. I knew this would be clunky but felt we at least needed to get a look at a fix like this. I also wanted this in hand since the timeline for fixing this in controls is uncertain.

beldenfox · 2025-07-16T16:42:17Z

So it it a responsibility of the entity that creates the secondary event to either consume the original event or let it go, or am I mistaken?

This would mean two unconsumed KeyEvents would bubble all the way up to the scene. This could trigger the same MenuItem twice even though the user only pressed the accelerator once.

beldenfox added 6 commits June 30, 2025 08:43

System menu accelerators triggered in scene graph but resolved in pla…

ea9a487

…tform code

Merge remote-tracking branch 'upstream/master' into sysmenuinplatform

8b8125b

Cleanup and fixed a popup redirection bug.

4f4cbe8

Validating the system menu key event against the originating NSEvent …

65266cb

…before sending it to NSApp.mainMenu. Various bits of cleanup and correct handling of the popup redirector.

Merge branch 'sysmenuinplatform' of ssh://github.com/beldenfox/jfx in…

d3ec923

…to sysmenuinplatform

Merge remote-tracking branch 'upstream/master' into sysmenuinplatform

f5b0505

openjdk bot added the rfr Ready for review label Jul 14, 2025

beldenfox mentioned this pull request Jul 14, 2025

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

Open

3 tasks

hjohn reviewed Jul 14, 2025

View reviewed changes

Review feedback related to code consistency and clarity.

411701c

8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts #1848

Are you sure you want to change the base?

8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts #1848

Uh oh!

Conversation

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

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Jul 14, 2025

Uh oh!

openjdk bot commented Jul 14, 2025

Uh oh!

openjdk bot commented Jul 14, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Jul 14, 2025

Uh oh!

openjdk bot commented Jul 14, 2025

Uh oh!

hjohn Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

hjohn Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

hjohn left a comment

Choose a reason for hiding this comment

Uh oh!

hjohn Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

hjohn commented Jul 14, 2025

Uh oh!

andy-goryachev-oracle commented Jul 15, 2025

Uh oh!

beldenfox commented Jul 15, 2025

Uh oh!

andy-goryachev-oracle commented Jul 15, 2025

Uh oh!

hjohn commented Jul 16, 2025

Uh oh!

mstr2 commented Jul 16, 2025

Uh oh!

beldenfox commented Jul 16, 2025

Uh oh!

beldenfox commented Jul 16, 2025

Uh oh!

Uh oh!

beldenfox commented Jul 14, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 14, 2025 •

edited

Loading