From e7de2f8955fc79f4a49e9ca2b95bbd27bb853eaa Mon Sep 17 00:00:00 2001
From: Shubham <mukati88shubh@gmail.com>
Date: Fri, 19 Jul 2024 14:26:17 +0530
Subject: [PATCH] Refactor UnsavedController to use event.preventDefault() to
 trigger browser confirmation dialog

Per https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

"best practice is to trigger the dialog by invoking `preventDefault()`
on the event object, while also setting `returnValue` to support legacy
cases."

We don't need to support legacy cases, as our supported browsers all
support the `preventDefault()` approach.

See also:
https://caniuse.com/mdn-api_window_beforeunload_event_preventdefault_activation

Fixes #12132

From PR #12139
---
 CHANGELOG.txt                                 |  1 +
 CONTRIBUTORS.md                               |  1 +
 .../src/controllers/ActionController.test.js  | 18 ++----
 .../src/controllers/UnsavedController.test.js | 39 +++++++++----
 client/src/controllers/UnsavedController.ts   | 55 +++++++++++--------
 docs/releases/6.3.md                          |  1 +
 .../templates/wagtailadmin/pages/create.html  |  2 +-
 .../templates/wagtailadmin/pages/edit.html    |  2 +-
 8 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index 7b4cdbe779..11de853659 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -281,6 +281,7 @@ Changelog
  * Maintenance: Remove jQuery usage in telepath widget classes (Matt Westcott)
  * Maintenance: Remove `xregexp` (IE11 polyfill) along with `window.XRegExp` global util (LB (Ben) Johnston)
  * Maintenance: Refactor the Django port of `urlify` to use TypeScript, officially deprecate `window.URLify` global util (LB (Ben) Johnston)
+ * Maintenance: Adopt the modern best practice for `beforeunload` usage in `UnsavedController` to trigger a leave page warning when edits have been made (Shubham Mukati, Sage Abdullah)
 
 
 6.0.6 (11.07.2024)
diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md
index ef54e855d8..f2217ab39a 100644
--- a/CONTRIBUTORS.md
+++ b/CONTRIBUTORS.md
@@ -833,6 +833,7 @@
 * Daniel Black
 * Atif Khan
 * Sanjeev Holla S
+* Shubham Mukati
 
 ## Translators
 
