From 809be7b893eacb93c36f87cd16e9f4441d6eea62 Mon Sep 17 00:00:00 2001 From: Robo Date: Wed, 19 May 2021 02:27:35 -0700 Subject: [PATCH] fix: adjust initial webContents focus calculation (#29204) * fix: adjust initial webContents focus calculation * fix: active window check on mac * fix: about:blank focus behavior * chore: add spec Co-authored-by: Raymond Zhao --- lib/browser/api/browser-window.ts | 14 -------- .../api/electron_api_browser_window.cc | 1 + .../browser/api/electron_api_web_contents.cc | 32 ++++++++++++++----- shell/browser/api/electron_api_web_contents.h | 5 ++- shell/browser/native_window.h | 4 +++ shell/browser/native_window_mac.h | 3 ++ shell/browser/native_window_mac.mm | 10 ++++++ spec-main/api-web-contents-spec.ts | 2 +- spec-main/chromium-spec.ts | 26 ++++++++++++++- typings/internal-electron.d.ts | 1 - 10 files changed, 70 insertions(+), 28 deletions(-) diff --git a/lib/browser/api/browser-window.ts b/lib/browser/api/browser-window.ts index d01be9c795f0f..62630bfffb2f5 100644 --- a/lib/browser/api/browser-window.ts +++ b/lib/browser/api/browser-window.ts @@ -20,20 +20,6 @@ BrowserWindow.prototype._init = function (this: BWT) { nativeSetBounds.call(this, bounds, ...opts); }; - // Sometimes the webContents doesn't get focus when window is shown, so we - // have to force focusing on webContents in this case. The safest way is to - // focus it when we first start to load URL, if we do it earlier it won't - // have effect, if we do it later we might move focus in the page. - // - // Though this hack is only needed on macOS when the app is launched from - // Finder, we still do it on all platforms in case of other bugs we don't - // know. - if (this.webContents._initiallyShown) { - this.webContents.once('load-url' as any, function (this: WebContents) { - this.focus(); - }); - } - // Redirect focus/blur event to app instance too. this.on('blur', (event: Event) => { app.emit('browser-window-blur', event, this); diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index aef0fefcce290..9bac9c6cc0a13 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -301,6 +301,7 @@ void BrowserWindow::OnWindowIsKeyChanged(bool is_key) { auto* rwhv = web_contents()->GetRenderWidgetHostView(); if (rwhv) rwhv->SetActive(is_key); + window()->SetActive(is_key); #endif } diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 6c3b0ba4a39d5..99af573c47106 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -696,8 +696,8 @@ WebContents::WebContents(v8::Isolate* isolate, // BrowserViews are not attached to a window initially so they should start // off as hidden. This is also important for compositor recycling. See: // https://github.com/electron/electron/pull/21372 - initially_shown_ = type_ != Type::kBrowserView; - options.Get(options::kShow, &initially_shown_); + bool initially_shown = type_ != Type::kBrowserView; + options.Get(options::kShow, &initially_shown); // Obtain the session. std::string partition; @@ -753,7 +753,7 @@ WebContents::WebContents(v8::Isolate* isolate, #endif } else { content::WebContents::CreateParams params(session->browser_context()); - params.initially_hidden = !initially_shown_; + params.initially_hidden = !initially_shown; web_contents = content::WebContents::Create(params); } @@ -1655,6 +1655,27 @@ void WebContents::DidRedirectNavigation( EmitNavigationEvent("did-redirect-navigation", navigation_handle); } +void WebContents::ReadyToCommitNavigation( + content::NavigationHandle* navigation_handle) { + // Don't focus content in an inactive window. + if (!owner_window()) + return; +#if defined(OS_MAC) + if (!owner_window()->IsActive()) + return; +#else + if (!owner_window()->widget()->IsActive()) + return; +#endif + // Don't focus content after subframe navigations. + if (!navigation_handle->IsInMainFrame()) + return; + // Only focus for top-level contents. + if (type_ != Type::kBrowserWindow) + return; + web_contents()->SetInitialFocus(); +} + void WebContents::DidFinishNavigation( content::NavigationHandle* navigation_handle) { if (!navigation_handle->HasCommitted()) @@ -3101,10 +3122,6 @@ v8::Local WebContents::Debugger(v8::Isolate* isolate) { return v8::Local::New(isolate, debugger_); } -bool WebContents::WasInitiallyShown() { - return initially_shown_; -} - content::RenderFrameHost* WebContents::MainFrame() { return web_contents()->GetMainFrame(); } @@ -3672,7 +3689,6 @@ v8::Local WebContents::FillObjectTemplate( .SetProperty("hostWebContents", &WebContents::HostWebContents) .SetProperty("devToolsWebContents", &WebContents::DevToolsWebContents) .SetProperty("debugger", &WebContents::Debugger) - .SetProperty("_initiallyShown", &WebContents::WasInitiallyShown) .SetProperty("mainFrame", &WebContents::MainFrame) .Build(); } diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index a38dfb895ff28..2950c5fe515fc 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -330,7 +330,6 @@ class WebContents : public gin::Wrappable, content::WebContents* HostWebContents() const; v8::Local DevToolsWebContents(v8::Isolate* isolate); v8::Local Debugger(v8::Isolate* isolate); - bool WasInitiallyShown(); content::RenderFrameHost* MainFrame(); WebContentsZoomController* GetZoomController() { return zoom_controller_; } @@ -567,6 +566,8 @@ class WebContents : public gin::Wrappable, content::NavigationHandle* navigation_handle) override; void DidRedirectNavigation( content::NavigationHandle* navigation_handle) override; + void ReadyToCommitNavigation( + content::NavigationHandle* navigation_handle) override; void DidFinishNavigation( content::NavigationHandle* navigation_handle) override; bool OnMessageReceived(const IPC::Message& message) override; @@ -730,8 +731,6 @@ class WebContents : public gin::Wrappable, v8::Global pending_child_web_preferences_; - bool initially_shown_ = true; - // The window that this WebContents belongs to. base::WeakPtr owner_window_; diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h index 9355658acc1b2..a29039f16afc4 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -135,6 +135,10 @@ class NativeWindow : public base::SupportsUserData, virtual void Invalidate() = 0; virtual void SetTitle(const std::string& title) = 0; virtual std::string GetTitle() = 0; +#if defined(OS_MAC) + virtual void SetActive(bool is_key) = 0; + virtual bool IsActive() const = 0; +#endif // Ability to augment the window title for the screen readers. void SetAccessibleTitle(const std::string& title); diff --git a/shell/browser/native_window_mac.h b/shell/browser/native_window_mac.h index f16f00ab29439..f440abfeb1181 100644 --- a/shell/browser/native_window_mac.h +++ b/shell/browser/native_window_mac.h @@ -139,6 +139,8 @@ class NativeWindowMac : public NativeWindow, void MoveTabToNewWindow() override; void ToggleTabBar() override; bool AddTabbedWindow(NativeWindow* window) override; + void SetActive(bool is_key) override; + bool IsActive() const override; bool SetWindowButtonVisibility(bool visible) override; @@ -263,6 +265,7 @@ class NativeWindowMac : public NativeWindow, bool is_simple_fullscreen_ = false; bool was_maximizable_ = false; bool was_movable_ = false; + bool is_active_ = false; NSRect original_frame_; NSInteger original_level_; NSUInteger simple_fullscreen_mask_; diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index f520f9a49ee93..5597a8c996cfb 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -1746,6 +1746,16 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) { } } +void NativeWindowMac::SetActive(bool is_key) { + if (is_key) + widget()->Activate(); + is_active_ = is_key; +} + +bool NativeWindowMac::IsActive() const { + return is_active_; +} + bool NativeWindowMac::CanResize() const { return resizable_; } diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 520e9518f8501..a484de7d5e68d 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -393,7 +393,7 @@ describe('webContents module', () => { const testFn = (process.platform === 'win32' && process.arch === 'arm64' ? it.skip : it); testFn('returns the focused web contents', async () => { const w = new BrowserWindow({ show: true }); - await w.loadURL('about:blank'); + await w.loadFile(path.join(__dirname, 'fixtures', 'blank.html')); expect(webContents.getFocusedWebContents().id).to.equal(w.webContents.id); const devToolsOpened = emittedOnce(w.webContents, 'devtools-opened'); diff --git a/spec-main/chromium-spec.ts b/spec-main/chromium-spec.ts index 48d94a3dd5092..912af40e675e0 100644 --- a/spec-main/chromium-spec.ts +++ b/spec-main/chromium-spec.ts @@ -1426,6 +1426,31 @@ describe('chromium features', () => { expect(pageExists).to.be.true(); }); }); + + describe('document.hasFocus', () => { + it('has correct value when multiple windows are opened', async () => { + const w1 = new BrowserWindow({ show: true }); + const w2 = new BrowserWindow({ show: true }); + const w3 = new BrowserWindow({ show: false }); + await w1.loadFile(path.join(__dirname, 'fixtures', 'blank.html')); + await w2.loadFile(path.join(__dirname, 'fixtures', 'blank.html')); + await w3.loadFile(path.join(__dirname, 'fixtures', 'blank.html')); + expect(webContents.getFocusedWebContents().id).to.equal(w2.webContents.id); + let focus = false; + focus = await w1.webContents.executeJavaScript( + 'document.hasFocus()' + ); + expect(focus).to.be.false(); + focus = await w2.webContents.executeJavaScript( + 'document.hasFocus()' + ); + expect(focus).to.be.true(); + focus = await w3.webContents.executeJavaScript( + 'document.hasFocus()' + ); + expect(focus).to.be.false(); + }); + }); }); describe('font fallback', () => { @@ -1610,7 +1635,6 @@ describe('navigator.clipboard', () => { let w: BrowserWindow; before(async () => { w = new BrowserWindow({ - show: false, webPreferences: { enableBlinkFeatures: 'Serial' } diff --git a/typings/internal-electron.d.ts b/typings/internal-electron.d.ts index 652a601342bdb..b60edf7c13439 100644 --- a/typings/internal-electron.d.ts +++ b/typings/internal-electron.d.ts @@ -62,7 +62,6 @@ declare namespace Electron { getLastWebPreferences(): Electron.WebPreferences; _getPreloadPaths(): string[]; equal(other: WebContents): boolean; - _initiallyShown: boolean; browserWindowOptions: BrowserWindowConstructorOptions; _windowOpenHandler: ((opts: {url: string, frameName: string, features: string}) => any) | null; _callWindowOpenHandler(event: any, url: string, frameName: string, rawFeatures: string): Electron.BrowserWindowConstructorOptions | null;