8359108: Mac - When Swing starts First, native application menu doesn't work for JavaFX

[pabulaner]

This pull request fixes the system menu bar on MacOS when combining windows of Swing and JavaFX.

Behavior before

If for some reason You needed to initialize AWT before JavaFX and You wanted to install the system menu bar from the JavaFX side, this wasn't possible. This issue is persistent even when You didn't open a Swing window.

One scenario where this kind of issue happens is when You use install4j (see https://www.ej-technologies.com/install4j). In this case AWT is initialized by install4j and therefore You can't use the JavaFX system menu bar anymore.

Behavior after

The fix allows JavaFX to install a system menu bar even if it is initialized after AWT. This is achieved by only changing code inside JavaFX. Each JavaFX window stores the previously installed menu bar when gaining focus and will restore this menu bar if the focus was lost. This only happens if the system menu bar installed by the JavaFX window is still unchanged.

Tests

This PR introduces tests for the system menu bar in addition to verifying its own behavior / changes. The tests include single- and multi-window tests while interacting with Swing. The tests ensure that the menu bar stays the same for each window, no matter how You switch focus between them.

Additional benifits

This fix is not specifically for AWT, but allows JavaFX to interact much more compatibly with other frameworks that make use of the system menu bar.

Review from AWT

In the previous PR related to this one, the comment was made that the folks from AWT should take a look at this fix. It would be great and much appreciated if someone could initiate it.

Add disable flag?

We could also add a flag to prevent JavaFX from installing a system menu bar for users who have found other fixes for their projects / setups. This could be used to restore the previous behavior when AWT is initialized first.

Co-Author: @FlorianKirmaier

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issue

• JDK-8359108: Mac - When Swing starts First, native application menu doesn't work for JavaFX (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1904

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back pabulaner! 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[bot]]

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

[mlbridge[bot]]

Webrevs

• 05: Full - Incremental (c5b5af07)
• 04: Full (358f670d)
• 03: Full - Incremental (7e6e9a6d)
• 02: Full - Incremental (33bc0e2a)
• 01: Full - Incremental (6afc5b5b)
• 00: Full (4b687692)

[kevinrushforth]

This approach seems much better than the previous in that it is activated only when a JavaFX Stage becomes the focused (key) window, and it saves / restores the menu such that whenever an different (e.g., AWT) window is focused, the AWT menu will be installed.

In addition to reviewing and checking that it only touches the menu when it should, testing will be critical.

Reviewers: @kevinrushforth @andy-goryachev-oracle @beldenfox Cc: @prrace @honkar-jdk

/reviewers 3

[kevinrushforth]

As mentioned above, testing will be key here. This needs to work reliably in the case of a multi-window app when the application is hidden and restored, and when switching between the windows of the app. The automated tests should already cover some of the cases. There may be others to consider.

Here are a few cases that I can think of to check:

• Pure FX app: 2 FX Stages (AWT toolkit not initialized at all) -- the fix should do nothing
• 2 FX Stages with FX initialized first (AWT is initialized last, but no AWT/Swing windows) -- the fix should do nothing
• 2 FX Stages with AWT initialized first (but no AWT/Swing windows) -- the fix should install the FX window when either Stage is focused (there will not be any AWT menu)
• a mix of AWT Windows and FX Stages with FX initialized first -- FX will install the menu and keep it there, since AWT won't install a menu when it doesn't own the NSApplication
• a mix of AWT Windows and FX Stages with FX initialized first -- AWT menu will be installed when an AWT window is active, FX menu will be installed when an FX Stage is active
• 2 AWT Stages one of which has a JFXPanel with an FX scene, initialize AWT first -- fix should be a noop
• 2 AWT Stages one of which has a JFXPanel with an FX scene, initialize FX first -- fix should be a noop

I presume that the automated tests will cover some of these, but we might want additional tests.

[openjdk[bot]]

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

[pabulaner]

Thanks for the test case ideas, my tests cover the AWT initialized first tests, but implementing the other tests You proposed won't be that difficult, as it is just copying the code and moving and deleting some lines. I will get to add those tests probably until tomorrow.