diff --git a/client/src/controllers/ActionController.test.js b/client/src/controllers/ActionController.test.js
index 30f5fa3ad9..fbb84333db 100644
--- a/client/src/controllers/ActionController.test.js
+++ b/client/src/controllers/ActionController.test.js
@@ -27,11 +27,7 @@ describe('ActionController', () => {
         reload: {
           configurable: true,
           value: jest.fn().mockImplementation(() => {
-            const event = new Event('beforeunload');
-            Object.defineProperty(event, 'returnValue', {
-              value: null,
-              writable: true,
-            });
+            const event = new Event('beforeunload', { cancelable: true });
             window.dispatchEvent(event);
           }),
         },
@@ -156,9 +152,8 @@ describe('ActionController', () => {
       expect(beforeUnloadHandler).toHaveBeenCalledTimes(1);
 
       const event = beforeUnloadHandler.mock.lastCall[0];
-      // These mean the browser confirmation dialog was not shown
+      // This means the browser confirmation dialog was not shown
       expect(event.defaultPrevented).toBe(false);
-      expect(event.returnValue).toBeNull();
 
       window.removeEventListener('beforeunload', beforeUnloadHandler);
     });
@@ -168,7 +163,7 @@ describe('ActionController', () => {
       <form
         data-controller="w-unsaved"
         data-action="beforeunload@window->w-unsaved#confirm"
-        data-w-unsaved-confirmation-value="You have unsaved changes!"
+        data-w-unsaved-confirmation-value="true"
       >
       </form>
       <button
@@ -197,7 +192,7 @@ describe('ActionController', () => {
 
       const event = beforeUnloadHandler.mock.lastCall[0];
       // This means the browser confirmation dialog was shown
-      expect(event.returnValue).toBe('You have unsaved changes!');
+      expect(event.defaultPrevented).toBe(true);
       window.removeEventListener('beforeunload', beforeUnloadHandler);
     });
   });
@@ -208,7 +203,7 @@ describe('ActionController', () => {
       <form
         data-controller="w-unsaved"
         data-action="beforeunload@window->w-unsaved#confirm"
-        data-w-unsaved-confirmation-value="You have unsaved changes!"
+        data-w-unsaved-confirmation-value="true"
       >
       </form>
       <button
@@ -239,9 +234,8 @@ describe('ActionController', () => {
       expect(beforeUnloadHandler).toHaveBeenCalledTimes(1);
 
       const beforeUnloadEvent = beforeUnloadHandler.mock.lastCall[0];
-      // If the browser confirmation was shown, these would be truthy
+      // If the browser confirmation was shown, this would be true
       expect(beforeUnloadEvent.defaultPrevented).toBe(false);
-      expect(beforeUnloadEvent.returnValue).toBeNull();
 
       expect(confirmHandler).toHaveBeenCalledTimes(1);
       const confirmEvent = confirmHandler.mock.lastCall[0];
diff --git a/client/src/controllers/UnsavedController.test.js b/client/src/controllers/UnsavedController.test.js
index bea18fd821..4ad27da833 100644
--- a/client/src/controllers/UnsavedController.test.js
+++ b/client/src/controllers/UnsavedController.test.js
@@ -52,7 +52,7 @@ describe('UnsavedController', () => {
       id="form"
       data-controller="w-unsaved"
       data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
-      data-w-unsaved-confirmation-value="You have unsaved changes!"
+      data-w-unsaved-confirmation-value="true"
     >
       <input type="text" id="name" value="John" />
       <button>Submit</submit>
@@ -212,15 +212,10 @@ describe('UnsavedController', () => {
   describe('showing a confirmation message when exiting the browser tab', () => {
     const mockBrowserClose = () =>
       new Promise((resolve) => {
-        const event = new Event('beforeunload');
-        Object.defineProperty(event, 'returnValue', {
-          value: false,
-          writable: true,
-        });
-
+        const event = new Event('beforeunload', { cancelable: true });
         window.dispatchEvent(event);
-
-        resolve(event.returnValue);
+        // If the event is prevented, the browser will show a confirmation message.
+        resolve(event.defaultPrevented);
       });
 
     it('should not show a confirmation message if no edits exist', async () => {
@@ -238,7 +233,7 @@ describe('UnsavedController', () => {
         <form
           data-controller="w-unsaved"
           data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
-          data-w-unsaved-confirmation-value="You have unsaved changes!"
+          data-w-unsaved-confirmation-value="true"
           data-w-unsaved-force-value="true"
         >
           <input type="text" id="name" value="John" />
@@ -248,10 +243,30 @@ describe('UnsavedController', () => {
 
       const result = await mockBrowserClose();
 
-      expect(result).toEqual('You have unsaved changes!');
+      expect(result).toEqual(true);
       expect(events['w-unsaved:confirm']).toHaveLength(1);
     });
 
+    it('should not show a confirmation message if forced but confirmation value is false', async () => {
+      await setup(`
+      <section>
+        <form
+          data-controller="w-unsaved"
+          data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
+          data-w-unsaved-confirmation-value="false"
+          data-w-unsaved-force-value="true"
+        >
+          <input type="text" id="name" value="John" />
+          <button>Submit</submit>
+        </form>
+      </section>`);
+
+      const result = await mockBrowserClose();
+
+      expect(result).toEqual(false);
+      expect(events['w-unsaved:confirm']).toHaveLength(0);
+    });
+
     it('should allow a confirmation message to show before the browser closes', async () => {
       await setup();
 
@@ -263,7 +278,7 @@ describe('UnsavedController', () => {
 
       const result = await mockBrowserClose();
 
-      expect(result).toEqual('You have unsaved changes!');
+      expect(result).toEqual(true);
       expect(events['w-unsaved:confirm']).toHaveLength(1);
     });
 
diff --git a/client/src/controllers/UnsavedController.ts b/client/src/controllers/UnsavedController.ts
index 7d9214cb8f..ae381f15d9 100644
--- a/client/src/controllers/UnsavedController.ts
+++ b/client/src/controllers/UnsavedController.ts
@@ -24,7 +24,7 @@ const DEFAULT_DURATIONS = {
  * <form
  *   data-controller="w-unsaved"
  *   data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
- *   data-w-unsaved-confirmation-value="You have unsaved changes!"
+ *   data-w-unsaved-confirmation-value="true"
  * >
  *   <input type="text" value="something" />
  *   <button>Submit</submit>
@@ -34,7 +34,7 @@ const DEFAULT_DURATIONS = {
  * <form
  *   data-controller="w-unsaved"
  *   data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
- *   data-w-unsaved-confirmation-value="You have unsaved changes!"
+ *   data-w-unsaved-confirmation-value="true"
  *   data-w-unsaved-watch-value="edits comments"
  * >
  *   <input type="text" value="something" />
@@ -45,7 +45,7 @@ const DEFAULT_DURATIONS = {
  * <form
  *   data-controller="w-unsaved"
  *   data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check"
- *   data-w-unsaved-confirmation-value="You have unsaved changes!"
+ *   data-w-unsaved-confirmation-value="true"
  *   data-w-unsaved-force-value="true"
  * >
  *   <input type="text" value="something" />
@@ -56,7 +56,7 @@ const DEFAULT_DURATIONS = {
  * <form
  *   data-controller="w-unsaved"
  *   data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm"
- *   data-w-unsaved-confirmation-value="Please double check before you close"
+ *   data-w-unsaved-confirmation-value="true"
  *   data-w-unsaved-force-value="true"
  *   data-w-unsaved-watch-value=""
  * >
@@ -66,7 +66,7 @@ const DEFAULT_DURATIONS = {
  */
 export class UnsavedController extends Controller<HTMLFormElement> {
   static values = {
-    confirmation: { default: '', type: String },
+    confirmation: { default: false, type: Boolean },
     durations: { default: DEFAULT_DURATIONS, type: Object },
     force: { default: false, type: Boolean },
     hasComments: { default: false, type: Boolean },
@@ -74,11 +74,21 @@ export class UnsavedController extends Controller<HTMLFormElement> {
     watch: { default: 'edits', type: String },
   };
 
-  /** Translated value for the beforeunload confirmation dialog, if empty no confirmation will show. */
-  declare confirmationValue: string;
+  /** Whether to show the browser confirmation dialog. */
+  declare confirmationValue: boolean;
   /** Configurable duration values. */
   declare durationsValue: typeof DEFAULT_DURATIONS;
-  /** When set to true, the form will always be considered dirty and the confirmation dialog will be forced to show. */
+  /**
+   * When set to `true`, the form will always be considered dirty.
+   * Useful for when the user just submitted an invalid form, in which case we
+   * consider the form to be dirty even on initial load.
+   *
+   * Setting this to `true` effectively disables the edit check, i.e. similar to
+   * setting `watchValue` to `''` and setting `hasEditsValue` to `true`.
+   *
+   * Note that the `confirmationValue` must still be set to `true` in order for
+   * the browser confirmation dialog to appear.
+   */
   declare forceValue: boolean;
   /** Value (state) tracking of what changes exist (comments). */
   declare hasCommentsValue: boolean;
@@ -172,25 +182,21 @@ export class UnsavedController extends Controller<HTMLFormElement> {
   }
 
   /**
-   * Trigger the beforeunload confirmation dialog if active (confirm value exists).
+   * Trigger the beforeunload confirmation dialog if active (confirm value is true).
    * @see https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event
    */
   confirm(event: BeforeUnloadEvent) {
-    const confirmationMessage = this.confirmationValue;
-
-    if (!confirmationMessage) return null;
+    if (!this.confirmationValue) return;
 
     if (this.forceValue || this.hasCommentsValue || this.hasEditsValue) {
+      // Dispatch a `confirm` event that is cancelable to allow for custom handling
+      // instead of the browser's default confirmation dialog.
       const confirmEvent = this.dispatch('confirm', { cancelable: true });
-      if (confirmEvent.defaultPrevented) return null;
+      if (confirmEvent.defaultPrevented) return;
 
-      // eslint-disable-next-line no-param-reassign
-      event.returnValue = confirmationMessage;
-
-      return confirmationMessage;
+      // This will trigger the browser's default confirmation dialog
+      event.preventDefault();
     }
-
-    return null;
   }
 
   hasCommentsValueChanged(current: boolean, previous: boolean) {
@@ -237,10 +243,10 @@ export class UnsavedController extends Controller<HTMLFormElement> {
   /**
    * When the form is submitted, ensure that the exit confirmation
    * does not trigger. Deactivate the confirmation by setting the
-   * confirm value to empty.
+   * confirmation value to false.
    */
   submit() {
-    this.confirmationValue = '';
+    this.confirmationValue = false;
   }
 
   /**
@@ -360,8 +366,11 @@ export class UnsavedController extends Controller<HTMLFormElement> {
         if (!(form instanceof HTMLFormElement)) return;
 
         [
-          ['data-w-unsaved-confirmation-value', confirmationMessage || ' '],
-          ['data-w-unsaved-force-value', String(alwaysDirty || false)],
+          [
+            'data-w-unsaved-confirmation-value',
+            `${!!confirmationMessage || true}`,
+          ],
+          ['data-w-unsaved-force-value', `${alwaysDirty || false}`],
           ['data-w-unsaved-watch-value', 'edits comments'],
         ].forEach(([key, value]) => {
           form.setAttribute(key, value);
diff --git a/docs/releases/6.3.md b/docs/releases/6.3.md
index fdf311700c..4036ec68c9 100644
--- a/docs/releases/6.3.md
+++ b/docs/releases/6.3.md
@@ -75,6 +75,7 @@ This release adds formal support for Django 5.1.
  * Migrate preview-panel JavaScript to Stimulus & TypeScript, add full unit testing (Sage Abdullah)
  * Move `wagtailConfig` values from inline scripts to the `wagtail_config` template tag (LB (Ben) Johnston, Sage Abdullah)
  * Deprecate the `{% locales %}` and `{% js_translation_strings %}` template tags (LB (Ben) Johnston, Sage Abdullah)
+ * Adopt the modern best practice for `beforeunload` usage in `UnsavedController` to trigger a leave page warning when edits have been made (Shubham Mukati, Sage Abdullah)
 
 
 ## Upgrade considerations - changes affecting all projects
diff --git a/wagtail/admin/templates/wagtailadmin/pages/create.html b/wagtail/admin/templates/wagtailadmin/pages/create.html
index 8ac274b745..5f42e5b997 100644
--- a/wagtail/admin/templates/wagtailadmin/pages/create.html
+++ b/wagtail/admin/templates/wagtailadmin/pages/create.html
@@ -18,7 +18,7 @@
         data-controller="w-init w-unsaved"
         data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check keyup->w-unsaved#check"
         data-w-init-event-value="{% if comments_enabled %}w-comments:init{% endif %}"
-        data-w-unsaved-confirmation-value="{{ _("This page has unsaved changes.") }}"
+        data-w-unsaved-confirmation-value="true"
         data-w-unsaved-force-value="{% if has_unsaved_changes %}true{% else %}false{% endif %}"
         data-w-unsaved-watch-value="edits{% if comments_enabled %} comments{% endif %}"
     >
diff --git a/wagtail/admin/templates/wagtailadmin/pages/edit.html b/wagtail/admin/templates/wagtailadmin/pages/edit.html
index c1e8f25e99..b1c07a4516 100644
--- a/wagtail/admin/templates/wagtailadmin/pages/edit.html
+++ b/wagtail/admin/templates/wagtailadmin/pages/edit.html
@@ -22,7 +22,7 @@
             data-controller="w-init w-unsaved"
             data-action="w-unsaved#submit beforeunload@window->w-unsaved#confirm change->w-unsaved#check keyup->w-unsaved#check"
             data-w-init-event-value="{% if comments_enabled %}w-comments:init{% endif %}"
-            data-w-unsaved-confirmation-value="{{ _("This page has unsaved changes.") }}"
+            data-w-unsaved-confirmation-value="true"
             data-w-unsaved-force-value="{% if has_unsaved_changes %}true{% else %}false{% endif %}"
             data-w-unsaved-watch-value="edits{% if comments_enabled %} comments{% endif %}"
         >