fix: adjust initial webContents focus calculation (#29234)

* fix: adjust initial webContents focus calculation

* fix: active window check on mac

* fix: about:blank focus behavior

* chore: add spec

* fix: window ordering on mac

* chore: focus <webview> after navigation

* chore: fix flaky fullscreen inheritance test

* chore: disable fullscreen test on mac arm

* chore: simplify found-in-page spec

* chore: fix lint

* chore: move found-in-page spec to main process

* chore: fix lint

* chore: remove focus workaround

* chore: improve found-in-page spec

Co-authored-by: Raymond Zhao <raymondzhao@microsoft.com>
Co-authored-by: deepak1556 <hop2deep@gmail.com>

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);

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
 }
 

shell/browser/api/electron_api_web_contents.cc

@@ -702,8 +702,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;
@@ -759,7 +759,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())
@@ -3126,10 +3147,6 @@ v8::Local<v8::Value> WebContents::Debugger(v8::Isolate* isolate) {
   return v8::Local<v8::Value>::New(isolate, debugger_);
 }
 
-bool WebContents::WasInitiallyShown() {
-  return initially_shown_;
-}
-
 content::RenderFrameHost* WebContents::MainFrame() {
   return web_contents()->GetMainFrame();
 }
@@ -3752,7 +3769,6 @@ v8::Local<v8::ObjectTemplate> WebContents::FillObjectTemplate(
       .SetProperty("hostWebContents", &WebContents::HostWebContents)
       .SetProperty("devToolsWebContents", &WebContents::DevToolsWebContents)
       .SetProperty("debugger", &WebContents::Debugger)
-      .SetProperty("_initiallyShown", &WebContents::WasInitiallyShown)
       .SetProperty("mainFrame", &WebContents::MainFrame)
       .Build();
 }

shell/browser/api/electron_api_web_contents.h

@@ -330,7 +330,6 @@ class WebContents : public gin::Wrappable<WebContents>,
   content::WebContents* HostWebContents() const;
   v8::Local<v8::Value> DevToolsWebContents(v8::Isolate* isolate);
   v8::Local<v8::Value> Debugger(v8::Isolate* isolate);
-  bool WasInitiallyShown();
   content::RenderFrameHost* MainFrame();
 
   WebContentsZoomController* GetZoomController() { return zoom_controller_; }
@@ -567,6 +566,8 @@ class WebContents : public gin::Wrappable<WebContents>,
       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;
@@ -735,8 +736,6 @@ class WebContents : public gin::Wrappable<WebContents>,
 
   v8::Global<v8::Value> pending_child_web_preferences_;
 
-  bool initially_shown_ = true;
-
   // The window that this WebContents belongs to.
   base::WeakPtr<NativeWindow> owner_window_;
 

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);

shell/browser/native_window_mac.h

@@ -149,6 +149,8 @@ class NativeWindowMac : public NativeWindow,
   gfx::Rect WindowBoundsToContentBounds(const gfx::Rect& bounds) const override;
   void NotifyWindowEnterFullScreen() override;
   void NotifyWindowLeaveFullScreen() override;
+  void SetActive(bool is_key) override;
+  bool IsActive() const override;
 
   void NotifyWindowWillEnterFullScreen();
   void NotifyWindowWillLeaveFullScreen();
@@ -270,6 +272,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_;

shell/browser/native_window_mac.mm

@@ -1671,6 +1671,14 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
   UpdateVibrancyRadii(false);
 }
 
