-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: accessibility issue of device preview options #63958
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thanks for contributing! However, this change won't solve the problem in question. Adding an aria-label means that now the accessible name won't match the visible label. The intention of the ticket is to ensure that whichever of the options is currently selected is announced differently from the unselected items. Possible solutions could include providing an accessible name for the checkbox (e.g., "selected"); adding a visible text label such as Desktop (selected); or adding an attribute like |
@joedolson updated video referenceScreen.Recording.2024-07-26.at.11.03.46.mov |
Thanks for the PR @up1512001! I think this UI should be implemented using the The 'Visual Editor' / 'Code Editor' options are an example of that component being used: |
@talldan replaced reference videoScreen.Recording.2024-07-28.at.00.33.50.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback. I've left one further comment, then this should be good to ship!
/** | ||
* Handles the selection of a device type. | ||
* | ||
* @param {string} value The device type. | ||
*/ | ||
const onSelect = ( value ) => { | ||
setDeviceType( value ); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes so far look good. Right now voiceover announces that the menu item is ticked on focus which is a big improvement over what's in trunk:
But when changing the option (e.g if I select 'Tablet' now) there's no announcement that the option was changed.
So another good accessibility improvement would be to add a spoken message when an option is selected (using the speak
function from the a11y
package). This would let screenreader users know that something happened when they change the option.
There's an example here of how the visual/code editor mode option does this:
speak( __( 'Visual editor selected' ), 'assertive' ); |
Something similar could be done for the device previews, either as part of this onSelect
function or within the setDeviceType
action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talldan added speak
on the device selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @up1512001, testing well, code looks good, this looks ready to merge
if ( value === 'Desktop' ) { | ||
speak( __( 'Desktop selected' ), 'assertive' ); | ||
} else if ( value === 'Tablet' ) { | ||
speak( __( 'Tablet selected' ), 'assertive' ); | ||
} else { | ||
speak( __( 'Mobile selected' ), 'assertive' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! These announcements shouldn't be necessary. Could you remove these in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeryj Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What?
aria-label
to desktop, tablet, mobile & preview links.Why?
Fixes #63910
Testing Instructions
Screenshots or screencast
Screen.Recording.2024-07-26.at.01.25.23.mov