Skip to content
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

Accessibility: Make text alignment items radio menu items. #19233

Merged
merged 6 commits into from Dec 19, 2019

Conversation

@MarcoZehe
Copy link
Contributor

MarcoZehe commented Dec 19, 2019

Description

Pass a role from the alignment toolbar to the dropdown menu. Make the dropdown menu aware of this role on each control and set it if present. Also set aria-checked on the active control if the role calls for it. Fixes #18721.

How has this been tested?

Screenshots

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
Pass a role from the alignment toolbar to the dropdown menu. Make the dropdown menu aware of this role on each control and set it if present. Also set aria-checked on the active control if the role calls for it. Fixes #18721.
MarcoZehe added 3 commits Dec 19, 2019
Copy link
Member

gziolo left a comment

It all makes sense. I guess I missed it completely when converting the alignment options to the dropdown. I think we have the same logic covered already in the MenuItem component, so it would be a good idea to use this component rather than the IconButton. The challenge here is that those block alignment toolbar (groups) support isCollapsed flag. When they aren't collapsed, they should be regular IconButton without the role applied. The easiest way to move forward would be to apply menuitemradio role conditionally and move forward with your proposal. What do you think?

@MarcoZehe

This comment has been minimized.

Copy link
Contributor Author

MarcoZehe commented Dec 19, 2019

It all makes sense. I guess I missed it completely when converting the alignment options to the dropdown. I think we have the same logic covered already in the MenuItem component, so it would be a good idea to use this component rather than the IconButton. The challenge here is that those block alignment toolbar (groups) support isCollapsed flag. When they aren't collapsed, they should be regular IconButton without the role applied. The easiest way to move forward would be to apply menuitemradio role conditionally and move forward with your proposal. What do you think?

Yes, that makes sense. I will add that bit, hoping it will pass. :-) However, I have a problem currently that the conditional role is not yet being applied to those icon buttons. So when I try this PR on gutenberg.run, it is still a normal menu item, and aria-checked is not set at all. So I am not sure why.

@gziolo gziolo merged commit 1de41d9 into WordPress:master Dec 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 19, 2019

Awesome work, congrats on your first code contribution to Gutenberg 🎉

@MarcoZehe MarcoZehe deleted the MarcoZehe:fix/18721 branch Dec 19, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@MarcoZehe MarcoZehe mentioned this pull request Jan 17, 2020
8 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.