8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts by beldenfox · Pull Request #2166 · openjdk/jfx

beldenfox · 2026-05-12T17:18:35Z

This PR alters the way ComboBox and Spinner deliver KeyEvents to their TextField editors. When a ComboBox or Spinner is the focus owner it is the target of all key events. Currently the skin installs a filter to catch key events and re-fire most of them at the TextEdit. The skin copies the event, fires the copy at the TextField, and then consumes the original event. This confuses the system menu bar logic on macOS; only the original event can trigger a menu item and that event is always being consumed.

In this PR only the original key event makes its way up and down the event dispatch chain. To drive the TextField the skin delivers the event copy directly to the TextField's event dispatcher and only consumes the original event if the TextField consumes the copy.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

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)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Michael Strauß (@mstr2 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2166

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-05-12T17:19:29Z

👋 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 · 2026-05-12T17:20:37Z

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

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

Reviewed-by: angorya, mstrauss

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

1718958: 8388174: Change JavaFX release version to 28
e221ae9: 8339270: GHA: build on Linux / aarch64
dbaf948: 8387626: java.lang.NullPointerException: Cannot invoke "com.sun.glass.ui.Window.isMinimized()" because "this.platformWindow" is null
... and 37 more: https://git.openjdk.org/jfx/compare/cd7b559799df64fe7001403474df555766b71055...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 · 2026-05-12T17:23:21Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

mlbridge · 2026-05-12T17:27:15Z

Webrevs

andy-goryachev-oracle

I like this idea very much (will do more testing).

Do you think these tickets are related or similar:

https://bugs.openjdk.org/browse/JDK-8174991
https://bugs.openjdk.org/browse/JDK-8229914
https://bugs.openjdk.org/browse/JDK-8088897
https://bugs.openjdk.org/browse/JDK-8234247
https://bugs.openjdk.org/browse/JDK-8337246
https://bugs.openjdk.org/browse/JDK-8320557

Also, I wonder if other virtualized controls with embedded editors such as ListView, Tree|TableView might benefit from this fix?

andy-goryachev-oracle · 2026-05-13T17:51:27Z

+        var dispatcher = textField.getEventDispatcher();
+        if (dispatcher == null) return false;
+
+        EventDispatchChain chain = new EventDispatchChainImpl();


would it make sense to create a public API to dispatch an event specifically to the given Node, bypassing the normal path (as in Event.fireEvent ?)

something along the lines EventUtil.fireEvent ?

would it make sense to create a public API to dispatch an event specifically to the given Node, bypassing the normal path (as in Event.fireEvent ?)

I wouldn't add such a specialized API. It would make more sense to provide an API so clients can easily create event dispatch chains. The EventDispatchChain interface is public but the implementation is not and there's no supported way for clients to create one. With one of those in hand it's easy to pass an event to a single dispatcher or even a set of dispatchers.

Maybe I wasn't clear. You have two methods - this and an identical one in
https://github.com/openjdk/jfx/pull/2166/changes#diff-9117771ea1db4c49f8f0e82634427f7d8205b1f15038c1429959a003a10a5af7R250

that dispatch an event to a component, bypassing the normal chain, correct?

Why not make it a public API? Event.dispatchTo(Node) or something like that?

Or, at a minimum, replace these two with a utility method added to EventUtil ?

Why not make it a public API? Event.dispatchTo(Node) or something like that?

Dispatching to a single node is a very specialized case and I don't think it's worth adding a public routine. We would get more mileage out of adding a public API to create an EventDispatchChain. Then clients could write their own version of this short routine or build more complicated chains that dispatch across multiple nodes.

Or, at a minimum, replace these two with a utility method added to EventUtil ?

This routine is only used in two places and I'm hoping it stays that way. The right location for this routine is a common superclass of ComboBox and Spinner (something like ForwardingControl) but that doesn't exist and I don't know how to add it after the fact. As it stands Spinner doesn't handle input method events or requests correctly and to fix that we'll have to copy code verbatim from ComboBox. New controls that need to do this sort of forwarding should start with a common superclass and it should probably pursue a different design.

I suspect it may not be a specialized case (any composite control might want to dispatch an event to its child control for example), and you already have two places with the duplicate code.

I am ok with this not being a public API, so perhaps it belongs in EventUtils. Certainly not in the ComboBox base class. I don't quite understand the reason for your objection here.

andy-goryachev-oracle · 2026-05-13T17:53:23Z

    }
