Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36267 closed defect (bug) (fixed)

Thickbox close button accessibility

Reported by: afercia's profile afercia Owned by: afercia's profile afercia
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-screenshots
Focuses: ui, accessibility Cc:

Description

Recently, [36964] changed the Thickbox button in a real button element but there's one more case where it's still a non-link link a href='#' that should be updated as well. Was not handled in #33305 because, as far as I can tell, core is not using "inline" content for Thickbox and was out of the scope of that ticket but plugins are using it and they get just an empty link with no content at all.
Screen readers will read it out a "link".
I'd propose to use the same markup already used for the other cases, making it a <button> element with some screen-reader-text inside.

Attachments (2)

36267.patch (1.3 KB) - added by afercia 9 years ago.
36267.2.patch (1.8 KB) - added by afercia 9 years ago.

Download all attachments as: .zip

Change History (9)

@aferciaCore Committer
9 years ago

#1 @aferciaCore Committer
9 years ago

  • Keywords has-patch added

In the proposed patch:

  • Thickbox: the "Close" button should (always) be a button

#2 @abrightclearweb
9 years ago

Applied patch successfully but it does not change the <a> tag to <button> on modals from the Installed Plugins and Add Plugins screens.

#3 @aferciaCore Committer
9 years ago

Hello @abrightclearweb and thanks for testing! Are you testing on trunk or latest stable version (4.4.2)?. The close "x" on the Installed and Add New Plugin screens is a button but just on trunk, for now :)

#4 @aferciaCore Committer
9 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to afercia
  • Status changed from new to assigned

#5 @aferciaCore Committer
9 years ago

  • Keywords has-screenshots added

Patch still applies, tested with a couple of plugins that use Thickbox and seems everything is OK. I'd propose to add just a small CSS change. In the screenshot below, a Thickbox example before the patch: notice the focus style around the "Close" link, it's the standard focus style for links

https://cldup.com/DqYC2ZgRaX.png

After the patch, the link will be a button and will lose the focus style that should be added again in thickbox.css,

@aferciaCore Committer
9 years ago

#6 @aferciaCore Committer
9 years ago

The focus style on the new button looks good, tested also on the Multisite Themes screen where Thickbox is used on the "view details" link when a Theme has available updates:

https://cldup.com/3zi-MJ_-8T.png

#7 @aferciaCore Committer
9 years ago

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

In 37531:

Accessibility: the Thickbox "Close" control should always be a button.

Fix the last case where the "Close" control was still a link. All the other ones
were already changed in buttons.

Fixes #36267.

Note: See TracTickets for help on using tickets.