[pabulaner]

So I added most of the tests You proposed, except for the last three.

For the last two I just need some more time to implement them.

For the other test with AWT being initialized last and still installing the menu is something not directly implemented in this fix. I think AWT won't install a system menu if it is running embedded and therefore this would need its own fix inside AWT itself (but I don't know exactly).

[pabulaner]

I have now also added the JFXPanel test, but only for the first of the two tests involving the JFXPanel (so where AWT is initialized first). In the other case AWT won't install a menu bar, since JavaFX was initialized first, so nothing will happen.

[beldenfox]

I’ve been reviewing how the platform code interacts with the rest of the system. For others who want to take a look at this PR here’s a quick run-down of how the current master branch works.

When JavaFX is not embedded the toolkit builds a single NSMenu to use as the system menu. It installs that NSMenu as NSApp.mainMenu when the first top-level window is shown but before it becomes the key window. This happens inside the _setMenubar call in GlassWindow.m. That menu stays in place as NSApp.mainMenu for the lifetime of the JavaFX application. Subsequent calls to _setMenubar and the logic that sets NSApp.mainMenu inside windowDidBecomeKey: are basically no-ops since it’s always the same NSMenu and it has already been installed.

It might look like the system is setting a separate menubar for each window but it’s not. There’s only one, it stays in place, and the toolkit rebuilds its content as windows gain and lose focus. And it always contains at least one menu, namely the default application menu.

The system also calls _setMenubar to hand the system NSMenu to each PopupWindow when it’s first shown. There doesn’t seem to be any point to this; the NSMenu was already installed by the popup’s owner and MenuBarSkin ignores popups when updating the system menu bar.

When JavaFX is embedded none of the above happens.

This PR doesn’t seem to change any behavior in the non-embedded case. In the embedded case everything changes. JavaFX now builds a system-wide NSMenu and hands it off to each top-level window but it only gets installed when that window becomes key. The menu is un-installed when the window resigns key (and the previous mainMenu is restored). That’s all new behavior but it seems to be working reliably.

Also new with this PR: if a popup is displayed in an FXPanel the system will call _setMenubar to set the system menu. The platform code is handling this correctly (basically ignoring it) but the checks it’s making shouldn’t be necessary. It would be far better if createTKPopupStage stopped calling _setMenubar.

This PR is turning on some code further up in the system that can lead to some minor bugs. For example, if you add a MenuBar to an FXPanel’s scene and then turn on useSystemMenuBar the entire menubar will become inaccessible. This is similar to another bug that’s already present in the master branch; if you add a MenuBar to a PopupWindow and then turn on useSystemMenuBar the entire menubar will disappear. These issues are all contrived and it’s easy for developers to avoid them but it would be good to tighten up the code a bit. The bugs I’ve found so far are minor enough not to block this PR and could be addressed in a separate issue. This should be easy to clean up though I can’t personally provide further assistance until November.

(The primary problem is in MenuBarSkin.setSystemMenu. It crawls up the stage ownership chain to decide which MenuBar to use to populate the system menu bar. It should ignore windows that aren’t eligible for the system menu bar like embedded and popup stages. One possible solution is to call getMenubar on each stage to see if the toolkit initialized it with the system menu.)

Still no show-stoppers, just some unnecessary complications from the way PopupWindows keep trying to force the system menu onto the platform code and a few very minor bugs that no developer is likely to notice.

[pabulaner]

Thanks for the detailed feedback, much appreciated. So as far as I understood Your feedback it means that the PR from Your side would be good to go, but maybe in the future some minor bug / issue fixes would be nice to have?

[pabulaner]

Thanks for the improvements. I should have implemented all of them, please take a look again.

[beldenfox]

Thanks for the detailed feedback, much appreciated. So as far as I understood Your feedback it means that the PR from Your side would be good to go, but maybe in the future some minor bug / issue fixes would be nice to have?

I need to do a more detailed review of the platform code but for now I haven't seen any problems with it. To review the changes to, say, _setMenubar I needed to first understand when it is called and that's what I was focusing on.

