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

[birgire]

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

[birgire]

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

[audrasjb]

Toggle Select All button to Deselect All when clicked

[audrasjb]

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

[audrasjb]

47048.diff patch

[birgire]

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.

[afercia]

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

[afercia]

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

• 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]

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]

Thank you for the thorough review and the patch @afercia

I like your checkbox suggestion, it feels like a much a better user experience, after having worked with it for the last hour :-)

In patch 47048-3.diff​ I looked at this part:

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

and 47048-3.diff.gif​ shows the testing.

[audrasjb]

Right, using a checkbox is the best option.

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

[afercia]

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

[birgire]

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]

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

[audrasjb]

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

[afercia]

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!

[birgire]

Thanks for a great review @afercia

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

Thanks for the headup. Usually phpcs and jshint are part of my workflow, but sometimes things slip by :-)

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.

Good question. According to:

​https://jquery.com/upgrade-guide/3.0/#breaking-change-removeattr-no-longer-sets-properties-to-false

Breaking change: .removeAttr() no longer sets properties to false
Prior to jQuery 3.0, using .removeAttr() on a boolean attribute such as checked, selected, or readonly would also set the corresponding named property to false. This behavior was required for ancient versions of Internet Explorer but is not correct for modern browsers because the attribute represents the initial value and the property represents the current (dynamic) value.

It is almost always a mistake to use .removeAttr( "checked" ) on a DOM element. The only time it might be useful is if the DOM is later going to be serialized back to an HTML string. In all other cases, .prop( "checked", false ) should be used instead.

47048-6.diff​ replaces the .removeAttr( "checked" ) instances with .prop( "checked", false ) .

I'd rename data-area to something along the lines of "data-items-type" or something like that to better clarify what it is.

Done.

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 $_REQUESTselectall? that's still in place should be removed?

47048-6.diff​ replaces that logic regarding $_REQUEST['selectall'].

[afercia]

In 46240:

Accessibility: Menus: Improve the menu items "Select All".

• changes "Select All" from a link to a checkbox
• the new checkbox is available only when JavaScript support is on
• semantically and for accessibility, a checkbox is a better user interface control because the available action is clear to all users and the selected state is communicated natively
• it's consistent with the existing pattern for the admin tables

Props birgire, audrasjb, afercia.
Fixes #47048.