On Mon, 14 Jul 2025 17:39:50 GMT, Martin Fox <m...@openjdk.org> wrote:
> 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.)
Java part looks like a simple enough change. I can't comment on the native
code.
> 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.
modules/javafx.graphics/src/main/java/com/sun/javafx/stage/WindowEventDispatcher.java
line 67:
> 65: if (supported == SupportedState.TRUE && event.getEventType()
> == KeyEvent.KEY_PRESSED && (event instanceof KeyEvent)) {
> 66:
> Toolkit.getToolkit().getSystemMenu().handleKeyEvent((KeyEvent)event);
> 67: }
Suggestion:
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.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1848#pullrequestreview-3017481535
PR Comment: https://git.openjdk.org/jfx/pull/1848#issuecomment-3070707798
PR Review Comment: https://git.openjdk.org/jfx/pull/1848#discussion_r2205630127
