From d25e511fc037c0b7d3e588fd0b4eeada4e6357de Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Thu, 5 Sep 2019 16:46:35 -0700 Subject: [PATCH] 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 --- docs/api/browser-window.md | 7 + shell/browser/api/atom_api_browser_window.cc | 22 +++ shell/browser/api/atom_api_browser_window.h | 2 + shell/browser/api/atom_api_web_contents.cc | 4 + spec-main/events-helpers.ts | 12 +- spec-main/fixtures/chromium/other-window.js | 21 ++ .../fixtures/chromium/visibilitystate.html | 19 ++ spec-main/visibility-state-spec.ts | 186 ++++++++++++++++++ 8 files changed, 269 insertions(+), 4 deletions(-) create mode 100644 spec-main/fixtures/chromium/other-window.js create mode 100644 spec-main/fixtures/chromium/visibilitystate.html create mode 100644 spec-main/visibility-state-spec.ts diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 8a069da09d..952ff4ed7c 100644 --- a/docs/api/browser-window.md +++ b/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. diff --git a/shell/browser/api/atom_api_browser_window.cc b/shell/browser/api/atom_api_browser_window.cc index bf595c1cc6..65f2db49cb 100644 --- a/shell/browser/api/atom_api_browser_window.cc +++ b/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 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()) { diff --git a/shell/browser/api/atom_api_browser_window.h b/shell/browser/api/atom_api_browser_window.h index a979bc55be..a7f64aac71 100644 --- a/shell/browser/api/atom_api_browser_window.h +++ b/shell/browser/api/atom_api_browser_window.h @@ -76,6 +76,8 @@ class BrowserWindow : public TopLevelWindow, void RemoveBrowserView(v8::Local value) override; void ResetBrowserViews() override; void SetVibrancy(v8::Isolate* isolate, v8::Local value) override; + void OnWindowShow() override; + void OnWindowHide() override; // BrowserWindow APIs. void FocusOnWebView(); diff --git a/shell/browser/api/atom_api_web_contents.cc b/shell/browser/api/atom_api_web_contents.cc index 22cf7e9c64..4ef943d71e 100644 --- a/shell/browser/api/atom_api_web_contents.cc +++ b/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 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); } diff --git a/spec-main/events-helpers.ts b/spec-main/events-helpers.ts index 2e5252c098..82f17ab3e5 100644 --- a/spec-main/events-helpers.ts +++ b/spec-main/events-helpers.ts @@ -19,13 +19,13 @@ export const waitForEvent = (target: EventTarget, eventName: string) => { * @param {string} eventName * @return {!Promise} 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(resolve => { + const p = new Promise(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 } diff --git a/spec-main/fixtures/chromium/other-window.js b/spec-main/fixtures/chromium/other-window.js new file mode 100644 index 0000000000..7303f3ea17 --- /dev/null +++ b/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) +}) diff --git a/spec-main/fixtures/chromium/visibilitystate.html b/spec-main/fixtures/chromium/visibilitystate.html new file mode 100644 index 0000000000..4e75cf03ea --- /dev/null +++ b/spec-main/fixtures/chromium/visibilitystate.html @@ -0,0 +1,19 @@ + + + + + + + Document + + + + + \ No newline at end of file diff --git a/spec-main/visibility-state-spec.ts b/spec-main/visibility-state-spec.ts new file mode 100644 index 0000000000..4fec94b1ee --- /dev/null +++ b/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 => { + 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') + }) + }) +})