Adding a js confirmation before navigating away from settings menus#10927
Adding a js confirmation before navigating away from settings menus#10927anukasha-mo wants to merge 9 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
src/js/_enqueues/admin/options.js
Outdated
| /** | ||
| * Warn the user if they have unsaved changes. | ||
| * | ||
| * The browser will show a native confirmation dialog when the user | ||
| * attempts to leave the page with unsaved changes. | ||
| */ | ||
| $( window ).on( 'beforeunload', function() { | ||
| // Skip warning if form is being submitted or content hasn't changed. | ||
| if ( isSubmitting || ! $form || ! $form.length ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( originalFormContent !== $form.serialize() ) { | ||
| return __( 'The changes you made will be lost if you navigate away from this page.' ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
This will break bfcache. The beforeunload should only be added once a field is modified. See https://web.dev/articles/bfcache#beforeunload-caution
For example, add a delegated event listener for the change event. For example:
document.addEventListener( 'change', () => {
window.addEventListener( 'beforeunload', () => {
// Check if the original form content is not the same as the current form content.
} );
}, { once: true } );
src/js/_enqueues/admin/options.js
Outdated
| } | ||
| } ); | ||
|
|
||
| }( jQuery ) ); |
There was a problem hiding this comment.
Let's avoid using jQuery unless we have to.
src/js/_enqueues/admin/options.js
Outdated
| return; | ||
| } | ||
|
|
||
| if ( originalFormContent !== $form.serialize() ) { |
There was a problem hiding this comment.
You can use FormData as an alternative to this jQuery API.
| '<p>' . __( '<a href="https://wordpress.org/support/forums/">Support forums</a>' ) . '</p>' | ||
| ); | ||
|
|
||
| wp_enqueue_script( 'user-profile' ); |
There was a problem hiding this comment.
Why not enqueue user-profile anymore?
There was a problem hiding this comment.
we do need to enqueue user-profile as well, re-added it
src/wp-includes/script-loader.php
Outdated
|
|
||
| $scripts->add( 'language-chooser', "/wp-admin/js/language-chooser$suffix.js", array( 'jquery' ), false, 1 ); | ||
|
|
||
| $scripts->add( 'options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); |
There was a problem hiding this comment.
This handle seems overly generic.
There was a problem hiding this comment.
Maybe something like wp-admin-unsaved-changes-confirmation (although this may be overly un-generic! 😄).
There was a problem hiding this comment.
how about wp-admin-options? Since we might be possibly moving other js hooked to admin_head here as well, keeping it a little generic..
There was a problem hiding this comment.
Renamed to wp-admin-unsaved-changes-confirmation since the js file is also now un-generic
joedolson
left a comment
There was a problem hiding this comment.
I'm not opposed to adding a new JS file that contains JS for options screens, but if so, I think we should be moving all the existing JS into it, as well. Currently, options.php contains a ton of admin_head hooked functions that print JS for the various pages, and we instead handle those differently in a central file.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an “unsaved changes” confirmation dialog to WordPress admin Settings (“options-*”) pages to prevent accidental navigation away from modified forms.
Changes:
- Registers a new
optionsadmin script with i18n support in core script loader. - Enqueues the new script across several Settings pages (general/reading/writing/discussion/media/permalink).
- Introduces
src/js/_enqueues/admin/options.jsand wires it into the Grunt build pipeline.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/wp-includes/script-loader.php | Registers the new admin script and translations. |
| src/wp-admin/options-*.php | Enqueues the new script on Settings screens. |
| src/js/_enqueues/admin/options.js | Implements dirty-form detection + beforeunload warning. |
| Gruntfile.js | Builds and copies the new admin bundle output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/wp-admin/options-general.php
Outdated
| '<p>' . __( '<a href="https://wordpress.org/support/forums/">Support forums</a>' ) . '</p>' | ||
| ); | ||
|
|
||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
This enqueue is duplicated across multiple options-*.php screens in the PR. Consider centralizing the enqueue in an appropriate hook (e.g. admin_enqueue_scripts keyed off the current screen) to avoid repeating the same logic across many files and reduce the risk of future drift if the handle/deps change.
@joedolson I agree with your point. -----Edit------ Functions hooked to Hence, for now, I am renaming |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/wp-includes/script-loader.php
Outdated
| $scripts->add( 'wp-admin-options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); | ||
| $scripts->set_translations( 'wp-admin-options' ); |
There was a problem hiding this comment.
The script is registered under the handle wp-admin-options, but the settings screens in this PR enqueue options. As-is, wp_enqueue_script( 'options' ) won't load anything and translations will also not be applied. Align the handle name (either register as options here, or update all enqueues to wp-admin-options) and keep it consistent with other admin script handles in this file.
| $scripts->add( 'wp-admin-options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); | |
| $scripts->set_translations( 'wp-admin-options' ); | |
| $scripts->add( 'options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); | |
| $scripts->set_translations( 'options' ); |
src/wp-admin/options-discussion.php
Outdated
| '<p>' . __( '<a href="https://wordpress.org/support/forums/">Support forums</a>' ) . '</p>' | ||
| ); | ||
|
|
||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
wp_enqueue_script( 'options' ) refers to a script handle that isn't registered in wp_default_scripts() in this PR (the added handle there is wp-admin-options). This means the confirmation script won't be loaded on this screen until the handle names are aligned.
| wp_enqueue_script( 'options' ); | |
| wp_enqueue_script( 'wp-admin-options' ); |
| event.preventDefault(); | ||
| return __( | ||
| 'The changes you made will be lost if you navigate away from this page.' | ||
| ); |
There was a problem hiding this comment.
This beforeunload listener is added via addEventListener(), but the handler only returns a string. For beforeunload, the return value from an event listener is ignored in many browsers; you generally need to set event.returnValue (and typically still call preventDefault()) to reliably trigger the confirmation dialog. Update the handler to assign event.returnValue to the translated message (and return it if needed for older compat).
| event.preventDefault(); | |
| return __( | |
| 'The changes you made will be lost if you navigate away from this page.' | |
| ); | |
| var message = __( | |
| 'The changes you made will be lost if you navigate away from this page.' | |
| ); | |
| event.preventDefault(); | |
| event.returnValue = message; | |
| return message; |
src/wp-includes/script-loader.php
Outdated
|
|
||
| $scripts->add( 'language-chooser', "/wp-admin/js/language-chooser$suffix.js", array( 'jquery' ), false, 1 ); | ||
|
|
||
| $scripts->add( 'wp-admin-options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); |
There was a problem hiding this comment.
This script's dependency list includes jquery, but options.js doesn't appear to use jQuery. Dropping the unused dependency avoids forcing jQuery onto the page and keeps the dependency graph accurate.
| $scripts->add( 'wp-admin-options', "/wp-admin/js/options$suffix.js", array( 'jquery', 'wp-i18n' ), false, 1 ); | |
| $scripts->add( 'wp-admin-options', "/wp-admin/js/options$suffix.js", array( 'wp-i18n' ), false, 1 ); |
src/wp-admin/options-writing.php
Outdated
| ); | ||
|
|
||
| wp_enqueue_script( 'user-profile' ); | ||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
wp_enqueue_script( 'options' ) refers to a script handle that isn't registered in wp_default_scripts() in this PR (the added handle there is wp-admin-options). This means the confirmation script won't be loaded on this screen until the handle names are aligned.
| wp_enqueue_script( 'options' ); | |
| wp_enqueue_script( 'wp-admin-options' ); |
src/wp-admin/options-reading.php
Outdated
| '<p>' . __( '<a href="https://wordpress.org/support/forums/">Support forums</a>' ) . '</p>' | ||
| ); | ||
|
|
||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
wp_enqueue_script( 'options' ) refers to a script handle that isn't registered in wp_default_scripts() in this PR (the added handle there is wp-admin-options). This means the confirmation script won't be loaded on this screen until the handle names are aligned.
| wp_enqueue_script( 'options' ); | |
| wp_enqueue_script( 'wp-admin-options' ); |
src/wp-admin/options-permalink.php
Outdated
| get_current_screen()->set_help_sidebar( $help_sidebar_content ); | ||
| unset( $help_sidebar_content ); | ||
|
|
||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
wp_enqueue_script( 'options' ) refers to a script handle that isn't registered in wp_default_scripts() in this PR (the added handle there is wp-admin-options). This means the confirmation script won't be loaded on this screen until the handle names are aligned.
| wp_enqueue_script( 'options' ); | |
| wp_enqueue_script( 'wp-admin-options' ); |
src/wp-admin/options-media.php
Outdated
| '<p>' . __( '<a href="https://wordpress.org/support/forums/">Support forums</a>' ) . '</p>' | ||
| ); | ||
|
|
||
| wp_enqueue_script( 'options' ); |
There was a problem hiding this comment.
wp_enqueue_script( 'options' ) refers to a script handle that isn't registered in wp_default_scripts() in this PR (the added handle there is wp-admin-options). This means the confirmation script won't be loaded on this screen until the handle names are aligned.
| wp_enqueue_script( 'options' ); | |
| wp_enqueue_script( 'wp-admin-options' ); |
|
|
||
| // Add the beforeunload listener only once a field is modified, to avoid | ||
| // breaking bfcache. | ||
| document.addEventListener( 'change', function () { |
There was a problem hiding this comment.
The change listener is attached to document, so any change anywhere on the page (e.g. Screen Options / Help toggles) will register a beforeunload handler and can still disable bfcache even if the settings form itself was never modified. Consider attaching the { once: true } change listener to the settings form (or its inputs) instead, so the beforeunload handler is only registered when the options form changes.
| document.addEventListener( 'change', function () { | |
| form.addEventListener( 'change', function () { |
Trac ticket: https://core.trac.wordpress.org/ticket/64623