WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 8 weeks ago

#47048 closed defect (bug) (fixed)

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:
PR Number:

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 (11)

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

Download all attachments as: .zip

Change History (29)

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

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

@audrasjb
7 months ago

Toggle Select All button to Deselect All when clicked

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

47048.diff patch

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

#7 @afercia
6 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
5 months ago

@birgire
5 months ago

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

#9 @audrasjb
4 months 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 4 months ago by audrasjb (previous) (diff)

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

@birgire
3 months ago

#11 @birgire
3 months 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
3 months ago

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

#12 @audrasjb
2 months 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.


2 months ago

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


2 months ago

#15 @afercia
8 weeks 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!

#16 @birgire
8 weeks ago

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'].

@birgire
8 weeks ago

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


8 weeks ago

#18 @afercia
8 weeks ago

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

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.

Note: See TracTickets for help on using tickets.