+void NativeWindowMac::SetActive(bool is_key) {
+  is_active_ = is_key;
+}
+
+bool NativeWindowMac::IsActive() const {
+  return is_active_;
+}
+
 void NativeWindowMac::ReorderButtonsView() {
   if (buttons_view_) {
     [buttons_view_ removeFromSuperview];

spec-main/api-browser-window-spec.ts

@@ -4311,6 +4311,20 @@ describe('BrowserWindow module', () => {
         await leaveFullScreen;
         expect(w.isFullScreen()).to.be.false('isFullScreen');
       });
+
+      ifit(process.arch === 'x64')('multiple windows inherit correct fullscreen state', async () => {
+        const w = new BrowserWindow();
+        const enterFullScreen = emittedOnce(w, 'enter-full-screen');
+        w.setFullScreen(true);
+        await enterFullScreen;
+        expect(w.isFullScreen()).to.be.true('isFullScreen');
+        await delay();
+        const w2 = new BrowserWindow({ show: false });
+        const enterFullScreen2 = emittedOnce(w2, 'enter-full-screen');
+        w2.show();
+        await enterFullScreen2;
+        expect(w2.isFullScreen()).to.be.true('isFullScreen');
+      });
     });
 
     describe('closable state', () => {

spec-main/api-web-contents-spec.ts

@@ -392,7 +392,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');

spec-main/chromium-spec.ts

@@ -1481,6 +1481,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', () => {
@@ -1665,7 +1690,6 @@ describe('navigator.clipboard', () => {
   let w: BrowserWindow;
   before(async () => {
     w = new BrowserWindow({
-      show: false,
       webPreferences: {
         enableBlinkFeatures: 'Serial'
       }

spec-main/webview-spec.ts

@@ -783,4 +783,51 @@ describe('<webview> tag', function () {
       })`);
     });
   });
+
+  describe('found-in-page event', () => {
+    let w: BrowserWindow;
+    beforeEach(async () => {
+      w = new BrowserWindow({ show: false, webPreferences: { webviewTag: true, contextIsolation: false } });
+      await w.loadURL('about:blank');
+    });
+    afterEach(closeAllWindows);
+
+    it('emits when a request is made', async () => {
+      await loadWebView(w.webContents, {
+        src: `file://${fixtures}/pages/content.html`
+      });
+      const result = await w.webContents.executeJavaScript(`new Promise((resolve, reject) => {
+        const webview = document.querySelector('webview')
+
+        const waitForEvent = (target, eventName) => {
+          return new Promise(resolve => {
+            target.addEventListener(eventName, resolve, { once: true })
+          })
+        }
+
+        async function startFind() {
+          const activeMatchOrdinal = []
+          const foundInPage = waitForEvent(webview, 'found-in-page')
+          webview.findInPage('virtual', { findNext: true })
+          const event = await foundInPage
+          if (event.result.matches) {
+            activeMatchOrdinal.push(event.result.activeMatchOrdinal)
+
+            for (let i=1; i < event.result.matches; i++) {
+              const foundInPage = waitForEvent(webview, 'found-in-page')
+              webview.findInPage('virtual', { findNext: false })
+              const event = await foundInPage
+              activeMatchOrdinal.push(event.result.activeMatchOrdinal)
+            }
+          }
+          webview.stopFindInPage('clearSelection')
+          resolve(activeMatchOrdinal)
+        }
+
+        webview.focus()
+        startFind()
+      })`);
+      expect(result).to.deep.equal([1, 2, 3]);
+    });
+  });
 });

spec/webview-spec.js

@@ -973,39 +973,6 @@ describe('<webview> tag', function () {
     });
   });
 
-  describe('found-in-page event', () => {
-    it('emits when a request is made', async () => {
-      const didFinishLoad = waitForEvent(webview, 'did-finish-load');
-      loadWebView(webview, { src: `file://${fixtures}/pages/content.html` });
-      // TODO(deepak1556): With https://codereview.chromium.org/2836973002
-      // focus of the webContents is required when triggering the api.
-      // Remove this workaround after determining the cause for
-      // incorrect focus.
-      webview.focus();
-      await didFinishLoad;
-
-      const activeMatchOrdinal = [];
-
-      for (;;) {
-        const foundInPage = waitForEvent(webview, 'found-in-page');
-        const requestId = webview.findInPage('virtual');
-        const event = await foundInPage;
-
-        expect(event.result.requestId).to.equal(requestId);
-        expect(event.result.matches).to.equal(3);
-
-        activeMatchOrdinal.push(event.result.activeMatchOrdinal);
-
-        if (event.result.activeMatchOrdinal === event.result.matches) {
-          break;
-        }
-      }
-
-      expect(activeMatchOrdinal).to.deep.equal([1, 2, 3]);
-      webview.stopFindInPage('clearSelection');
-    });
-  });
-
   describe('<webview>.getWebContentsId', () => {
     it('can return the WebContents ID', async () => {
       const src = 'about:blank';

typings/internal-electron.d.ts

@@ -67,7 +67,6 @@ declare namespace Electron {
     getLastWebPreferences(): Electron.WebPreferences;
     _getPreloadPaths(): string[];
     equal(other: WebContents): boolean;
-    _initiallyShown: boolean;
     browserWindowOptions: BrowserWindowConstructorOptions;
     _windowOpenHandler: ((details: Electron.HandlerDetails) => any) | null;
     _callWindowOpenHandler(event: any, details: Electron.HandlerDetails): Electron.BrowserWindowConstructorOptions | null;