On Thu, 12 Mar 2026 23:48:59 GMT, Jose Pereda <[email protected]> wrote:
> This PR fixes an issue on macOS with a system menu bar, that happens when a > dialog is shown while a menu from the system menu bar is also showing. > > Currently the menubar is removed whenever the stage loses focus, but that > causes a broken state in the menu (as reported in > https://bugs.openjdk.org/browse/JDK-8335541) as the dialog showing prevents > for a clean tear down of the menubar. And even if there were no issues, there > is actually no need of destroying and recreating the same menubar all over > again, since nothing really changed (the dialog doesn't have a menubar on its > own, and shares the very same one from its owner stage). > > A test has been added, that fails before this patch and passes with it. > Another minor test has been included, just to give some details about what > happens when a menu titled exactly `"Help"` is added to the system menu bar. > It works before and after the patch. > > While adding the test, I noticed that there were failures when the menu was > hidden: > > SystemMenuBarHelpMenuTest STANDARD_ERROR > Exception in thread "JavaFX Application Thread" > java.lang.IllegalStateException: Not on FX application thread; currentThread > = JavaFX Application Thread > at > javafx.graphics@27-internal/com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:282) > at > javafx.graphics@27-internal/com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:480) > at > javafx.controls@27-internal/javafx.scene.control.Menu.hide(Menu.java:428) > at > javafx.graphics@27-internal/com.sun.javafx.tk.quantum.GlassMenuEventHandler.handleMenuClosed(GlassMenuEventHandler.java:46) > at > javafx.graphics@27-internal/com.sun.glass.ui.Menu.notifyMenuClosed(Menu.java:196) > > so I wrapped the calls in `Menu::notifyMenuClosed` and > `Menu::notifyMenuClosed` with ` Utils::runOnFxThread`. ## Pull request overview Adds coverage and fixes for macOS system menu bar behavior when the Help menu is open and a modal dialog is shown, ensuring menu open/close notifications are processed safely on the JavaFX thread. **Changes:** - Added a new system robot test that opens the macOS system Help menu, interacts with its items, and verifies behavior across modal dialog show/hide. - Ensured Glass `Menu` open/close callbacks are dispatched on the FX thread. - Adjusted `MenuBarSkin` system menu selection logic to avoid clearing the system menu bar when a stage loses focus (e.g., when an owned dialog takes focus). ### Reviewed changes Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments. | File | Description | | ---- | ----------- | | tests/system/src/test/java/test/robot/javafx/scene/SystemMenuBarHelpMenuTest.java | New macOS system-menu regression test covering Help menu behavior with modal dialogs. | | modules/javafx.graphics/src/main/java/com/sun/glass/ui/Menu.java | Dispatches menu opening/closing notifications onto the FX thread. | | modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java | Prevents system menu bar removal on focus loss to keep owner-stage menu during owned dialogs. | --- 💡 <a href="/openjdk/jfx/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot"; class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>. modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 514: > 512: // The stage lost focus, but we don't need to remove its > menubar now. > 513: // If an owned dialog is shown, the owner stage should keep > it as it was. > 514: return; Indentation is off for the `return;` statement (it appears to have an extra leading space), which breaks the surrounding formatting consistency. modules/javafx.graphics/src/main/java/com/sun/glass/ui/Menu.java line 188: > 186: if (this.eventHandler != null) { > 187: Utils.runOnFxThread(() -> > eventHandler.handleMenuOpening(this, System.nanoTime())); > 188: } notifyMenuOpening/notifyMenuClosed now defer to the FX thread, but the timestamp passed to the handler is being taken when the runnable executes (potentially later), not when the native menu event occurred. Consider capturing System.nanoTime() into a local variable before scheduling to preserve event-time semantics. tests/system/src/test/java/test/robot/javafx/scene/SystemMenuBarHelpMenuTest.java line 98: > 96: /** > 97: * Verifies that after opening the Help menu includes the Spotlight > search field, which is > 98: * included seamlessly by the system. Javadoc sentence is ungrammatical: "Verifies that after opening the Help menu includes..." reads like it's missing a comma/subject (e.g., "after opening, the Help menu includes..."). This makes the test intent harder to parse. tests/system/src/test/java/test/robot/javafx/scene/SystemMenuBarHelpMenuTest.java line 171: > 169: > 170: // Verify the menu remains opened. The Spotlight search field is > also visible > 171: // (thought this can only be verified visually) Typo in comment: "thought this can only be verified visually" should be "though this can only be verified visually". ------------- PR Review: https://git.openjdk.org/jfx/pull/2107#pullrequestreview-3940604639 PR Review Comment: https://git.openjdk.org/jfx/pull/2107#discussion_r2928108482 PR Review Comment: https://git.openjdk.org/jfx/pull/2107#discussion_r2928108456 PR Review Comment: https://git.openjdk.org/jfx/pull/2107#discussion_r2928108402 PR Review Comment: https://git.openjdk.org/jfx/pull/2107#discussion_r2928108428
