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

Let Chromium manage `document.visibilityState` and `document.hidden` #9178

Merged
merged 7 commits into from Jun 7, 2017

Conversation

Projects
None yet
3 participants
@poiru
Copy link
Member

poiru commented Apr 12, 2017

Chromium already includes the necessary plumbing to manage the
visibility properties and visibilitychange event so this gets rid of
most of our custom logic for BrowserWindow and BrowserView.

Note that webview remains unchanged and is still affected by the issues
listed below.

User facing changes:

  • The document visibility properties and visibilitychange event are
    now also updated/fired in response to occlusion changes on macOS. In
    other words, document.visibilityState will now be hidden on macOS
    if the window is occluded by another window.

  • Previously, visibilitychange was also fired by both Electron and
    Chromium in some cases (e.g. when hiding the window). Now it is only
    fired by Chromium so you no longer get duplicate events.

  • The visiblity state of BrowserWindows created with { show: false }
    is now initially visible until the window is shown and hidden.

  • The visibility state of BrowserWindows with backgroundThrottling
    disabled is now permanently visible.

This should also fix #6860 (but not for webview).

@poiru poiru requested a review from kevinsawicki Apr 12, 2017

@poiru poiru force-pushed the visibilitystate branch 5 times, most recently from 4c01231 to 3bfa421 Apr 12, 2017

@lukeapage

This comment has been minimized.

Copy link
Contributor

lukeapage commented May 2, 2017

Is there a timeline for this PR? Looking forward to it...

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 2, 2017

Is there a timeline for this PR? Looking forward to it...

@poiru did you mention you were seeing some spec crashes?

@poiru poiru force-pushed the visibilitystate branch 3 times, most recently from 3c5af45 to 7266b06 May 4, 2017

@poiru

This comment has been minimized.

Copy link
Member Author

poiru commented May 4, 2017

@kevinsawicki Fixed the crash so this is ready to be reviewed/merged.

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 17, 2017

@poiru merged latest master and pushed a few changes to the specs to wait for events instead of using timeouts, they worked for me locally but might be flaky on CI so I'll take care of any build failures.

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 17, 2017

I'm seeing a flicker when un-maximizing via clicking the icon from the macOS dock, little tricky to screen capture, but here are two gifs of this branch vs. master:

this branch

hidden-flicker

master

hidden-flicker2

page

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
    <style>
      webview {
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0
      }
    </style>
  </head>
  <body>
    <webview src="https://github.com"></webview>
  </body>
</html>

@poiru poiru force-pushed the visibilitystate branch from 118af95 to 557b441 May 22, 2017

@poiru

This comment has been minimized.

Copy link
Member Author

poiru commented May 22, 2017

@kevinsawicki I went ahead and reverted the webview related changes. In other words, this PR now only applies to BrowserWindow/BrowserView.

FWIW, I also squashed all changes (including your test changes) for easier reverting.

Let Chromium manage `document.visibilityState` and `document.hidden`
Chromium already includes the necessary plumbing to manage the
visibility properties and `visibilitychange` event so this gets rid of
most of our custom logic for `BrowserWindow` and `BrowserView`.

Note that `webview` remains unchanged and is still affected by the issues
listed below.

User facing changes:

- The `document` visibility properties and `visibilitychange` event are
  now also updated/fired in response to occlusion changes on macOS. In
  other words, `document.visibilityState` will now be `hidden` on macOS
  if the window is occluded by another window.

- Previously, `visibilitychange` was also fired by *both* Electron and
  Chromium in some cases (e.g. when hiding the window). Now it is only
  fired by Chromium so you no longer get duplicate events.

- The visiblity state of `BrowserWindow`s created with `{ show: false }`
  is now initially `visible` until the window is shown and hidden.

- The visibility state of `BrowserWindow`s with `backgroundThrottling`
  disabled is now permanently `visible`.

This should also fix #6860 (but not for `webview`).

@kevinsawicki kevinsawicki force-pushed the visibilitystate branch from 557b441 to 7d2226e Jun 6, 2017

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented Jun 7, 2017

Tests are green now, pushed a few updates to them but I think the original cases are still covered.

Thanks for this @poiru 👍 🚀

@kevinsawicki kevinsawicki merged commit 915f6a6 into master Jun 7, 2017

8 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #6879154 succeeded in 79s
Details
electron-linux-ia32 Build #6879155 succeeded in 75s
Details
electron-linux-x64 Build #6879156 succeeded in 192s
Details
electron-mas-x64 Build #4401 succeeded in 11 min
Details
electron-osx-x64 Build #4407 succeeded in 14 min
Details
electron-win-ia32 Build #3377 succeeded in 11 min
Details
electron-win-x64 Build #3354 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the visibilitystate branch Jun 7, 2017

50Wliu added a commit to atom/atom that referenced this pull request Sep 21, 2017

⬇️ electron@1.7.2
Interested in seeing if electron/electron#9178 is the cause of the focus issues

50Wliu added a commit to atom/atom that referenced this pull request Sep 28, 2017

⬇️ electron@1.7.2
Interested in seeing if electron/electron#9178 is the cause of the focus issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment