Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#34484 closed defect (bug) (fixed)

WP oEmbed: Sharing dialog accessibility improvements

Reported by: afercia's profile afercia Owned by: afercia's profile afercia
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch commit
Focuses: ui, accessibility, javascript Cc:

Description

When a WordPress oEmbed gets rendered in a page, it has a button to open a "Sharing dialog" with options to share the embedded post. The dialog's content is split in two "tabs", happy to see this because it's maybe the first working example of an ARIA tabpanel widget in WordPress.

The dialog itself and a couple of things about the tabs can be further improved for accessibility. First thing I've noticed, the tab panels are "labelled" by the tabs but one of the aria-labelledby has a value that refers to a non-existing ID:

https://cldup.com/67bIa5ubaj.png

Not a big deal, just needs to be changed and use the right value but testing with Firefox + NVDA I've noticed a weird thing: the tabs are always announced as "1 of 1" and I really have no idea where that "property page" being announced comes from:

https://cldup.com/TtZCJHu0q9.png

Honestly, I have no idea why Firefox/NVDA fail here. Since the tab panels content is made by a form input and a textarea, users land directly inside them and there isn't the need to "label" the panels. I'd propose to remove the IDs and the aria-labelledby attributes. Looks like this fixes the weird Firefox/NVDA behavior.

Additional improvements to consider:

  • add role="dialog" and an aria-label attribute to the dialog
  • associate input/textarea fields with their descriptions using aria-describedby
  • give initial focus to the current Tab, this may need some JavaScript optimization but the issue here is the current Tab needs to be checked each time
  • buttons with a type="button" don't need preventDefault() because there's no default action to prevent
  • constrain tabbing within the modal dialog

Attachments (4)

34484.patch (5.3 KB) - added by afercia 8 years ago.
34484.2.patch (5.4 KB) - added by swissspidy 8 years ago.
34484.3.patch (5.4 KB) - added by swissspidy 8 years ago.
Changed selector in openSharingDialog()
34484.4.patch (5.2 KB) - added by afercia 8 years ago.

Download all attachments as: .zip

Change History (12)

@afercia
8 years ago

#1 @afercia
8 years ago

  • Keywords has-patch added

First pass. The JavaScript part may need some optimization, as mentioned above. In the screenshot below, the NVDA Speech Viewer showing how the dialog content gets read out with the patch applied. Notice:

  • users are now informed they're in a "dialog"
  • initial focus goes to the current Tab
  • switching tabs with arrow keys announce them correctly as "1 of 2" and "2 of 2"
  • when the input and textarea get focus, their descriptions get read out
  • tabbing is constrained within the dialog

https://cldup.com/22ZiLJ22B3.png

#2 @swissspidy
8 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.4

Looks good to me so far! Will have a closer look later on. Definitely something we should do in 4.4. Now it's still easy to make changes, especially when it's about something as important as accessibility.

@swissspidy
8 years ago

#3 @swissspidy
8 years ago

In my tests keyboard navigation worked fine. 34484.2.patch just includes some small updates with renamed class names and different wording for the comment.

@peterwilsoncc Are you able to do some IE testing using Browserstack? I don't see a reason it shouldn't work there, but you never know :-)

#4 follow-up: @afercia
8 years ago

Noticed one thing: openSharingDialog should not assume the current Tab is the first one. That's why in the first patch I used:

document.querySelector( '.wp-embed-share-tab-button [aria-selected="true"]' ).focus();

Consider users can open, close and re-open the dialog multiple times, for example:

  • open the dialog
  • switch to the second Tab
  • press Escape, the dialog closes
  • open the dialog again
  • at this point the current Tab is the second one

so it made sense for me to re-get the current Tab each time when opening the dialog :)

Last edited 8 years ago by afercia (previous) (diff)

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


8 years ago

@swissspidy
8 years ago

Changed selector in openSharingDialog()

#6 in reply to: ↑ 4 @swissspidy
8 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to afercia
  • Status changed from new to assigned

Replying to afercia:

Consider users can open, close and re-open the dialog multiple times, so it made sense for me to re-get the current Tab each time when opening the dialog :)

I was more thinking that it makes sense to "start fresh" when opening the dialog again, but after further testing it with a screen reader (I used ChromeVox) your approach (and as it is now) makes more sense. 34484.3.patch changes the selector again.

ChromeVox read the actions correctly when opening and navigating inside the sharing dialog ("tab 1 of 2 selected", etc.). It was an interesting experience btw :)

Im my opinion this is good to go.

@afercia
8 years ago

#7 @afercia
8 years ago

Refreshed patch after [35472].

#8 @afercia
8 years ago

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

In 35492:

WP oEmbed: Improve the Sharing dialog accessibility.

Improves ARIA attributes, focus handling, and constrains tabbing within the modal dialog.

Fixes #34484.

Note: See TracTickets for help on using tickets.