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

Add aria-pressed to the editor toolbar toggle buttons. #630

Merged
merged 2 commits into from
May 8, 2017

Conversation

afercia
Copy link
Contributor

@afercia afercia commented May 3, 2017

Fixes #628

@@ -28,7 +28,9 @@ function Toolbar( { controls } ) {
} }
className={ classNames( 'editor-toolbar__control', {
'is-active': control.isActive
} ) } />
} ) }
aria-pressed={ control.isActive ? 'true' : 'false' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just be { control.isActive }.

@afercia
Copy link
Contributor Author

afercia commented May 4, 2017

Now that I look back at that, I think the original intent was to have true and false as strings, because HTML attributes are strings. See also the aria-expanded for the inserter toggle. Minor point though.

@nylen
Copy link
Member

nylen commented May 4, 2017

React should handle this for us. It's worth checking what it's actually doing in the dom though. Is the absence of an aria-pressed attribute the same as false?

@afercia
Copy link
Contributor Author

afercia commented May 4, 2017

Is the absence of an aria-pressed attribute the same as false?

No because the presence of the attribute (even when false) make screen readers announce the button as toggle button instead of just button.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

aria-pressed

@nylen
Copy link
Member

nylen commented May 4, 2017

Thanks for checking, @aduth, I made those last few comments while mobile.

It would be good to circle back to this later and add tests to make sure we're rendering the attribute appropriately (see #641 (comment)).

For now, +1 for merging.

@jasmussen
Copy link
Contributor

I see approval here, so I'm merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants