Function listening to AJAX actions to set `Prefers reduced motion` is affecting whole WP Admin in 6.4 RC

[bplv]

1. The newly added jQuery function on 6.4 RC listens to all completed Ajax requests. ​https://github.com/WordPress/wordpress-develop/commit/ef54f4e57fc936ca3f505cb1f00b54a74ac75114#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2201

2. The function expects string but if the data is an object it throws an error for other requests as well. ​https://github.com/WordPress/wordpress-develop/commit/ef54f4e57fc936ca3f505cb1f00b54a74ac75114#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2203

1. The newly added jQuery function on 6.4 RC 1 listens to all completed Ajax requests. ​https://github.com/WordPress/wordpress-develop/commit/ef54f4e57fc936ca3f505cb1f00b54a74ac75114#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2201

2. The function expects string but if the data is an object it throws an error for other requests as well. ​https://github.com/WordPress/wordpress-develop/commit/ef54f4e57fc936ca3f505cb1f00b54a74ac75114#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2203

Trac ticket: https://core.trac.wordpress.org/ticket/59689

[joedolson]

Thanks, @bplv! This seems like a reasonable fix to an issue introduced in 6.4; will review.

[bplv]

Thank you @joedolson ,
I've attached a patch too if that helps :)

[bplv]

Patch-2

[hellofromTonya]

Hey @joedolson, Are you available to review the patch(es) ahead of 6.4 RC2 (which is tomorrow, Oct 24th)?

[joedolson]

@hellofromTonya No, I won't be able to get to this by then.

[rudlinkon]

@bplv thanks for your patch. I checked your last patch 59689.4.diff and it looks good to me because at first, you checked the current page plugin-install by using window.pagenow !== 'plugin-install' which will prevent to execute any other pages Ajax request. Then you also check is settings data string by using typeof settings.data === "string" which will exclude all other types of settings data.

Just to keep consistency, I think you can use 'string' instead of "string" because the single quote is a more strict sting and everywhere uses a single quote of this file.

[hellofromTonya]

Patch: either 59689.4.diff​ or ​PR 5536 - both are the same.

The patch(es) need test reports.

[bplv]

@rudlinkon Updated the patch.

[rudlinkon]

Replying to bplv:

@rudlinkon Updated the patch.

It now looks good to me, waiting for the test report.

[huzaifaalmesbah]

Thanks for Patch @bplv
59689.5.diff This patch looks good for me.

[joedolson]

Testing instructions:

1) For easier observation, add something obvious like console.log( 'Caught AJAX event' ); after the ajaxComplete in the listener.
2) Execute an AJAX driven action in the admin, e.g. approving or unapproving a comment in the Dashboard activity widget.
3) Observe the console log printing the caught event.

This demonstrates that the ajaxComplete listener is catching all ajax actions.

However, I'm not sure that the provided PR solves this; ajaxComplete still executes, it just exits earlier. If you implemented the same instructions above, the test would still fail.

Per my comment in the PR, I think that the check should happen as a condition for the listener to run.

[joedolson]

Updated patch wraps the ajaxComplete listener inside the window.pagenow check.

[joedolson]

Test results with 59689.6.diff

• Before patch: console log statement appears when performing irrelevant AJAX actions.
• After patch: console log statement no longer appears, plugin animations are still stopped as expected on plugin install screen.

[rudlinkon]

Replying to joedolson:

Testing instructions:

1) For easier observation, add something obvious like console.log( 'Caught AJAX event' ); after the ajaxComplete in the listener.
2) Execute an AJAX driven action in the admin, e.g. approving or unapproving a comment in the Dashboard activity widget.
3) Observe the console log printing the caught event.

This demonstrates that the ajaxComplete listener is catching all ajax actions.

However, I'm not sure that the provided PR solves this; ajaxComplete still executes, it just exits earlier. If you implemented the same instructions above, the test would still fail.

Per my comment in the PR, I think that the check should happen as a condition for the listener to run.

Yes, you are right. It is more logical to check the current page before starting the listener.

[jorbin]

Tested and 59689.6.diff looks good to me.

[joedolson]

FYI, 59689.6.diff included an unrelated change by mistake; but it's not relevant to this test at all, so doesn't impact @jorbin's comments. Updating patch, then committing to trunk.

[joedolson]

Updated patch

[joedolson]

In 57022:

Plugins: Prevent ajaxComplete listener from observing all events.

Add a conditional to prevent the prefers-reduced-motion ajaxComplete listener from observing events not occurring in the plugin installation screen. Improve handling of settings data test.

The listener observing ajaxComplete in [56541] was intercepting all ajaxComplete events, creating potential for unexpected errors in unrelated functions.

Props bplv, afercia, rudlinkon, hellofromTonya, huzaifaalmesbah, joedolson, jorbin.
Fixes #59689.

[joedolson]

Reopening for backport to 6.4. Adding dev-feedback for second committer sign off.

[jorbin]

[57022] is good for a backport to 6.4.

In https://core.trac.wordpress.org/changeset/57022

[joedolson]

In 57024:

Plugins: Prevent ajaxComplete listener from observing all events.

Add a conditional to prevent the prefers-reduced-motion ajaxComplete listener from observing events not occurring in the plugin installation screen. Improve handling of settings data test.

The listener observing ajaxComplete in [56541] was intercepting all ajaxComplete events, creating potential for unexpected errors in unrelated functions.

Props bplv, afercia, rudlinkon, hellofromTonya, huzaifaalmesbah, joedolson, jorbin.
Reviewed by jorbin.
Merges [57022] to the 6.4 branch.
Fixes #59689.