Browse Source

fix: ensure document.visibilityState aligns with the visibility of the TopLevelWindow (#20134)

* fix: ensure document.visibilityState aligns with the visibility of the TopLevelWindow

* chore: disable the specs on linux on CI
trop[bot] 5 years ago
parent
commit
d25e511fc0

+ 7 - 0
docs/api/browser-window.md

@@ -51,6 +51,9 @@ This event is usually emitted after the `did-finish-load` event, but for
 pages with many remote resources, it may be emitted before the `did-finish-load`
 event.
 
+Please note that using this event implies that the renderer will be considered "visible" and
+paint even though `show` is false.  This event will never fire if you use `paintWhenInitiallyHidden: false`
+
 ## Setting `backgroundColor`
 
 For a complex app, the `ready-to-show` event could be emitted too late, making
@@ -184,6 +187,7 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
     leave it undefined so the executable's icon will be used.
   * `show` Boolean (optional) - Whether window should be shown when created. Default is
     `true`.
+  * `paintWhenInitiallyHidden` Boolean (optional) - Whether the renderer should be active when `show` is `false` and it has just been created.  In order for `document.visibilityState` to work correctly on first load with `show: false` you should set this to `false`.  Setting this to `false` will cause the `ready-to-show` event to not fire.  Default is `true`.
   * `frame` Boolean (optional) - Specify `false` to create a
     [Frameless Window](frameless-window.md). Default is `true`.
   * `parent` BrowserWindow (optional) - Specify parent window. Default is `null`.
@@ -485,6 +489,9 @@ Emitted when the window is hidden.
 Emitted when the web page has been rendered (while not being shown) and window can be displayed without
 a visual flash.
 
+Please note that using this event implies that the renderer will be considered "visible" and
+paint even though `show` is false.  This event will never fire if you use `paintWhenInitiallyHidden: false`
+
 #### Event: 'maximize'
 
 Emitted when window is maximized.

+ 22 - 0
shell/browser/api/atom_api_browser_window.cc

@@ -9,6 +9,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"  // nogncheck
 #include "content/browser/renderer_host/render_widget_host_owner_delegate.h"  // nogncheck
+#include "content/browser/web_contents/web_contents_impl.h"  // nogncheck
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "gin/converter.h"
@@ -44,10 +45,21 @@ BrowserWindow::BrowserWindow(v8::Isolate* isolate,
   if (options.Get(options::kBackgroundColor, &value))
     web_preferences.Set(options::kBackgroundColor, value);
 
+  // Copy the transparent setting to webContents
   v8::Local<v8::Value> transparent;
   if (options.Get("transparent", &transparent))
     web_preferences.Set("transparent", transparent);
 
+  // Copy the show setting to webContents, but only if we don't want to paint
+  // when initially hidden
+  bool paint_when_initially_hidden = true;
+  options.Get("paintWhenInitiallyHidden", &paint_when_initially_hidden);
+  if (!paint_when_initially_hidden) {
+    bool show = true;
+    options.Get(options::kShow, &show);
+    web_preferences.Set(options::kShow, show);
+  }
+
   if (options.Get("webContents", &web_contents) && !web_contents.IsEmpty()) {
     // Set webPreferences from options if using an existing webContents.
     // These preferences will be used when the webContent launches new
@@ -422,6 +434,16 @@ void BrowserWindow::Cleanup() {
   Observe(nullptr);
 }
 
+void BrowserWindow::OnWindowShow() {
+  web_contents()->WasShown();
+  TopLevelWindow::OnWindowShow();
+}
+
+void BrowserWindow::OnWindowHide() {
+  web_contents()->WasHidden();
+  TopLevelWindow::OnWindowHide();
+}
+
 // static
 mate::WrappableBase* BrowserWindow::New(mate::Arguments* args) {
   if (!Browser::Get()->is_ready()) {

+ 2 - 0
shell/browser/api/atom_api_browser_window.h

@@ -76,6 +76,8 @@ class BrowserWindow : public TopLevelWindow,
   void RemoveBrowserView(v8::Local<v8::Value> value) override;
   void ResetBrowserViews() override;
   void SetVibrancy(v8::Isolate* isolate, v8::Local<v8::Value> value) override;
+  void OnWindowShow() override;
+  void OnWindowHide() override;
 
   // BrowserWindow APIs.
   void FocusOnWebView();

+ 4 - 0
shell/browser/api/atom_api_web_contents.cc

@@ -364,6 +364,9 @@ WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options)
   // Whether to enable DevTools.
   options.Get("devTools", &enable_devtools_);
 
+  bool initially_shown = true;
+  options.Get(options::kShow, &initially_shown);
+
   // Obtain the session.
   std::string partition;
   mate::Handle<api::Session> session;
@@ -418,6 +421,7 @@ WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options)
 #endif
   } else {
     content::WebContents::CreateParams params(session->browser_context());
+    params.initially_hidden = !initially_shown;
     web_contents = content::WebContents::Create(params);
   }
 

+ 8 - 4
spec-main/events-helpers.ts

@@ -19,13 +19,13 @@ export const waitForEvent = (target: EventTarget, eventName: string) => {
  * @param {string} eventName
  * @return {!Promise<!Array>} With Event as the first item.
  */
-export const emittedOnce = (emitter: NodeJS.EventEmitter, eventName: string) => {
-  return emittedNTimes(emitter, eventName, 1).then(([result]) => result)
+export const emittedOnce = (emitter: NodeJS.EventEmitter, eventName: string, trigger?: () => void) => {
+  return emittedNTimes(emitter, eventName, 1, trigger).then(([result]) => result)
 }
 
-export const emittedNTimes = (emitter: NodeJS.EventEmitter, eventName: string, times: number) => {
+export const emittedNTimes = async (emitter: NodeJS.EventEmitter, eventName: string, times: number, trigger?: () => void) => {
   const events: any[][] = []
-  return new Promise<any[][]>(resolve => {
+  const p = new Promise<any[][]>(resolve => {
     const handler = (...args: any[]) => {
       events.push(args)
       if (events.length === times) {
@@ -35,4 +35,8 @@ export const emittedNTimes = (emitter: NodeJS.EventEmitter, eventName: string, t
     }
     emitter.on(eventName, handler)
   })
+  if (trigger) {
+    await Promise.resolve(trigger())
+  }
+  return p
 }

+ 21 - 0
spec-main/fixtures/chromium/other-window.js

@@ -0,0 +1,21 @@
+const { app, BrowserWindow } = require('electron')
+
+const ints = (...args) => args.map(a => parseInt(a, 10))
+
+const [x, y, width, height] = ints(...process.argv.slice(2))
+
+let w
+
+app.whenReady().then(() => {
+  w = new BrowserWindow({
+    x,
+    y,
+    width,
+    height
+  })
+  console.log('__ready__')
+})
+
+process.on('SIGTERM', () => {
+  process.exit(0)
+})

+ 19 - 0
spec-main/fixtures/chromium/visibilitystate.html

@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="UTF-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1.0">
+  <meta http-equiv="X-UA-Compatible" content="ie=edge">
+  <title>Document</title>
+</head>
+<body>
+  <script>
+    require('electron').ipcRenderer.on('get-visibility-state', (event) => {
+      event.sender.send('visibility-state', document.visibilityState)
+    })
+    document.addEventListener('visibilitychange', () => {
+      require('electron').ipcRenderer.send('visibility-change')
+    })
+  </script>
+</body>
+</html>

+ 186 - 0
spec-main/visibility-state-spec.ts

@@ -0,0 +1,186 @@
+import { expect } from 'chai'
+import * as cp from 'child_process';
+import { BrowserWindow, BrowserWindowConstructorOptions, ipcMain } from 'electron'
+import * as path from 'path'
+
+import { emittedOnce } from './events-helpers'
+import { closeWindow } from './window-helpers';
+import { ifdescribe } from './spec-helpers';
+
+// visibilityState specs pass on linux with a real window manager but on CI
+// the environment does not let these specs pass
+ifdescribe(process.platform !== 'linux' || !isCI)('document.visibilityState', () => {
+  let w: BrowserWindow
+
+  afterEach(() => {
+    return closeWindow(w)
+  })
+
+  const load = () => w.loadFile(path.resolve(__dirname, 'fixtures', 'chromium', 'visibilitystate.html'))
+
+  const itWithOptions = (name: string, options: BrowserWindowConstructorOptions, fn: Mocha.Func) => {
+    return it(name, async function (...args) {
+      w = new BrowserWindow({
+        ...options,
+        paintWhenInitiallyHidden: false,
+        webPreferences: {
+          ...(options.webPreferences || {}),
+          nodeIntegration: true
+        }
+      })
+      await Promise.resolve(fn.apply(this, args))
+    })
+  }
+
+  const getVisibilityState = async (): Promise<string> => {
+    const response = emittedOnce(ipcMain, 'visibility-state')
+    w.webContents.send('get-visibility-state')
+    return (await response)[1]
+  }
+
+  itWithOptions('should be visible when the window is initially shown by default', {}, async () => {
+    await load()
+    const state = await getVisibilityState()
+    expect(state).to.equal('visible')
+  })
+
+  itWithOptions('should be visible when the window is initially shown', {
+    show: true,
+  }, async () => {
+    await load()
+    const state = await getVisibilityState()
+    expect(state).to.equal('visible')
+  })
+
+  itWithOptions('should be hidden when the window is initially hidden', {
+    show: false,
+  }, async () => {
+    await load()
+    const state = await getVisibilityState()
+    expect(state).to.equal('hidden')
+  })
+
+  itWithOptions('should be visible when the window is initially hidden but shown before the page is loaded', {
+    show: false,
+  }, async () => {
+    w.show()
+    await load()
+    const state = await getVisibilityState()
+    expect(state).to.equal('visible')
+  })
+
+  itWithOptions('should be hidden when the window is initially shown but hidden before the page is loaded', {
+    show: true,
+  }, async () => {
+    // TODO(MarshallOfSound): Figure out if we can work around this 1 tick issue for users
+    if (process.platform === 'darwin') {
+      // Wait for a tick, the window being "shown" takes 1 tick on macOS
+      await new Promise(r => setTimeout(r, 0))
+    }
+    w.hide()
+    await load()
+    const state = await getVisibilityState()
+    expect(state).to.equal('hidden')
+  })
+
+  itWithOptions('should be toggle between visible and hidden as the window is hidden and shown', {}, async () => {
+    await load()
+    expect(await getVisibilityState()).to.equal('visible')
+    await emittedOnce(ipcMain, 'visibility-change', () => w.hide())
+    expect(await getVisibilityState()).to.equal('hidden')
+    await emittedOnce(ipcMain, 'visibility-change', () => w.show())
+    expect(await getVisibilityState()).to.equal('visible')
+  })
+
+  itWithOptions('should become hidden when a window is minimized', {}, async () => {
+    await load()
+    expect(await getVisibilityState()).to.equal('visible')
+    await emittedOnce(ipcMain, 'visibility-change', () => w.minimize())
+    expect(await getVisibilityState()).to.equal('hidden')
+  })
+
+  itWithOptions('should become visible when a window is restored', {}, async () => {
+    await load()
+    expect(await getVisibilityState()).to.equal('visible')
+    await emittedOnce(ipcMain, 'visibility-change', () => w.minimize())
+    await emittedOnce(ipcMain, 'visibility-change', () => w.restore())
+    expect(await getVisibilityState()).to.equal('visible')
+  })
+
+  describe('on platforms that support occlusion detection', () => {
+    let child: cp.ChildProcess
+
+    before(function() {
+      if (process.platform !== 'darwin') this.skip()
+    })
+
+    const makeOtherWindow = (opts: { x: number; y: number; width: number; height: number; }) => {
+      child = cp.spawn(process.execPath, [path.resolve(__dirname, 'fixtures', 'chromium', 'other-window.js'), `${opts.x}`, `${opts.y}`, `${opts.width}`, `${opts.height}`])
+      return new Promise(resolve => {
+        child.stdout!.on('data', (chunk) => {
+          if (chunk.toString().includes('__ready__')) resolve()
+        })
+      })
+    }
+
+    afterEach(() => {
+      if (child && !child.killed) {
+        child.kill('SIGTERM')
+      }
+    })
+
+    itWithOptions('should be visible when two windows are on screen', {
+      x: 0,
+      y: 0,
+      width: 200,
+      height: 200,
+    }, async () => {
+      await makeOtherWindow({
+        x: 200,
+        y: 0,
+        width: 200,
+        height: 200,
+      })
+      await load()
+      const state = await getVisibilityState()
+      expect(state).to.equal('visible')
+    })
+
+    itWithOptions('should be visible when two windows are on screen that overlap partially', {
+      x: 50,
+      y: 50,
+      width: 150,
+      height: 150,
+    }, async () => {
+      await makeOtherWindow({
+        x: 100,
+        y: 0,
+        width: 200,
+        height: 200,
+      })
+      await load()
+      const state = await getVisibilityState()
+      expect(state).to.equal('visible')
+    })
+
+    itWithOptions('should be hidden when a second window completely conceals the current window', {
+      x: 50,
+      y: 50,
+      width: 50,
+      height: 50,
+    }, async function () {
+      this.timeout(240000)
+      await load()
+      await emittedOnce(ipcMain, 'visibility-change', async () => {
+        await makeOtherWindow({
+          x: 0,
+          y: 0,
+          width: 300,
+          height: 300,
+        })
+      })
+      const state = await getVisibilityState()
+      expect(state).to.equal('hidden')
+    })
+  })
+})