Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8233678: [macos 10.15] System menu bar does not work initially on macOS Catalina #361

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 2, 2020

This is a proposed fix for the bug where the Apple system menubar is initially non-responsive on macOS 10.15 and later after a JavaFX application has started up. The end user can workaround this by switching to some other application and then back to the JavaFX app, but there is no known workaround that the application developer can use.

JavaFX is using a non-standard approach to creating the system menus, and seems likely that some change in macOS 10.15 means that this no longer works the same as in previous versions of macOS. We have had problems with application startup on macOS in the past that affected the system menubar: see JDK-8123430 and JDK-8093743.

The solution is to deactivate and then reactivate the application after the app has initially been made active, but before any window is initialized and before the system menu is populated.

I pushed this as two commits: one with the fix and one with some temporary verbose debug print statements. I will remove the print statements prior to the formal review, but wanted to leave them in for now in case someone wanted to test them, and ran into an issue (the debug print statements could help isolate any problems).

I have tested this on macOS 10.14, 10.15, and 11 (Big Sur). It will need additional testing.

The only drawback I see with this approach is that there can be a very brief flash when launching the JavaFX app from a terminal window as the FX application activates, deactivates, and reactivates.

/reviewers 2


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8233678: [macos 10.15] System menu bar does not work initially on macOS Catalina

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/361/head:pull/361
$ git checkout pull/361

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 2, 2020

👋 Welcome back kcr! 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
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 2, 2020

I note that this does not seem to be a problem for Swing apps. AWT uses the Java Runtime Support (JRS) framework to initialize the application and menubar.

@kevinrushforth kevinrushforth requested review from arapte, johanvos and prrace Dec 2, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 2, 2020

Note to testers: JFXPanel applications deadlock with this fix. I need to check whether glass is being started up for a normal Taskbar application, and not being embedded in an FXCanvas or JFXPanel.

… and non-taskbar apps)
@johanvos
Copy link
Collaborator

@johanvos johanvos commented Dec 3, 2020

I tested the patch, and it works for me (MacOS Catalina, 10.15.7)
I will look into more detail in the code, as I'm slightly worried about not following the Apple conventions. From experience, this often leads to more issues sooner or later.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 3, 2020

Thanks for checking. I'll remove the debug print statements, turn off verbose logging, and send it out for review. I would appreciate a thorough review of the code. Given the way JavaFX initializes the macOS application and sets the system menu bar, this seems like the least intrusive and least risky fix, but it would be nice if we can find a better fix that isn't overly complicated or risky.

@kevinrushforth kevinrushforth marked this pull request as ready for review Dec 3, 2020
@openjdk openjdk bot added the rfr label Dec 3, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2020

Webrevs

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 3, 2020

For some reason, the GitHub actions test results weren't added by the Skara bot. I filed SKARA-839 to track this.

You can click here to see the results of the GitHub action for the most recent commit I pushed.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Dec 3, 2020

I added a few comments. In general, the approach looks ok to me. My comments are about edge cases, for which I'm not sure how we need to handle them.

@kevinrushforth kevinrushforth requested review from aghaisas and removed request for arapte Dec 4, 2020
Copy link
Collaborator

@aghaisas aghaisas left a comment

I tested the test program provided in the JBS on macOS 10.15.4. This PR fixes the reported issue.
The side effect of brief flash when the application is launched is undesirable, but livable given the important issue this PR fixes.

I spotted a typo in code comments.

@kevinrushforth kevinrushforth requested review from aghaisas and johanvos Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

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

8233678: [macos 10.15] System menu bar does not work initially on macOS Catalina

Reviewed-by: jvos, aghaisas

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

  • ca0d3d0: 8256821: TreeViewSkin/Behavior: misbehavior on switching skin
  • 00a8646: 8247576: Labeled/SkinBase: misbehavior on switching skin
  • c8d9c94: 8254049: Update WebView to public suffix list 2020-04-24

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 openjdk bot added the ready label Dec 7, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 7, 2020

/integrate

@openjdk openjdk bot closed this Dec 7, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@kevinrushforth Since your change was applied there have been 3 commits pushed to the master branch:

  • ca0d3d0: 8256821: TreeViewSkin/Behavior: misbehavior on switching skin
  • 00a8646: 8247576: Labeled/SkinBase: misbehavior on switching skin
  • c8d9c94: 8254049: Update WebView to public suffix list 2020-04-24

Your commit was automatically rebased without conflicts.

Pushed as commit e7dbdfc.

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

@kevinrushforth kevinrushforth deleted the kevinrushforth:8233678-macos-menu-bar branch Dec 7, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 7, 2020

Mailing list message from Bruce Johnson on openjfx-dev:

It would be useful, at least to me, to see a brief comment in this email thread about why this approach (which results in the application flashing) is necessary. Why does JavaFX require this approach, or is there an alternative, but more complex approach that could ultimately be used in later releases. I?d just like to be able to look back in the email thread when I (or the users of my software) see the flash to understand why it's there.

Bruce

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 7, 2020

I don't yet know whether there is an alternative that would allow the menubar to work without deactivating the app, but I filed a follow-up bug to track this:

JDK-8257835: Brief flash in terminal window when launching FX app on macOS

It seems like it might be possible to fix this, since AWT initialization doesn't run into this. I note that AWT uses an Apple-provided "JavaRuntime Support" (JRS) Framework. We would need to see whether there is something else possible using public macOS APIs.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 17, 2021

Mailing list message from Tom Schindl on openjfx-dev:

Hi Kevin,

I today saw this tweet by Alex Blewitt [1] in my twitter feed and a
possible fix without the de/reactivation. So I thought I would bring it
up here as well.

Tom

[1] https://twitter.com/alblue/status/1350877893979746305

Am 07.12.20 um 16:58 schrieb Bruce Johnson:

It would be useful, at least to me, to see a brief comment in this email thread about why this approach (which results in the application flashing) is necessary. Why does JavaFX require this approach, or is there an alternative, but more complex approach that could ultimately be used in later releases. I?d just like to be able to look back in the email thread when I (or the users of my software) see the flash to understand why it's there.

Bruce

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Jan 21, 2021

@tomsontom Thanks for letting me know. He reached to me directly as well, so I'll plan to add the information to the follow-on bug, JDK-8257835.

@alblue
Copy link

@alblue alblue commented Jan 23, 2021

BTW there were observations that the setActivationPolicy call needs to be inside the callback as well, at least on 10.15. I've updated the bug on the StackOverflow question, and the Eclipse bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=552063 has a fix about to go in with the suggested change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants