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

fix: simpleFullscreen window should be on top #14881

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@lynaghk
Copy link
Contributor

commented Sep 29, 2018

First let me say that Electron is great and has empowered me to build fun + useful desktop software that I wouldn't have otherwise been able to create.
So thanks for your hard work and stewardship of the project --- if any of y'all catch me in person at a conference, etc., drinks are on me!

Description of Change

If an app has no menu bar (because app.dock.hide() has been called),
OS X will still render the menu bar of the previously-focused app.

This commit ensures simpleFullscreen windows will be drawn on top of
that menu bar by setting their level to NSPopUpMenuWindowLevel while
simpleFullscreen mode is active.

Ref: #11468

This fix is motivated by Finda, a keyboard-only app that needs to popup full screen on any OS X space without showing up in the Dock or with a menu bar.
The video on that page demonstrates this problem "in the wild" (note how Finda doesn't cover the QuickTime Player OS X menu bar when it pops up.)

Here's a reduced video showing before/after the fix:

simplefullscreen-no-dock-fix

Before fix the no_dock.js still shows iTerm's menu bar; after the fix the no_dock.js is actually fullscreen.
Included dock.js to show that this fix doesn't break menu bar when dock isn't hidden.

no_dock.js:

const electron = require('electron')
const app = electron.app
const BrowserWindow = electron.BrowserWindow

app.dock.hide()
app.on('ready', function(){
  new BrowserWindow({simpleFullscreen: true, fullscreen: true})
})

(dock.js is the same, but without the app.dock.hide() line.)

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines
Release Notes

Notes: Fixed simpleFullscreen windows in hidden-dock apps from being drawn below OS X menu bar of previously-focused app.

@lynaghk lynaghk requested a review from as a code owner Sep 29, 2018

@welcome

This comment has been minimized.

Copy link

commented Sep 29, 2018

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codebytere
Copy link
Member

left a comment

this looks good, thanks for adding this!

@codebytere codebytere changed the title fix: simpleFullscreen window should be on top of other OS X menu bars. fix: simpleFullscreen window should be on top Oct 4, 2018

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@lynaghk would you mind rebasing this on master?
Once that happens we should be good to merge! just want to get CI green 😁

@lynaghk lynaghk force-pushed the lynaghk:fix-simple-fullscreen-no-dock branch from 0f84954 to fa19dda Oct 4, 2018

@lynaghk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

No prob. Rebase was clean, I'll keep my fingers crossed for CI. Thanks for shepherding this through!

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Hi @lynaghk!
We currently have some issues with linux tests for PRs from forks, it will be resolved in #14984.
That's the reason why "linux-...-tests" jobs are show as failed for your PR.

@lynaghk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Thanks for letting me know, @alexeykuzmin. I suspected it didn't have anything to do with my changes, so I wasn't taking the CI failures personally =P

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

@lynaghk can you please rebase your branch onto the latest electron:master?
It is required to run CI builds on linux.

@lynaghk lynaghk force-pushed the lynaghk:fix-simple-fullscreen-no-dock branch from fa19dda to 2a08e91 Oct 7, 2018

@lynaghk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

@alexeykuzmin Done. Rebase was clean. Thanks.

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@lynaghk one last rebase to bring in #15026, then we'll finally be good!

fix: simpleFullscreen window should be on top of other OS X menu bars.
If an app has no menu bar (because `app.dock.hide()` has been called),
OS X will still render the menu bar of the previously-focused app.

This commit ensures simpleFullscreen windows will be drawn on top of
that menu bar by setting their level to NSPopUpMenuWindowLevel while
simpleFullscreen mode is active.

Ref: #11468

@lynaghk lynaghk force-pushed the lynaghk:fix-simple-fullscreen-no-dock branch from 2a08e91 to b2bae08 Oct 9, 2018

@lynaghk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Done! Thanks for the reminders.

I'll pay additional tribute to the CI gods next harvest so that this doesn't happen again =P

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

thanks! Our CI is a wild west right now owing to a recent build system change from gyp to GN so we appreciate the patience

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Failure is

not ok 715 webContents module getWebPreferences() API should not crash when called for devTools webContents

which i believe to be unrelated

@codebytere codebytere merged commit af4cf1e into electron:master Oct 10, 2018

21 of 24 checks passed

Backportable? - 2-0-x Backport Failed
Details
ci/circleci: osx-testing-tests Your tests failed on CircleCI
Details
Backportable? - 3-0-x
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: mas-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
electron-lint Build #20181009.41 succeeded
Details
release-notes Release notes found
@welcome

This comment has been minimized.

Copy link

commented Oct 10, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk

This comment has been minimized.

Copy link

commented Oct 10, 2018

Release Notes Persisted

Fixed simpleFullscreen windows in hidden-dock apps from being drawn below OS X menu bar of previously-focused app.

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@lynaghk would you mind opening up backport PRs targeting 2-0-x and 3-0-x?

@lynaghk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.