MED-1960: group aria elements together on outer elements for VoiceOver #869
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
… up when component is disconnected
| super.connectedCallback(); | ||
| window.addEventListener('mouseup', () => { this._barUp(); }); | ||
| window.addEventListener('mousemove', (event) => { this._onTrack(event); }); | ||
| if (!this.label) { |
There was a problem hiding this comment.
For required properties we normally would try to use PropertyRequiredMixin. Here is an example were we use it: https://github.com/BrightspaceUI/core/blob/main/components/dialog/dialog.js#L50
| if (!this.label) { | ||
| console.error('d2l-labs-slider-bar requires a "label" property for accessibility'); | ||
| } | ||
| this.setAttribute('aria-label', this.label || 'slider'); |
There was a problem hiding this comment.
Can we apply these to the internal sliderContainer element? That would avoid the need to sprout them on the host, as well as avoid the need to sync them in updated.
…htspaceUI/labs into llefebvre/MED-1960-move-slider-role
| <div id="sliderContainer" | ||
| role="slider" | ||
| tabindex="0" | ||
| aria-label="${this.label}" |
There was a problem hiding this comment.
What is the purpose of the title attribute that's being set from the media-palyer? Is it to show the native browser tooltip on hover? If so, perhaps it can be applied here instead, so that it's not up to the consumer to do this. It looks like we're setting the same value for the label and the title, so we would already have the value we need here.
There was a problem hiding this comment.
It seems the best practise is to eliminate it if you already have aria-label, so I removed it. For sighted users, I believe the tooltip is redundant and therefore of little value.
Using both can cause some screen readers to announce the label twice, which creates a bad user experience.
The best practice is to use aria-label for the accessible name and consider an alternative, script-based tooltip solution if a visual tooltip is essential.
| aria-valuemin="${String(this.min)}" | ||
| aria-valuemax="${String(this.max)}" | ||
| aria-valuenow="${String(ariaValueNow)}" | ||
| @keydown=${this._onKeyPress} |
There was a problem hiding this comment.
Nit: I know the handler was already named this, but I'd normally name this _onKeyDown to align with the event... just to minimize any possible confusion reading the code in the future.
dbatiste
left a comment
dbatiste
left a comment
There was a problem hiding this comment.
Approval so I'm not blocking you. I'd probably do those last couple tweaks though.
|
🎉 This PR is included in version 2.42.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
MED-1960
Clean up accessibility properties for
media-playerslider-barelement.Summary of changes:
aria-*attributes from<d2l-labs-slider-bar>element inmedia-player.js.sliderBartosliderContainersliderContainerThe focus looks slightly different now that it is on the sliderContainer instead of the sliderBar. In the screenshot below, the new focus look is shown in the upper image. (The border of the slider bar is now more visible than before with a heavier line)
Tested keyboard navigation and keypress functionality in the demo application and in local instance of media-player (Media Library video preview and edit dialogs).