WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 39 minutes 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
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 (10)

before-clicking-on-select-all.jpg (20.7 KB) - added by birgire 5 months ago.
after-clicking-select-all.jpg (21.9 KB) - added by birgire 5 months ago.
47048.diff (1.5 KB) - added by audrasjb 5 months ago.
Toggle Select All button to Deselect All when clicked
e865dface44e499bdd14c4bf5416c4b7.gif (153.3 KB) - added by audrasjb 5 months ago.
47048.diff patch
47048.2.diff (3.2 KB) - added by afercia 4 months ago.
47048-3.diff (4.3 KB) - added by birgire 3 months ago.
47048-3.diff.gif (546.7 KB) - added by birgire 3 months ago.
47048-4.diff (4.6 KB) - added by birgire 5 weeks ago.
47048-4.gif (179.4 KB) - added by birgire 5 weeks ago.
47048-5.diff (4.9 KB) - added by birgire 5 weeks ago.
Only unset the checked .select-all checkbox during searches, if it's already set.

Download all attachments as: .zip

Change History (25)

#1 @birgire
5 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
5 months ago

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

@audrasjb
5 months ago

Toggle Select All button to Deselect All when clicked

#3 @audrasjb
5 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
5 months ago

47048.diff patch

#4 @birgire
5 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
4 months 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
4 months 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
4 months ago

#7 @afercia
4 months 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
3 months ago

@birgire
3 months ago

#8 @birgire
3 months 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 3 months ago by birgire (previous) (diff)

#9 @audrasjb
7 weeks ago

  • Keywords 2nd-opinion removed

Right, using a checkbox is the best option.

@birgire I tested your last patch and it's looking perfect to me :-)

Last edited 7 weeks ago by audrasjb (previous) (diff)

#10 @afercia
7 weeks ago

The checkbox seems to work pretty well :)

There's one think to address though. In the Search tab:

  • do a first search
  • Select All: all checkboxes get selected, so far so good
  • do a new search (in my case, the search term was "edge")
  • the new results appear and of course their checkboxes are unchecked
  • the Select All checkbox is still checked though

http://cldup.com/ld2v6nMC9E.png

@birgire
5 weeks ago

@birgire
5 weeks ago

#11 @birgire
5 weeks ago

Thanks for testing @audrasjb

Good catch @afercia

I've updated the patch in 47048-4.diff

The screenvideo in 47048-4.gif is based on the latest patch.

@birgire
5 weeks ago

Only unset the checked .select-all checkbox during searches, if it's already set.

#12 @audrasjb
11 days ago

I tested 47048-5.diff and it work fine on my side. Thanks for the refresh @birgire 👏

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


10 days ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 days ago

#15 @afercia
39 minutes ago

Reviewing a bit 47048-5.diff, noticed a few things:

  • Within attachTabsPanelListeners the variable selectAll is undefined: it needs to be declared at the top of the function. Please run grunt jshint to lint the JS files
  • While it is a partially pre-existing thing, could anyone please refresh my mind about the usage of removeAttr( 'checked' ) vs. prop( 'checked', false ) and prop( 'checked', true )? Genuinely asking.
  • I'd rename data-area to something along the lines of "data-items-type" or something like that to better clarify what it is.
  • Originally, the various "Select all" were links. It's a pattern that was commonly used in WordPress for no-js compatibility. Progressive enhancement: the basic functionality was available also whit JS off (see it was using a &selectall=1 param). Now that the "Select All" are checkboxes and their functionality is based on JavaScript, when JS support is off they're hidden with the CSS class hide-if-no-js. Personally I don't have objections to make this feature only available when JS is on but I guess the logic to check for $_REQUEST['selectall'] that's still in place should be removed?

@birgire thanks for taking care of this issue. Do you have the bandwidth to address these points in the next days? Thanks!

Note: See TracTickets for help on using tickets.