Repository: frappe/frappe
Pull request: feat(UI): Add beforeunload Listener for 'Leave Site' Dialog to dirty forms
This closes #17058
When the tab of an unsaved document form is closed, a "Leave Site" Dialog is triggered.

The Dialog will not be triggered when the Bench is in developer mode

For performance reasons, the Listener for the Dialog is only added when dirty() is called on the frame and removed when the frame is refreshed

Link to Documentation: https://frappeframework.com/docs/v14/user/en/desk/edit?wiki_page_patch=f4109a89cc

Problem statement:
Leave Site Dialog for dirty/unsaved Forms
**The Problem**
When a user forgets to save a modified document and closes the Tab, then the changes are lost

**The Solution**
Closing a Tab with unsaved changes triggers a warning via the beforeunload event listener.
Ideally this would probably be configurable on a per-user basis or system wide. But I personally think that this should be standard behaviour

Hints from issue discussion:

Commit message: Feat(UI): Add beforeunload Listener for 'Leave Site' Dialog to dirty forms

Patch to review:
diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js
index a997287a52f4..7b3bdb9d7a57 100644
--- a/frappe/public/js/frappe/form/form.js
+++ b/frappe/public/js/frappe/form/form.js
@@ -112,6 +112,7 @@ frappe.ui.form.Form = class FrappeForm {
 		this.setup_file_drop();
 		this.setup_doctype_actions();
 		this.setup_notify_on_rename();
+		this.set_dirty_listener()
 
 		this.setup_done = true;
 	}
@@ -421,6 +422,17 @@ frappe.ui.form.Form = class FrappeForm {
 		}
 	}
 
+
+	// Adds un beforeunload Listener to trigger a "Leave Site" Dialog, when a dirty (unsaved) form is closed
+	set_dirty_listener() {
+		addEventListener('beforeunload', (event) => {
+			if (cur_frm.is_dirty()) {
+			event.preventDefault();
+			return event.returnValue = 'There are unsaved changes, are you sure you want to exit?';
+			}
+			}, {capture: true});
+	}
+
 	execute_action(action) {
 		if (typeof action === 'string') {
 			// called by label - maybe via custom script


Code review comments:
diff_hunk: @@ -421,6 +422,17 @@ frappe.ui.form.Form = class FrappeForm {
 		}
 	}
 
+
+	// Adds un beforeunload Listener to trigger a "Leave Site" Dialog, when a dirty (unsaved) form is closed
+	set_dirty_listener() {
+		addEventListener('beforeunload', (event) => {
+			if (cur_frm.is_dirty()) {
+			event.preventDefault();
+			return event.returnValue = 'There are unsaved changes, are you sure you want to exit?';
+			}
path: frappe/public/js/frappe/form/form.js
text: @user1:
Thanks for this PR, very good addition for UX 🎉 

Few concerns:

1. This should probably be disabled in developer mode (annoying with JS build reloader) `frappe.boot.developer_mode` flag can be used. 
2. The return value doesn't appear in chrome at least, I got default warning `Changes that you made may not be saved.`
3. Check if `cur_frm` is truthy before checking if it's dirty, it might be unset if user moved to another page. 

<img width="1072" alt="Screenshot 2022-06-02 at 3 15 26 PM" src="https://user-images.githubusercontent.com/9079960/171604014-99e1eb69-86bd-4322-a8c0-f3821f2dbd61.png">


@author:
You make good points.
1.**Disabled in developer mode**  absolutely, I will  implement it
2.**The return value doesn't appear**: The [Mozilla Docs](https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event)  state:

> When this event returns (or sets the returnValue property to) a value other than null or undefined, the user will be prompted to confirm the page unload. In older browsers, the return value of the event is displayed in this dialog. Starting with Firefox 44, Chrome 51, Opera 38, and Safari 9.1, a generic string not under the control of the webpage will be shown instead of the returned string

3. **Check if cur_frm is truthy** I will also implement this
4. The Mozilla Docs also mention:
 >It is recommended that developers listen for beforeunload [...] only when they actually have unsaved changes, so as to minimize the effect on performance.

Therefore, the listener should probably be added/removed at the same point in code, where the is_dirty is set/unset

@author:
1. and 4. are implemented with https://github.com/frappe/frappe/pull/17059/commits/094eebdf303380cc899074f722540332bf446d5b 
2.: I will leave the returned String as it is for compatability with older Browsers, since it doesn't hurt in newer Browsers, the return Value just has to be truthy
3. is no longer necessary, because I don't check for is_dirty anymore, but add the listener on dirty() and remove it on refresh()

diff_hunk: @@ -421,6 +422,17 @@ frappe.ui.form.Form = class FrappeForm {
 		}
 	}
 
+
+	// Adds un beforeunload Listener to trigger a "Leave Site" Dialog, when a dirty (unsaved) form is closed
+	set_dirty_listener() {
+		addEventListener('beforeunload', (event) => {
+			if (cur_frm.is_dirty()) {
+			event.preventDefault();
+			return event.returnValue = 'There are unsaved changes, are you sure you want to exit?';
path: frappe/public/js/frappe/form/form.js
text: @user1:
https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

The custom string for the popup is deprecated

@author:
I'll leave it in for backwards compatability