WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 months ago

#36202 assigned defect (bug)

Menus screen: fix persistence of toggles for displayed nav menu item properties

Reported by: afercia Owned by: ryankienstra
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Menus Keywords: needs-testing has-patch needs-refresh
Focuses: ui, javascript Cc:

Description

Splitting this out from #35273.

The Screen Options in nav-menus.php would need a similar treatment as the one used for the options in the Customizer.

When quickly clicking on the checkboxes, multiple separate AJAX requests fire and there's no guarantee about the order they will be processed. This could lead to a non-synchronized state of the options in the Menus Screen and in the Customizer.

To consider: maybe do this for all the screen options in all the admin screens?

For details, see #35273 and the solution implemented in [36908].

Attachments (2)

screen options.png (69.7 KB) - added by westonruter 21 months ago.
36202.diff (4.7 KB) - added by ryankienstra 20 months ago.

Download all attachments as: .zip

Change History (14)

#1 @ryankienstra
21 months ago

Pull Request

Please see this GitHub pull request for the code review of this ticket's code.

#2 @westonruter
21 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to westonruter
  • Status changed from new to accepted

#3 @westonruter
21 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from westonruter to ryankienstra
  • Status changed from accepted to assigned

#4 @westonruter
21 months ago

@ryankienstra I found one more edge case related to our PR: https://github.com/xwp/wordpress-develop/pull/159

See screen options.png. The screen options on the edit posts screen, in addition to these checkboxes, also has a form for inputting pagination and the view mode. This form has an Apply button. If you are quick with your checkbox selections and then quickly tab down to hit Apply, the hidden-columns Ajax request will not have a change to send, and once the form has been submitted and the page reloads, the checkboxes will remain as they were before.

The only thing I can think of to fix this would be to perhaps add a submit event handler for the adv-settings form which can listen to whether or not there is a pending hidden-columns Ajax request. If so, then the submit should be prevented and instead deferred until the Ajax request is done. Not super pretty. There may be a better way to go about it, like maybe including the hidden-columns in the adv-settings request.

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


21 months ago

#6 @ryankienstra
21 months ago

Commits To Address Form Submission Issue

Hi @westonruter, You commented above that on edit.php page, it's possible to refresh the page by clicking "Apply," before the ajax requests are sent. This commit to this ticket's GitHub pull request passes true as the immediate argument to _.debounce. So this submits the first ajax request, without a 2000 ms delay. This fixes an issue where you click one "Columns" checkbox, like "Author," and quickly click "Apply."

But there's still an issue if you click on several screen options, and then click "Apply." This next commit delays form submission until the current ajax request completes. But there could be 3 ajax requests queued with _.debounce. This handler isn't very useful for clicking multiple screen options, and clicking "Apply."

Last edited 21 months ago by ryankienstra (previous) (diff)

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


20 months ago

@ryankienstra
20 months ago

#8 @ryankienstra
20 months ago

Request For Review Of Patch

Hi @afercia, Could you please review this patch, which debounces the ajax requests on selecting screen options? Most of the code review was on the GitHub pull request, which is identical to the attached patch.

#9 @ryankienstra
20 months ago

  • Keywords needs-testing has-patch added; needs-patch removed

#10 @afercia
20 months ago

Sorry for the late reply. Tested 36202.diff and seems to me it works just on the menus screen and just for the "Boxes" checkboxes, not for the "Show advanced menu properties" ones. On edit.php doesn't work as intended since, as you pointed out, immediate is set to true so on a first click the request is triggered immediately and any subsequent click in the following 2 seconds doesn't trigger a new request: nothing happens. You have to wait two seconds to actually trigger a new requests. The columns disappear just because the hidden CSS class gets applied but, for example, when quickly unchecking the columns, wait a few seconds, and then refresh the page, their state is not saved. Just the first one is saved. Preventing the form submit when clicking "Apply" is not so ideal, I think, since it could lead to unexpected results and mislead users. They could think everything is saved, while it is not.

Overall, I think we're trying to solve with JS an issue that it's intrinsic to the UI. Maybe we should reconsider the reasons why the settings tab is using two different save methods in the same UI. I'd say if they work in a different way, that should be self-evident in the UI itself. I'd also consider to entirely remove the AJAX saving as pointed out by @helen in a similar case on the Customizer.

Last edited 6 months ago by afercia (previous) (diff)

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


20 months ago

#12 @afercia
20 months ago

  • Keywords needs-refresh added
  • Milestone changed from 4.7 to Future Release

Discussed in today's dev bug-scrub and since time is running out for this release cycle, punting. I'd also recommend to do an effort to think at this globally, since there are other parts of the UI in the admin with the same issue.

Note: See TracTickets for help on using tickets.