Skip to content

core: respect visible menuBarVisibility for fullscreen #11119

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.

Sign up for GitHub

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

Jump to bottom
Merged
vince-fugnitto merged 1 commit into from
May 25, 2022
Merged

core: respect visible menuBarVisibility for fullscreen #11119

vince-fugnitto merged 1 commit into from
May 25, 2022

Conversation

@vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented May 9, 2022 •
edited
Loading

What it does

Fixes #10077.
Closes #10213.

The pull-request updates the handling of "toggle full screen" (electron) to properly respect the preference window.menuBarVisibility. If the preference is set to visible then the menu should remain visible on fullscreen for both the custom and native main-menus. The change also includes a listener to properly update the main-menu when in full-screen.

How to test

toggling behavior:

  1. set the preference window.titleBarStyle to native
  2. for each value in the preference window.menuBarVisibility, confirm the behavior
Style Visibility Expectation
native classic toggling full-screen (F11) should hide the main menu
native hidden toggling full-screen (F11) should hide the main menu
native compact toggling full-screen (F11) should hide the main menu
native visible toggling full-screen (F11) should show the main menu
custom classic toggling full-screen (F11) should hide the main menu
custom hidden toggling full-screen (F11) should hide the main menu
custom compact toggling full-screen (F11) should hide the main menu
custom visible toggling full-screen (F11) should show the main menu

The behavior of each option should be similar to what is experienced in vscode.

full-screen behavior:

  1. when in full-screen, confirm for both custom and native titlebar styles that:
    • updating menuBarVisibility will properly update the visibility
    • setting the visibility to visible should display the menu in full-screen

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label May 9, 2022
colin-grant-work
colin-grant-work reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the combination of custom title bar and classic menu bar visibility give an unexpected result.

Preference Expectation Behavior
window.titleBarStyle = native && window.menuBarVisibility = classic toggling full-screen (F11) should hide the main menu Hidden
window.titleBarStyle = native && window.menuBarVisibility = hidden toggling full-screen (F11) should hide the main menu Hidden
window.titleBarStyle = native && window.menuBarVisibility = compact toggling full-screen (F11) should hide the main menu Compact
window.titleBarStyle = native && window.menuBarVisibility = visible toggling full-screen (F11) should show the main menu Visible
window.titleBarStyle = custom && window.menuBarVisibility = classic toggling full-screen (F11) should hide the main menu Visible ❌
window.titleBarStyle = custom && window.menuBarVisibility = hidden toggling full-screen (F11) should hide the main menu Hidden
window.titleBarStyle = custom && window.menuBarVisibility = compact toggling full-screen (F11) should hide the main menu Compact
window.titleBarStyle = custom && window.menuBarVisibility = visible toggling full-screen (F11) should show the main menu Visible

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented May 13, 2022

@colin-grant-work I've updated the pull-request so that it properly handles both native and custom titlebar styles (previous iteration only properly handled native). I also added logic to properly update the visibility when in fullscreen by listening to the menuBarVisibility preference update and applying the same handling.

@colin-grant-work colin-grant-work self-requested a review May 13, 2022 14:03
@vince-fugnitto
core: respect visible menu for fullscreen
a4c8f2e 
The commit updates the handling of `toggle full screen` to respect
`menuBarVisibility` properly. If the `menuBarVisibility` is set to
`visible` the menu should be visible when the app is in fullscreen.

Signed-off-by: vince-fugnitto <[email protected]>
colin-grant-work
colin-grant-work approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 16 combinations of fullscreen, title bar style, and menu bar visibility now behave as expected 👍

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented May 16, 2022

@msujew I just wanted to know if you had any feedback given you touched the menus in the past :)

@msujew msujew self-requested a review May 16, 2022 19:16
@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented May 24, 2022

I'll try to merge tomorrow if there are no objections.

msujew
msujew approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look reasonable to me!

@vince-fugnitto vince-fugnitto merged commit f94732e into master May 25, 2022
@vince-fugnitto vince-fugnitto deleted the vf/menu-fullscreen branch May 25, 2022 16:02
@github-actions github-actions bot added this to the 1.26.0 milestone May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msujew msujew msujew approved these changes

@colin-grant-work colin-grant-work colin-grant-work approved these changes

Assignees

No one assigned

Labels

electron issues related to the electron target

Projects

None yet

Milestone

1.26.0

Development

Successfully merging this pull request may close these issues.

Menu is not shown when Electron on Linux is switched to full-screen mode

4 participants

@vince-fugnitto @msujew @colin-grant-work