Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#35576 closed defect (bug) (fixed)

Menus the "Select All" link should be hidden in the "quick search" tab

Reported by: afercia's profile afercia Owned by: sayedwp's profile sayedwp
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots good-first-bug has-patch
Focuses: ui, accessibility, javascript Cc:

Description

In the Menus screen, when users click on the "Search" tab the "Select All" link is still visible and can be activated, but there's nothing to select:

https://cldup.com/sUcFAHo6OA.png

Should probably be hidden when the Quick Search tab is the active one, would probably need a CSS class set on the container.

Additionally, when JavaScript is on, the "Select All" link should have a role="button" because it behaves like a button, while when JavaScript is off it behaves like a link. It would be as easy as adding the aria-button-if-js CSS class introduced in [35947]

Attachments (4)

35576.diff (2.4 KB) - added by sayedwp 9 years ago.
35576-2.diff (1.6 KB) - added by sayedwp 9 years ago.
35576-3.diff (1.7 KB) - added by sayedwp 9 years ago.
35576.2.diff (2.6 KB) - added by afercia 8 years ago.

Download all attachments as: .zip

Change History (21)

#1 @afercia
9 years ago

  • Component changed from General to Menus

@sayedwp
9 years ago

#2 @sayedwp
9 years ago

  • Keywords has-patch added; needs-patch removed

#3 follow-up: @tywayne
9 years ago

https://cldup.com/LVSJZ3Xdik.gif

Applied patch and tested. Partially worked for me, but doesn't handle switching between tabs as shown.

Additionally, as feedback on the patch itself, I think this could be done much more simply. Shouldn't need to add a new function, but rather toggle a CSS class, or use js with show() / hide() in the existing processQuickSearchQueryResponse function, and the check for items and toggle accordingly in the attachTabsPanelListeners to catch switching between tabs.

#4 in reply to: ↑ 3 @afercia
9 years ago

Replying to tywayne:

Additionally, as feedback on the patch itself, I think this could be done much more simply. Shouldn't need to add a new function, but rather toggle a CSS class, or use js with show() / hide()

I'd agree it could be done in a simpler way, preferably toggling a CSS class and avoiding show() / hide()

#5 @sayedwp
9 years ago

@afercia and @tywayne you are right. I will attach a new patch.

@sayedwp
9 years ago

#6 @sayedwp
9 years ago

I agree with @afercia for toggling a css class instead of .show()/.hide() however I did not find .hide or any similar class for hiding an element in WordPress admin. We can write a little css for this case but why cannot we add a .hide class so it can be useful for anyone who does not want to use jQuery .show()/.hide()

Last edited 9 years ago by sayedwp (previous) (diff)

#7 @afercia
9 years ago

@sayedwp there's a .hidden class in wp-admin/css/common.css but probably you would need some CSS class to set on a parent container.

#8 @sayedwp
9 years ago

@afercia Thanks, yes you are right .hidden will not work here, I will upload a new patch.

@sayedwp
9 years ago

#9 @abrightclearweb
9 years ago

Tested patch 35576-3.diff.

The Select All link did not disappear when I clicked on the Search link with the patch applied.

GIF attached.

https://cldup.com/BFO-iE2xko.gif

#10 @sayedwp
9 years ago

@abrightclearweb The patch works, it just needs to go to their respective minified files.

https://cldup.com/qkLuEX9IlO.gif

Last edited 9 years ago by sayedwp (previous) (diff)

#11 @DrewAPicture
9 years ago

  • Owner set to sayedwp
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

See 35576-3.diff

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


8 years ago

#13 @afercia
8 years ago

  • Milestone changed from Awaiting Review to 4.7

Let's try this for 4.7 :)

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


8 years ago

#15 @helen
8 years ago

  • Keywords needs-refresh added

Patch needs a little coding standard/whitespace rework - anybody care to refresh that and/or test some more?

@afercia
8 years ago

#16 @afercia
8 years ago

  • Keywords needs-refresh removed

Refreshed patch and added the aria-button-if-js CSS class on the "Select All" links. However, in order to properly test, the format of the search results should be fixed in #33742, right now they're returned as links instead of checkboxes and labels:

trunk:

https://cldup.com/pXIHcdZPOX.png

4.6

https://cldup.com/oT_4Z-t8DD.png

#17 @ocean90
8 years ago

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

In 38754:

Menus: Hide controls in the search tab if no items are found.

Props sayedwp, afercia, tywayne, abrightclearweb.
Fixes #35576.

Note: See TracTickets for help on using tickets.