plone
Toolbar button a11y
https://github.com/plone/volto/issues/5118
- Fixed the low contrast of the toolbar expand/collapse button and added a hover effect.
- Added an additional dynamic text for the expand button to properly indicate both open and closed states.
https://github.com/user-attachments/assets/a792576e-8d02-491b-b8b7-c51bf04340cf
The label changes look good, if you could provide a screenshot of the style changes it would be awesome. Thanks!
@pnicolli Done
@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.
it could as well be global but that could also cause breaking changes.
@pnicolli I don't understand why you think this would be a breaking change. There's no visually-hidden class in Volto. I think since it's global, it could be reused. But isn't there already a class that does this?
There is also this PR #6356
@JeffersonBledsoe actually I think you are right, nice catch! @wesleybl adding any global class that can be a breaking change for users. If users have a different implementation of the same class we could break it. I'm ok with adding this since we are in v19 alpha but I would document this in the upgrade guide. Good pointing out that another PR exists, let's resurrect that, I'll leave a comment/review there!
@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.
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.
@sneridagh interesting fact, unrelated to the PR itself, I think I see an issue here: tests are green but it's a fake info. Tests should break, after the second to last commit, the one that I made to change the id, snapshots should break but then I merged main into this one and tests are green because it is not running all the tests, I think it is actually skipping those unit tests. In other words, merging this would break the tests on main. If what I understand is true, I think we should run all tests on CI.