Page MenuHomePhabricator

[web] Prevent user from closing the tab with unsaved edits
ClosedPublic

Authored by kuba on May 17 2023, 3:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 9:06 PM
Unknown Object (File)
Fri, Dec 20, 3:12 PM
Unknown Object (File)
Fri, Dec 20, 9:24 AM
Unknown Object (File)
Wed, Dec 11, 3:47 AM
Unknown Object (File)
Wed, Dec 11, 3:47 AM
Unknown Object (File)
Tue, Dec 3, 10:55 PM
Unknown Object (File)
Tue, Dec 3, 10:55 PM
Unknown Object (File)
Tue, Dec 3, 10:54 PM
Subscribers

Details

Summary

Display confirmation alert when the user leaves the page with unsaved edit.

Test Plan
  • Entered edit mode,
  • Closed tab without making any changes -> no alert,
  • Closed tab with some changes -> alert displayed,
  • Exited edit mode, checked if the alert is not displayed after closing the modal.

Screenshot:

Screenshot 2023-05-17 at 12.24.32.png (502×588 px, 52 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)
  1. Why are you mutating the event you're passing in?
  2. Why are you returning a string | event type?
web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)

I used the code from the official docs:
https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Their example:

const beforeUnloadListener = (event) => {
  event.preventDefault();
  return (event.returnValue = "");
};
web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)

It is done that way due to the compatibility, quote from docs:

The HTML specification states that authors should use the Event.preventDefault() method instead of using Event.returnValue to prompt the user. However, this is not yet supported by all browsers.

web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)

Thanks for explaining. Can you clarify why you're returning event on line 108?

web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)

It's because of the eslint:

Arrow function expected no return value. eslint(consistent-return)
web/chat/edit-text-message.react.js
111 ↗(On Diff #26580)

Does this function need to return something? Can't we just have two bare return;s?

web/chat/edit-text-message.react.js
92 ↗(On Diff #26647)

Event | string is a very strange return type. Let's figure out what the return type should be. Is return undefined or return null possible?

kuba added inline comments.
web/chat/edit-text-message.react.js
92 ↗(On Diff #26647)

Both works. Updated with return null.

kuba marked an inline comment as done.

Changed return type or preventCloseTab

This revision is now accepted and ready to land.May 22 2023, 6:16 AM