WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 4 weeks ago

#47048 accepted defect (bug)

Nav Menus Admin: Missing Deselect All text when toggling Select All

Reported by: birgire Owned by: audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-screenshots has-patch 2nd-opinion
Focuses: ui, accessibility, administration Cc:

Description

Under /wp-admin/nav-menus.php we can select all menu items, by clicking on the "Select All" link.

This will mark all visible items as checked.

But it's odd having to click again on "Select All" to deselect all.

I would expect "Select All" to change to "Deselect All" after the first click.

References:

PHP:

https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-admin/includes/nav-menu.php#L623

JS:

https://core.trac.wordpress.org/browser/tags/5.1.1/src/js/_enqueues/lib/nav-menu.js#L1093

Attachments (7)

before-clicking-on-select-all.jpg (20.7 KB) - added by birgire 3 months ago.
after-clicking-select-all.jpg (21.9 KB) - added by birgire 3 months ago.
47048.diff (1.5 KB) - added by audrasjb 2 months ago.
Toggle Select All button to Deselect All when clicked
e865dface44e499bdd14c4bf5416c4b7.gif (153.3 KB) - added by audrasjb 2 months ago.
47048.diff patch
47048.2.diff (3.2 KB) - added by afercia 8 weeks ago.
47048-3.diff (4.3 KB) - added by birgire 4 weeks ago.
47048-3.diff.gif (546.7 KB) - added by birgire 4 weeks ago.

Download all attachments as: .zip

Change History (15)

#1 @birgire
3 months ago

  • Keywords has-screenshots added

The screenshot after-clicking-select-all.jpg shows that we need to click on "Select All" to deselect all visible menu items.

#2 @audrasjb
2 months ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to audrasjb
  • Status changed from new to accepted

@audrasjb
2 months ago

Toggle Select All button to Deselect All when clicked

#3 @audrasjb
2 months ago

  • Keywords has-patch dev-feedback added

Not sure it's the perfect way to do that but 47048.diff works fine on my side.

@audrasjb
2 months ago

47048.diff patch

#4 @birgire
2 months ago

  • Focuses accessibility added

Thanks for the patch and the gif-animation @audrasjb

The data-toggle attribute approach seems to work nicely.

I noticed another approach in Customizing > Menus for the Reorder/Done button link:

Reorder:

<button type="button" class="button-link reorder-toggle" aria-label="Reorder menu items" aria-describedby="reorder-items-desc-28">
	<span class="reorder">Reorder</span>
	<span class="reorder-done">Done</span>
</button>

Done:

<button type="button" class="button-link reorder-toggle" aria-label="Close reorder mode" aria-describedby="reorder-items-desc-28">
	<span class="reorder">Reorder</span>
	<span class="reorder-done">Done</span>
</button>

where the aria label and the visibility of each span item changes accordingly via javascript.

Source Ref:

https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/customize/class-wp-customize-nav-menu-control.php#L53
https://core.trac.wordpress.org/browser/tags/5.1.1/src/js/_enqueues/wp/customize/nav-menus.js#L2813
https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/class-wp-customize-nav-menus.php#L474

It would be great to get a feedback regarding the accessibility here, so I add that tag to the ticket.

#5 @afercia
8 weeks ago

I'm not sure dynamically changing the link (sic) text is the best approach. Control names shouldn't change depending on the state and ideally there should be one control for one, specific, action.

Screen reader users, users with cognitive impairments, etc. may struggle to understand why a "Select All" control they previously encountered then disappears.

I've noticed there are also a few additional issues. Theoretically, the menu page should work also when support for JavaScript is off. That's the historical reason why some controls are actually links with URL query parameters.

  • disable JS in your browser: the Select All link has a selectall=1 param
  • clicking the link reloads the page and all Posts are selected
  • however, the link is still selectall=1 so there's no way to deselect
  • theoretically it should change to selectall=0
  • however, seems these links don't work correctly anyways
  • clicking Select All for categories and tags doesn't select the checkboxes
  • haven't tested with CPTs or custom taxonomies...

Instead of trying to fix all this, I'd suggest something along these lines:

  • consider "Select All" an enhancement available only when JavaScript support is enabled
  • at this point, "Select All" could be a checkbox, cons:
    • it's consistent with the pattern for the admin tables
    • it gives a visible and semantic feedback about the selection state
  • the "Select All" checkbox should then be hidden when JS is off using the CSS class hide-if-no-js

#6 @afercia
8 weeks ago

  • Keywords 2nd-opinion added; dev-feedback removed

Here's how the approach I'd like to propose would look like visually:

http://cldup.com/t0BSoGZU94.png

  • semantically and for accessibility this is far better because the checkbox action is clear and the state is communicated natively
  • visually, it's consistent with the patterns used in other places in core

@afercia
8 weeks ago

#7 @afercia
8 weeks ago

47048.2.diff is a first pass for the approach I'd like to propose. It still needs:

  • some clean-up in the PHP part
  • the Select All should also reflect the correct state when manually checking / unchecking all the checkboxes
  • when in the Search tab, the Select All checkbox should be deselected at any new search result

@birgire
4 weeks ago

@birgire
4 weeks ago

#8 @birgire
4 weeks ago

Thank you for the thorough review and the patch @afercia

I like your checkbox suggestion

It feels like a much a better user experience to me,

after having worked with it for the last hour :-)

The patch 47048-3.diff adds support for this part:

the Select All should also reflect the correct state when manually checking / unchecking all the checkboxes

The screencast in 47048-3.diff.gif shows the testing.

Last edited 4 weeks ago by birgire (previous) (diff)
Note: See TracTickets for help on using tickets.