From a2ab0d8ebe80909f0504d412ee492d658ca604ad Mon Sep 17 00:00:00 2001 From: Nitish Sakhawalkar Date: Fri, 7 Sep 2018 11:21:58 -0700 Subject: [PATCH] fix: notify focus change right away rather not on next tick (#14453) * fix: Notify focus change right away, not on next tick * fix: emit the JS blur/focus events on next tick to avoid race condition * address feedback from review * fix: bind deferred Emit() calls to a WeakPtr This is so that the deferred Emit() calls will be canceled if the TopLevelWindow is destroyed. * chore: remove wip/test code cruft * fix: make linter happy * Enable disabled tests * refactor: cleaner impl of EmitEventSoon() * Revert "Merge branch 'fix-win-focus' of github.com:electron/electron into fix-win-focus" This reverts commit 90576806eb271d059f0a215c67e9b4b04f7396a4, reversing changes made to 9c13e47779a3af78fe0970c1f3d6cd040a5354e6. * Restore 704722c1, which was removed in error. We apologise again for the fault in the subtitles. Those responsible for sacking the people who have just been sacked have been sacked. --- atom/browser/api/atom_api_top_level_window.cc | 4 ++-- atom/browser/api/atom_api_top_level_window.h | 9 +++++++++ atom/browser/native_window_views.cc | 10 ++++------ spec/api-browser-window-spec.js | 16 +++++----------- spec/api-web-contents-spec.js | 3 +-- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc index c3110099aa6d..557dad7133ae 100644 --- a/atom/browser/api/atom_api_top_level_window.cc +++ b/atom/browser/api/atom_api_top_level_window.cc @@ -163,11 +163,11 @@ void TopLevelWindow::OnWindowEndSession() { } void TopLevelWindow::OnWindowBlur() { - Emit("blur"); + EmitEventSoon("blur"); } void TopLevelWindow::OnWindowFocus() { - Emit("focus"); + EmitEventSoon("focus"); } void TopLevelWindow::OnWindowShow() { diff --git a/atom/browser/api/atom_api_top_level_window.h b/atom/browser/api/atom_api_top_level_window.h index 4ad8ac65f3b8..255bc784f902 100644 --- a/atom/browser/api/atom_api_top_level_window.h +++ b/atom/browser/api/atom_api_top_level_window.h @@ -14,6 +14,7 @@ #include "atom/browser/native_window.h" #include "atom/browser/native_window_observer.h" #include "atom/common/api/atom_api_native_image.h" +#include "content/public/browser/browser_thread.h" #include "native_mate/handle.h" namespace atom { @@ -222,6 +223,14 @@ class TopLevelWindow : public mate::TrackableObject, // Remove this window from parent window's |child_windows_|. void RemoveFromParentChildWindows(); + template + void EmitEventSoon(base::StringPiece eventName) { + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(base::IgnoreResult(&TopLevelWindow::Emit), + weak_factory_.GetWeakPtr(), eventName)); + } + #if defined(OS_WIN) typedef std::map MessageCallbackMap; MessageCallbackMap messages_callback_map_; diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc index 280410b7ba82..9b1d17c05839 100644 --- a/atom/browser/native_window_views.cc +++ b/atom/browser/native_window_views.cc @@ -1109,12 +1109,10 @@ void NativeWindowViews::OnWidgetActivationChanged(views::Widget* changed_widget, if (changed_widget != widget()) return; - // Post the notification to next tick. - content::BrowserThread::PostTask( - content::BrowserThread::UI, FROM_HERE, - base::Bind(active ? &NativeWindow::NotifyWindowFocus - : &NativeWindow::NotifyWindowBlur, - GetWeakPtr())); + if (active) + NativeWindow::NotifyWindowFocus(); + else + NativeWindow::NotifyWindowBlur(); // Hide menu bar when window is blured. if (!active && IsMenuBarAutoHide() && IsMenuBarVisible()) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 17d278ffc7e6..00d49dd78750 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -410,8 +410,7 @@ describe('BrowserWindow module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the test. Passes locally. - xdescribe('BrowserWindow.getFocusedWindow()', (done) => { + describe('BrowserWindow.getFocusedWindow()', (done) => { it('returns the opener window when dev tools window is focused', (done) => { w.show() w.webContents.once('devtools-focused', () => { @@ -728,8 +727,7 @@ describe('BrowserWindow module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the test. Passes locally. - xdescribe('BrowserWindow.alwaysOnTop() resets level on minimize', () => { + describe('BrowserWindow.alwaysOnTop() resets level on minimize', () => { before(function () { if (process.platform !== 'darwin') { this.skip() @@ -1966,8 +1964,7 @@ describe('BrowserWindow module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the tests. They pass locally. - xdescribe('document.visibilityState/hidden', () => { + describe('document.visibilityState/hidden', () => { beforeEach(() => { w.destroy() }) function onVisibilityChange (callback) { @@ -2571,7 +2568,6 @@ describe('BrowserWindow module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the test. It passes locally. describe('kiosk state', () => { before(function () { // Only implemented on macOS. @@ -2619,8 +2615,7 @@ describe('BrowserWindow module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the tests. They pass locally. - xdescribe('fullscreen state', () => { + describe('fullscreen state', () => { before(function () { // Only implemented on macOS. if (process.platform !== 'darwin') { @@ -2768,8 +2763,7 @@ describe('BrowserWindow module', () => { } }) - // TODO(alexeykuzmin): [Ch66] Enable the test. Fails on CI bots, passes locally. - xit('exits HTML fullscreen when window leaves fullscreen', (done) => { + it('exits HTML fullscreen when window leaves fullscreen', (done) => { w.destroy() w = new BrowserWindow() w.webContents.once('did-finish-load', () => { diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index b6fc840708ff..32f40640c4f1 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -363,8 +363,7 @@ describe('webContents module', () => { }) }) - // TODO(alexeykuzmin): [Ch66] Enable the test. Passes locally. - xdescribe('focus()', () => { + describe('focus()', () => { describe('when the web contents is hidden', () => { it('does not blur the focused window', (done) => { ipcMain.once('answer', (event, parentFocused, childFocused) => {