+
+    // Ensure initial shortcut event is not consumed
+    @Test public void testShortcutNotConsumed() {


I'd strongly encourage to place @test annotation on its own line in all the new code...

beldenfox · 2026-05-13T20:09:23Z

I like this idea very much (will do more testing).

Do you think these tickets are related or similar:

I looked into several of these and even prototyped some fixes. The problems with ESCAPE and ENTER being consumed are at a higher level. For example, in some case the InputMaps are auto-consuming ESCAPE when it should be consumed conditionally.

This PR deals with the way ComboBox and Spinner re-fire events. Some of the bug reports mention event re-firing but I think those might be obsolete. There's comments in the code suggesting TextEdits used to re-fire ESCAPE at their parents but I don't think that's happening anymore.

Also, I wonder if other virtualized controls with embedded editors such as ListView, Tree|TableView might benefit from this fix?

I'm not familiar with those controls but as far as I know they don't re-fire KeyEvents to drive their editors. I think they set focus directly on their editors when editing in which case this fix isn't relevant. Some quick testing seems to confirm this; they process shortcuts like Cmd+Q just fine.

andy-goryachev-oracle · 2026-05-15T18:47:29Z

+        if (dispatcher == null) return false;
+
+        EventDispatchChain chain = new EventDispatchChainImpl();
+        chain.append(textField.getEventDispatcher());


you already have the dispatcher instance in L185

andy-goryachev-oracle · 2026-05-15T18:48:08Z

+        if (dispatcher == null) return false;
+
+        EventDispatchChain chain = new EventDispatchChainImpl();
+        chain.append(textField.getEventDispatcher());


replace with the dispatcher pointer?

andy-goryachev-oracle · 2026-05-15T18:53:10Z

+        var dispatcher = textField.getEventDispatcher();
+        if (dispatcher == null) return false;
+
+        EventDispatchChain chain = new EventDispatchChainImpl();


Maybe I wasn't clear. You have two methods - this and an identical one in
https://github.com/openjdk/jfx/pull/2166/changes#diff-9117771ea1db4c49f8f0e82634427f7d8205b1f15038c1429959a003a10a5af7R250

that dispatch an event to a component, bypassing the normal chain, correct?

Why not make it a public API? Event.dispatchTo(Node) or something like that?

Or, at a minimum, replace these two with a utility method added to EventUtil ?

andy-goryachev-oracle · 2026-05-15T21:49:57Z

You probably want to sync up with the latest master. This should be a rule for all long-lived PRs (which is pretty much all of them, sorry about that!)

andy-goryachev-oracle · 2026-05-18T20:14:05Z

        return targetDispatchChain.dispatchEvent(event);
    }
+
+    public static Event dispatch(Event event, EventDispatcher dispatcher) {


would it make sense to pass the Node instead of the dispatcher, and return a boolean?

EventUtil is in the javafx.base module and doesn't have access to Node.

Dispatching an event returns an event (possibly null) and all other routines in EventUtil return that event. It strikes me as artificial to only check whether the event was consumed or not.

This is a utility method to dispatch an Event to a Node, right? So returning a boolean which is then passed on makes sense. The Node.getEventDispatcher() is a public API.

Perhaps I don't quite understand your objection?

EventUtil is in the javafx.base module and can't import anything from javafx.graphics, including Node.

I've moved the utility routine to the skin Utils class.

andy-goryachev-oracle · 2026-05-18T20:41:59Z

                ke.consume();
            } else if (textField != null) {
-                textField.fireEvent(ke);
+                forwardToTextField(ke);


Should the ke event be consumed if its copy was consumed?

My policy is to preserve the old logic as much as possible. There's a bunch of tricky code related to handling ENTER and ESCAPE that I don't want to disturb.

In this case the event is KEY_RELEASED for ENTER. I'm pretty sure the TextField will ignore it and it will never be consumed anyway.

So if the application installs a listener on this textField and consumes the event for the purposes of stopping the dispatch, the original event will continue to propagate, unconsumed?

I view this as a bad decision on the part of fx in general, but then again, it's a larger discussion that should happen elsewhere.

In this case, the argument about preserving the existing logic wins.

andy-goryachev-oracle · 2026-05-18T20:43:53Z

    }

