feat: hide menu bar on windows fullscreen #43402
Conversation
There was a problem hiding this comment.
Since this a change in behavior, can we add this under breaking changes similar to this callout? https://github.com/electron/electron/blob/main/docs/breaking-changes.md#behavior-changed-webcontents-property-on-login-on-app
We can mention that this aligns with Linux.
Thanks for the review! Is this ok? |
| menu_bar_visible_before_fullscreen_ = IsMenuBarVisible(); | ||
| SetMenuBarVisibility(false); | ||
| } else { | ||
| // No fullscreen -> fullscreen video -> un-fullscreen video results |
There was a problem hiding this comment.
Is this only strictly true when fullscreening a video or is it anytime html fullscreen is requested? If a video isn't required, maybe we should update this to reflect that.
There was a problem hiding this comment.
I think it's the latter, that's why I called the test "correctly remembers state prior to HTML fullscreen transition". I just used the video example because that's one of the rare occasions where a HTML fullscreen transition can occur organically via user action instead of via JavaScript. I imagine the alternatives would illicit these reactions:
- "HTML fullscreen transition results...": "Don't know what that even is, probably doesn't matter"
- ".requestFullscreen() results...": "I as a developer never call that function, probably doesn't matter"
- "No fullscreen -> fullscreen video -> un-fullscreen video results": "Video elements are common, this covers an important edge case"
There was a problem hiding this comment.
Fair point on calling out the most understood use case.
|
Release Notes Persisted
|
* feat: hide menu bar on windows fullscreen * test: state prior to html fullscreen transition * refactor: restore `#ifdef` for readability Reference: electron#43402 (comment) * docs: menu bar behavior changed --------- Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Description of Change
Made menu bar hidden on fullscreen in Windows.
Resolves #38414
The menu bar previously not being hidden on Windows seems intentional (#6429) which is why I marked this PR as a
feat.Test Fiddle Gist: https://gist.github.com/piotrpdev/2aea2013cfc86484e1b9ba41ee4c2806
Note
If the user performs this series of steps:
The video still takes up the whole window but the menu bar is shown.
I didn't change this because I assume it is the intended behavior.
Checklist
npm testpassesRelease Notes
Notes: Made menu bar hidden on fullscreen in Windows.