ToggleButton: indicate toggle button state on underlying button elements #438
Conversation
… elements Because the accessibility object model (AOM) is still in active development and doesn't enjoy wide browser vendor adoption at present, we need to maintain the accessibility for assistive technologies in the underlying native elements rather than the custom element host containers. For things like buttons, this often means adding a property that gets rendered onto the underlying native element as an `aria-` attribute.
daneah
requested review from
chrisjbrown,
SMQuazi and
jialin-he
and removed request for
a team
🦋 Changeset detectedLatest commit: e1df0a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| @@ -17,6 +17,8 @@ export type ButtonType = 'button' | 'submit' | 'reset'; | |||
|
|
|||
| export type ButtonVariant = 'primary' | 'secondary' | 'subtle' | 'overlay'; | |||
|
|
|||
| export type PressedState = 'false' | 'true' | 'mixed' | 'undefined'; | |||
There was a problem hiding this comment.
Curious what state is a "mixed" state?
There was a problem hiding this comment.
Have a read of the docs for aria-pressed
| * @attr value | ||
| */ | ||
| @property({ type: String, reflect: true }) | ||
| public pressed: PressedState = 'undefined'; |
There was a problem hiding this comment.
should this be set to the string undefined?
There was a problem hiding this comment.
Yes, it's one of the PressedStates. Have a read of the docs for aria-pressed; this value means "I'm not a toggle button" by default.
There was a problem hiding this comment.
I was able to validate this update and confirm the expected behavior with VoiceOver on MacOS in Firefox and Safari and on Windows with NVDA and Chrome/Firefox/Edge.
However, this appears to have introduced a bug in Chrome (Version 97.0.4692.71) with VO where none of the toggle buttons have a calculated name and are just announced as "Toggle button" along with current state.
|
@sirrah-tam Thanks for looking! Interestingly, I observed Chrome working with VO on my end 🤔 |
|
Hmm, it appears that my installation of Chrome on my work laptop has been kept back by some org settings. Tested this on my personal laptop and can confirm its working as expected. Tested on Chrome v 107.0.5304.87. |
* develop: Tabs: fix initial tab selection in more cases (#449) Storybook: add eslint-plugin-storybook and fix issues (#446) feat(storybook): install Measure and Outline addons (#445) Docs: update copy for Button's on-background usage (#444) Storybook: Convert stories to CSF 3 (#435) feat(tabs): fire event when tab selected programmatically (#442) ToggleButton: indicate toggle button state on underlying button elements (#438) Tooltip, DropdownMenu: replace popperjs with floating-ui (#434) Site: improve imports (#433)
|
@daneah - One side effect of the change we made to this is that all Pharos buttons now have the aria-pressed="undefined" attribute added by default. While this isn't necessarily problematic at first, it has shown a bit of inconsistency across some browser & AT combinations. While testing with VoiceOver on Safari, the presence of that attribute makes the button be parsed as a "toggle button" and is announced as such even though I don't believe that's the intention. Users will expect certain behavior if its announced as that type of button control, and could become confused if it does something different. |
|
@sirrah-tam can you please open a new issue? |
…nts (ithaka#438) * fix(toggle-button): indicate toggle button state on underlying button elements Because the accessibility object model (AOM) is still in active development and doesn't enjoy wide browser vendor adoption at present, we need to maintain the accessibility for assistive technologies in the underlying native elements rather than the custom element host containers. For things like buttons, this often means adding a property that gets rendered onto the underlying native element as an `aria-` attribute. * fix(toggle-button): address type imports and add unit tests * chore: add changeset
This change: (check at least one)
Is this a breaking change? (check one)
Is the: (complete all)
What does this change address?
Resolves #430
Because the accessibility object model (AOM) is still in active development and doesn't enjoy wide browser vendor adoption at present, we need to maintain the accessibility for assistive technologies in the underlying native elements rather than the custom element host containers.
How does this change work?
For things like buttons, this often means adding a property that gets rendered onto the underlying native element as an
aria-attribute. Here, I add apressedproperty to Button so that the ToggleButton can set it appropriately, which gets rendered asaria-pressedon the underlying<button>element. I also add agroup-labelprop to ToggleButton that renders thearia-labelfor the full button group, useful for consumers to add the most contextualized description they can for each usage.Additional context
The approach here is a generally desired one for optimal accessibility, and not unique to this particular component. Some of these may prove more difficult to manage, such as getting the right
aria-describedbyvalue down into the native element for any and every component that might need a tooltip. One approach could be for the tooltip to attach itself to the outermost shadow DOM element, but we'll need to take stock to see how future-proof that'd be.