+    // Returns 'true' if the event was consumed.
+    private boolean forwardToTextField(KeyEvent event) {


this method seems unnecessary, why not fold it into EventUtil.dispatch() including copying and and returning a boolean?

and perhaps consuming the original event?

andy-goryachev-oracle · 2026-05-19T15:21:55Z

+
+    // Dispatches the event to the Node's dispatcher and returns
+    // true if the event was consumed.
+    public static boolean dispatchToNode(Event event, Node node) {


thank you!
last thing (sorry!) - would it make sense to also describe in the comment that this event bypasses the usual chain, or words to this effect?

andy-goryachev-oracle

Looks good, thank you!

mstr2 · 2026-05-20T15:21:17Z

While this PR seems to fix an actual bug, it also runs the risk of preempting the discussion on focus delegation / multi-level focus that we've had several times, but never reached a conclusion. I wouldn't want this proposed solution (directly dispatching the event to its final target) to be the last word on the topic of focus delegation, especially when it comes to a potential API-level solution.

andy-goryachev-oracle · 2026-05-20T15:27:20Z

@mstr2 : Unless you can bring up a scenario where the proposed solution breaks things, we should probably keep these two things separate.

Once we reach the agreement, it is very likely that changes will be needed everywhere anyway. I don't want to get stuck in endless discussions that lead nowhere, I would rather see the platform progressing.

Do you have a possible failure mode?

mstr2 · 2026-05-20T15:42:14Z

@mstr2 : Unless you can bring up a scenario where the proposed solution breaks things, we should probably keep these two things separate.

Once we reach the agreement, it is very likely that changes will be needed everywhere anyway. I don't want to get stuck in endless discussions that lead nowhere, I would rather see the platform progressing.

Do you have a possible failure mode?

This depends on the definition of "failure mode". With this solution, an event listener that is installed on a node in the skin subgraph that eventually contains the TextField will not see the event (because it does not travel through the subgraph, it is dispatched directly to the final target). This is different from how events in the scene graph usually work.

It's a small inconsistency, and it may be completely acceptable. What I'm saying is that I don't want a narrow-scoped solution as the one presented here to sneak into FX and then just by being there become the de-facto solution for focus-delegation-like problems even when we never conclusively decided how we want to solve the bigger picture.

andy-goryachev-oracle · 2026-05-20T16:26:57Z

Right, I completely agree!
I don't think this method imposes any new standard, we are still free to explore all the alternatives.

beldenfox · 2026-05-20T17:53:55Z

This depends on the definition of "failure mode". With this solution, an event listener that is installed on a node in the skin subgraph that eventually contains the TextField will not see the event (because it does not travel through the subgraph, it is dispatched directly to the final target). This is different from how events in the scene graph usually work.

I thought about this but only in the context of the default skin (I doubt we want to encourage clients to attach event listeners to random parts of the skin we create). I forgot that clients can create their own skins and may want to attach filters and handlers. I could alter the code so the dispatch chain covers the entire skin.

Just to be clear I don't like the solution in this PR. I began prototyping some cleaner approaches but was stymied by the control/skin/behavior split which is particularly murky with ComboBox and Spinner. These skins are also playing games with the way they handle ENTER and ESCAPE which would also need to be re-worked.

Here's a question: if you install a KeyEvent filter on a scene and the user is typing in a ComboBox would you prefer the target of the key event to be the ComboBox or the TextField? In the master branch you would see two events, one targeted at the ComboBox and the other at the TextField. If you had to choose just one which would it be?

This is a general question about how key event forwarding should work. If the answer is ComboBox then this crude solution will be enough until something better comes along. If the answer is TextField then we should probably scrap this PR rather than set a bad precedent.

andy-goryachev-oracle · 2026-05-20T18:47:17Z

These are very good questions!

if you install a KeyEvent filter on a scene and the user is typing in a ComboBox would you prefer the target of the key event to be the ComboBox or the TextField?

This is why I don't like the Event.target at all. In my opinion (and I understand I am late to the party and the things are unlikely to change) there should only be one event, delivered to its target (which, in the case of a KeyEvent, a current focus owner which is the TextField). If someone adds a listener or a filter somewhere in the hierarchy - fine.

There should be no copying of the event objects for the sole purpose of changing its target. That's how it works in AWT and Swing. FX chose a different path of complexity, event clones, ignoring the consumed flag, and happy dispatching of already consumed events. Which means we'll have much fun dealing with the consequences.

mstr2 · 2026-05-20T19:08:21Z

I thought about this but only in the context of the default skin (I doubt we want to encourage clients to attach event listeners to random parts of the skin we create). I forgot that clients can create their own skins and may want to attach filters and handlers. I could alter the code so the dispatch chain covers the entire skin.

Just to be clear I don't like the solution in this PR. I began prototyping some cleaner approaches but was stymied by the control/skin/behavior split which is particularly murky with ComboBox and Spinner. These skins are also playing games with the way they handle ENTER and ESCAPE which would also need to be re-worked.

For a bugfix, I'm generally okay with any of the possible approaches: 1) do nothing, 2) send the event directly to the target, or 3) dispatch it through the skin subgraph. Since I don't like any of the solutions, I have no clear preference for or against any of them.

Maran23 · 2026-05-21T22:55:15Z

While this PR seems to fix an actual bug, it also runs the risk of preempting the discussion on focus delegation / multi-level focus that we've had several times, but never reached a conclusion. I wouldn't want this proposed solution (directly dispatching the event to its final target) to be the last word on the topic of focus delegation, especially when it comes to a potential API-level solution.

Same for me, +1. But I think we all agree here on this.

For a bugfix, I'm generally okay with any of the possible approaches: 1) do nothing, 2) send the event directly to the target, or 3) dispatch it through the skin subgraph. Since I don't like any of the solutions, I have no clear preference for or against any of them.

Also +1. This solutions is at least IMO a bit better than the previous one.
Once again, as I don't have a Mac, I might not be able to test this the way I want to - but still watching this discussion and code.

beldenfox · 2026-06-08T15:31:28Z

For a bugfix, I'm generally okay with any of the possible approaches: 1) do nothing, 2) send the event directly to the target, or 3) dispatch it through the skin subgraph.

The TextField of a ComboBox or Spinner is a direct child so option 2 and 3 are the same. This assumption is baked into the code in a few places including the FakeFocusTextField.

I wouldn't want this proposed solution (directly dispatching the event to its final target) to be the last word on the topic of focus delegation, especially when it comes to a potential API-level solution.

I've entered draft PR #2181 which sketches out an alternative solution for focus delegation. It shows how a control class can delegate events as a drop-in solution; it doesn't require updating Scene or Node in any way to work.

bridgekeeper · 2026-07-06T19:04:08Z

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

beldenfox · 2026-07-07T14:02:40Z

/touch

openjdk · 2026-07-07T14:03:43Z

@beldenfox The pull request is being re-evaluated and the inactivity timeout has been reset.

beldenfox · 2026-07-13T18:14:13Z

Is there anything more I need to do to get this PR approved? Beyond creating a comprehensive focus-delegation framework from scratch?

I'm not in a hurry to get this into JavaFX 27. These controls have all sorts of oddball behavior and bugs and developers have had to resort to some interesting and fragile workarounds. Based on what I'm seeing in the bug database it looks like any time we touch ComboBox or Spinner we break some client's filtering routine and I'd prefer to deal with that at the start of a release cycle rather than the end.

andy-goryachev-oracle · 2026-07-13T18:46:30Z

Is there anything more I need to do to get this PR approved?

You need two approvals, and possibly some general consensus. In this case, @mstr2 voiced some concerns re: general purpose focus framework. Generally, I don't think the desire for better solution should preempt small incremental improvements (since it risks completely stalling the progress).

As @mstr2 correctly pointed out, some of the PRs linger for a very long time, often one approval short of integration. If everyone would review one PR before submitting their own, things might go differently, but alas. Any ideas?

mstr2

I'll approve with the condition that no one is allowed to bring this up as "it's fixed, why bother" when we'll have our next annual discussion on a general-purpose focus/event delegation mechanism.

kevinrushforth · 2026-07-18T18:47:04Z

@beldenfox This is ready for you to integrate.

beldenfox · 2026-07-19T21:35:31Z

/integrate

openjdk · 2026-07-19T21:37:09Z

Going to push as commit 62b62e6.
Since your change was applied there have been 40 commits pushed to the master branch:

1718958: 8388174: Change JavaFX release version to 28
e221ae9: 8339270: GHA: build on Linux / aarch64
dbaf948: 8387626: java.lang.NullPointerException: Cannot invoke "com.sun.glass.ui.Window.isMinimized()" because "this.platformWindow" is null
... and 37 more: https://git.openjdk.org/jfx/compare/cd7b559799df64fe7001403474df555766b71055...master

Your commit was automatically rebased without conflicts.

openjdk · 2026-07-19T21:37:18Z

@beldenfox Pushed as commit 62b62e6.

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

beldenfox added 2 commits May 12, 2026 09:40

New approach to forwarding key events to ComboBox and Spinner editors

1efb16b

Only run tests on Mac

75af2a5

openjdk Bot added the rfr Ready for review label May 12, 2026

andy-goryachev-oracle reviewed May 13, 2026

View reviewed changes

andy-goryachev-oracle reviewed May 15, 2026

View reviewed changes

Clean up based on review feedback

2cf952e

beldenfox added 2 commits May 18, 2026 10:17

Moved event dispatch to utility method in EventUtil.

b9e4963

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

2730c38

andy-goryachev-oracle reviewed May 18, 2026

View reviewed changes

Moved event dispatching utility to skin Utils

d4c5312

andy-goryachev-oracle reviewed May 19, 2026

View reviewed changes

Better comments for dispatching utility

78d8ebe

andy-goryachev-oracle approved these changes May 19, 2026

View reviewed changes

mstr2 approved these changes Jul 13, 2026

View reviewed changes

openjdk Bot added the ready Ready to be integrated label Jul 13, 2026

openjdk Bot added the integrated Pull request has been integrated label Jul 19, 2026

openjdk Bot closed this Jul 19, 2026

openjdk Bot removed ready Ready to be integrated rfr Ready for review labels Jul 19, 2026

Uh oh!

Conversation

beldenfox commented May 12, 2026 • 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 May 12, 2026

Uh oh!

openjdk Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented May 12, 2026

Uh oh!

mlbridge Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beldenfox commented May 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

mstr2 commented May 20, 2026

Uh oh!

andy-goryachev-oracle commented May 20, 2026

Uh oh!

mstr2 commented May 20, 2026

Uh oh!

andy-goryachev-oracle commented May 20, 2026

Uh oh!

beldenfox commented May 20, 2026

Uh oh!

andy-goryachev-oracle commented May 20, 2026

Uh oh!

mstr2 commented May 20, 2026

Uh oh!

beldenfox commented May 12, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented May 12, 2026 •

edited

Loading

mlbridge Bot commented May 12, 2026 •

edited

Loading