Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#59689 closed defect (bug) (fixed)

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

Reported by: bplv's profile bplv Owned by: joedolson's profile joedolson
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.4
Component: Plugins Keywords: has-patch needs-testing has-testing-info dev-reviewed
Focuses: javascript Cc:

Attachments (6)

59689.diff (2.0 KB) - added by bplv 7 months ago.
59689.2.diff (2.2 KB) - added by bplv 7 months ago.
Patch-2
59689.3.diff (2.0 KB) - added by bplv 7 months ago.
59689.4.diff (2.0 KB) - added by bplv 7 months ago.
59689.5.diff (2.0 KB) - added by bplv 7 months ago.
59689.6.diff (2.5 KB) - added by joedolson 7 months ago.
Updated patch

Download all attachments as: .zip

Change History (32)

#2 @joedolson
7 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to joedolson
  • Status changed from new to accepted

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

#3 @joedolson
7 months ago

  • Component changed from General to Plugins
  • Focuses javascript added

@bplv
7 months ago

#4 @bplv
7 months ago

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

@bplv
7 months ago

Patch-2

@bplv
7 months ago

@bplv
7 months ago

#5 @hellofromTonya
7 months ago

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

#6 @joedolson
7 months ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

#8 @rudlinkon
7 months ago

@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.

Last edited 7 months ago by rudlinkon (previous) (diff)

#9 @rudlinkon
7 months ago

  • Keywords changes-requested added

#10 @hellofromTonya
7 months ago

  • Keywords needs-testing added

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

The patch(es) need test reports.

@bplv
7 months ago

#11 follow-up: @bplv
7 months ago

@rudlinkon Updated the patch.

#12 in reply to: ↑ 11 @rudlinkon
7 months ago

  • Keywords changes-requested removed

Replying to bplv:

@rudlinkon Updated the patch.

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

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


7 months ago

#14 @huzaifaalmesbah
7 months ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

#16 follow-up: @joedolson
7 months ago

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.

#17 @joedolson
7 months ago

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

#18 @joedolson
7 months ago

  • Keywords has-testing-info added

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.

#19 in reply to: ↑ 16 @rudlinkon
7 months ago

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.

#20 @jorbin
7 months ago

  • Keywords commit added

Tested and 59689.6.diff looks good to me.

#21 @joedolson
7 months ago

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
7 months ago

Updated patch

#22 @joedolson
7 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#23 @joedolson
7 months ago

  • Keywords dev-feedback added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#24 @jorbin
7 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[57022] is good for a backport to 6.4.

#26 @joedolson
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.