There is an unexpected testing burden. When a popup appears in an FXPanel (as a tooltip, combo box, or menu) the core code will try to set the system menu bar for no good reason and it's up to the platform code to make sure it doesn't succeed. I'm not sure it warrants a system test but some testing needs to be done for these cases. This core behavior really should be addressed in a follow up issue but that's not something I can do until November.

[kevinrushforth]

I note that your branch is very out of date. Please merge the latest master into your branch. There is a minor merge conflict in modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Overrides.m that you will need to resolve. Leave the commented out NSLog (which is present in the latest master and conflicts with your change) as the first line of the function.

[pabulaner]

I have rebased to the master and ran the tests again on another MacBook, but for me everything seems to work. Could You maybe provide me with some more information what exactly doesn't work with the tests for You?

[openjdk[bot]]

@pabulaner Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[kevinrushforth]

I have rebased to the master

To echo what the Skara said: next time, please merge master instead of rebasing to master.. Rebasing makes reviewing new change since last review difficult at best.

for me everything seems to work. Could You maybe provide me with some more information what exactly doesn't work with the tests for You?

Sure. I'll provide the logs.

[kevinrushforth]

Here are the assertion failures for the two failing tests:

MacOSSystemMenuMultiWindowFXOnlySwingLast > test() FAILED
    org.opentest4j.AssertionFailedError: Menu size is different: 5 != 2 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
        at app//test.com.sun.glass.ui.mac.MacOSSystemMenuTestBase.compareMenus(MacOSSystemMenuTestBase.java:350)
        at app//test.com.sun.glass.ui.mac.MacOSSystemMenuMultiWindowFXOnlySwingLast.test(MacOSSystemMenuMultiWindowFXOnlySwingLast.java:70)

MacOSSystemMenuMultiWindowTest > test() FAILED
    org.opentest4j.AssertionFailedError: Menu size is different: 5 != 2 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
        at app//test.com.sun.glass.ui.mac.MacOSSystemMenuTestBase.compareMenus(MacOSSystemMenuTestBase.java:350)
        at app//test.com.sun.glass.ui.mac.MacOSSystemMenuMultiWindowTest.test(MacOSSystemMenuMultiWindowTest.java:69)

I instrumented the code and it is picking up the menus of the Terminal app from which I ran gradle test. If I click on another app (e.g., the Finder) after starting gradle but before the test actually starts then the test passes.

I fired off a headful test on our Jenkins system and none of the tests pass. They all timeout or fail with an error from osascript.

All of this leads me to think that it might be better to convert these tests to manual test. Instead of using osascript, you could have each window show a TextArea with a list of what the top level menu items should be when you click in the window. What are your thoughts on this?

[pabulaner]

As long as manual tests are considered a good practice in such cases, I would be fine with it. This would of course mean that the test cases won't automatically detect if something doesn't work anymore. Otherwise I see no problem with manual tests.

[pabulaner]

So I have fixed Your minor merge issues and have converted the tests to manual tests. Maybe You can take a look and give me some feedback on what might need to be changed, since I don't know exactly what requirements manual tests need to satisfy.

PS: Sorry for the rebasing, I am used to this workflow from my work, but I will use merge in the future.

[kevinrushforth]

As long as manual tests are considered a good practice in such cases, I would be fine with it. This would of course mean that the test cases won't automatically detect if something doesn't work anymore. Otherwise I see no problem with manual tests.

Exactly. Automated tests are preferred where feasible. This seems like a case where they aren't quite (which is too bad, since I liked the idea of your automated tests).

Manual tests seem fine in this case. I'll review and test them. Thanks.

[pabulaner]

Hey, are there any news on this PR?

[FlorianKirmaier]

If we can't run the Tests on the CI, i think we should at least keep them - because they are really valuable for every person working on this topic in the future.

Anything left for us to do?

[kevinrushforth]

This is on my list to review this week. Sorry for the delay.

[pabulaner]

So are there any updates?

[kevinrushforth]

Not yet, but it's still on my list.