-
-
Couldn't load subscription status.
- Fork 800
Toolbar button a11y #7500
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
base: main
Are you sure you want to change the base?
Toolbar button a11y #7500
Conversation
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 label changes look good, if you could provide a screenshot of the style changes it would be awesome. Thanks!
@pnicolli Done |
|
@pnicolli Test passed |
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.
I think it's probably ok, I'm not fully convinced about having the visually-hidden class nested in there in the css code, it could as well be global but that could also cause breaking changes. I would love a couple more eyes from @plone/volto-team on this regard please.
|
@Wagner3UB We should probably set 'aria-controls' on the button too and have its ID set as the toolbar to make it clear that the thing that is expanded isn't the button itself. |
@pnicolli I don't understand why you think this would be a breaking change. There's no There is also this PR #6356 |
Other interesting things to be added were pointed out
|
@JeffersonBledsoe actually I think you are right, nice catch! |
|
@JeffersonBledsoe Thank you! I will manage to have that too. @pnicolli @wesleybl The global insertion of this class was already discussed within the A11Y team, and we all agreed it was an excellent idea. I overlapped Jeff’s PR even though I was aware it already existed and was nearly complete, since I had been in contact with him and he mentioned he was having some difficulties deciding the best approach and location to define the variable in order to cover all possible cases. |
|
@JeffersonBledsoe @pnicolli I added the aria-controls even though it may serve only a semantic purpose and not really as a functional instrument. The way I had implemented it before, using aria-live with translations to announce the states, mentioning the toolbar, and including aria-expanded, was already sufficient according to several sources I found. Still, I added it anyway to make sure accessibility testing tools would not flag its absence. |
Makes perfect sense to me 👍 Let's maybe try to merge the other PR so we can then use the global class here. |
#5118
Screen.Recording.2025-10-15.at.12.07.20.mov