Browse Source

fix: beforeunload and unload firing in BrowserViews (#28382)

* fix: beforeunload and unload firing in BrowserViews

* Ensure UserGesture is sent for BV webContents

* spec: add tests

* refactor: clean up logic

* spec: fixup specs

* docs: document event behavior for BrowserViews
Shelley Vohr 4 years ago
parent
commit
7d04f729d8

+ 2 - 0
docs/api/web-contents.md

@@ -375,6 +375,8 @@ win.webContents.on('will-prevent-unload', (event) => {
 })
 ```
 
+**Note:** This will be emitted for `BrowserViews` but will _not_ be respected - this is because we have chosen not to tie the `BrowserView` lifecycle to its owning BrowserWindow should one exist per the [specification](https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event).
+
 #### Event: 'crashed' _Deprecated_
 
 Returns:

+ 21 - 4
shell/browser/api/electron_api_browser_window.cc

@@ -238,18 +238,35 @@ void BrowserWindow::OnCloseButtonClicked(bool* prevent_default) {
   if (window_unresponsive_closure_.IsCancelled())
     ScheduleUnresponsiveEvent(5000);
 
+  // Already closed by renderer.
   if (!web_contents())
-    // Already closed by renderer
     return;
 
   // Required to make beforeunload handler work.
   api_web_contents_->NotifyUserActivation();
 
-  if (web_contents()->NeedToFireBeforeUnloadOrUnloadEvents())
+  // Trigger beforeunload events for associated BrowserViews.
+  for (NativeBrowserView* view : window_->browser_views()) {
+    auto* vwc = view->web_contents();
+    auto* api_web_contents = api::WebContents::From(vwc);
+
+    // Required to make beforeunload handler work.
+    if (api_web_contents)
+      api_web_contents->NotifyUserActivation();
+
+    if (vwc) {
+      if (vwc->NeedToFireBeforeUnloadOrUnloadEvents()) {
+        vwc->DispatchBeforeUnload(false /* auto_cancel */);
+      }
+    }
+  }
+
+  if (web_contents()->NeedToFireBeforeUnloadOrUnloadEvents()) {
     web_contents()->DispatchBeforeUnload(false /* auto_cancel */);
-  else
+  } else {
     web_contents()->Close();
-}
+  }
+}  // namespace api
 
 void BrowserWindow::OnWindowBlur() {
   if (api_web_contents_)

+ 2 - 1
shell/browser/api/electron_api_web_contents.cc

@@ -1120,7 +1120,8 @@ content::WebContents* WebContents::OpenURLFromTab(
 void WebContents::BeforeUnloadFired(content::WebContents* tab,
                                     bool proceed,
                                     bool* proceed_to_fire_unload) {
-  if (type_ == Type::kBrowserWindow || type_ == Type::kOffScreen)
+  if (type_ == Type::kBrowserWindow || type_ == Type::kOffScreen ||
+      type_ == Type::kBrowserView)
     *proceed_to_fire_unload = proceed;
   else
     *proceed_to_fire_unload = true;

+ 31 - 4
spec-main/api-web-contents-spec.ts

@@ -3,7 +3,7 @@ import { AddressInfo } from 'net';
 import * as path from 'path';
 import * as fs from 'fs';
 import * as http from 'http';
-import { BrowserWindow, ipcMain, webContents, session, WebContents, app } from 'electron/main';
+import { BrowserWindow, ipcMain, webContents, session, WebContents, app, BrowserView } from 'electron/main';
 import { clipboard } from 'electron/common';
 import { emittedOnce } from './events-helpers';
 import { closeAllWindows } from './window-helpers';
@@ -49,7 +49,7 @@ describe('webContents module', () => {
 
   describe('will-prevent-unload event', function () {
     afterEach(closeAllWindows);
-    it('does not emit if beforeunload returns undefined', async () => {
+    it('does not emit if beforeunload returns undefined in a BrowserWindow', async () => {
       const w = new BrowserWindow({ show: false });
       w.webContents.once('will-prevent-unload', () => {
         expect.fail('should not have fired');
@@ -60,14 +60,41 @@ describe('webContents module', () => {
       await wait;
     });
 
-    it('emits if beforeunload returns false', async () => {
+    it('does not emit if beforeunload returns undefined in a BrowserView', async () => {
+      const w = new BrowserWindow({ show: false });
+      const view = new BrowserView();
+      w.setBrowserView(view);
+      view.setBounds(w.getBounds());
+
+      view.webContents.once('will-prevent-unload', () => {
+        expect.fail('should not have fired');
+      });
+
+      await view.webContents.loadFile(path.join(__dirname, 'fixtures', 'api', 'beforeunload-undefined.html'));
+      const wait = emittedOnce(w, 'closed');
+      w.close();
+      await wait;
+    });
+
+    it('emits if beforeunload returns false in a BrowserWindow', async () => {
       const w = new BrowserWindow({ show: false });
       await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'beforeunload-false.html'));
       w.close();
       await emittedOnce(w.webContents, 'will-prevent-unload');
     });
 
-    it('supports calling preventDefault on will-prevent-unload events', async () => {
+    it('emits if beforeunload returns false in a BrowserView', async () => {
+      const w = new BrowserWindow({ show: false });
+      const view = new BrowserView();
+      w.setBrowserView(view);
+      view.setBounds(w.getBounds());
+
+      await view.webContents.loadFile(path.join(__dirname, 'fixtures', 'api', 'beforeunload-false.html'));
+      w.close();
+      await emittedOnce(view.webContents, 'will-prevent-unload');
+    });
+
+    it('supports calling preventDefault on will-prevent-unload events in a BrowserWindow', async () => {
       const w = new BrowserWindow({ show: false });
       w.webContents.once('will-prevent-unload', event => event.preventDefault());
       await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'beforeunload-false.html'));