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

Only change aria-pressed if it's not an input-based radio or checkbox group #22400

Merged
merged 3 commits into from
Apr 10, 2017
Merged

Only change aria-pressed if it's not an input-based radio or checkbox group #22400

merged 3 commits into from
Apr 10, 2017

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Apr 9, 2017

aria-pressed="true"/aria-pressed="false" is really only useful for making on/off toggles out of, say, <button> elements. the attribute is useless (and potentially confusing/conflicting) on, say, <label> elements for an existing <input type="radio"> or similar.

closes #22040

… group

aria-pressed="true"/aria-pressed="false" is really only useful for
making on/off toggles out of, say, `<button>` elements. the attribute is
useless (and potentially confusing/conflicting) on, say, `<label>`
elements for an existing `<input type="radio">` or similar.
Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

Maybe is it possible to add a unit test here ?

@patrickhlauke
Copy link
Member Author

@Johann-S ah yes, good call on unit tests. will get on that this morning

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Apr 10, 2017

@Johann-S mind having a look over the unit test i just added? admittedly, i'm not so hot on qunit nor clean jquery, so likely i'm going about this in an unnecessarily convoluted way.

however, i have run the unit test against the current v4-dev, where it fails (because current v4 DOES add those aria-pressed attributes to the <label> elements), and obviously against the modified code in this PR, where the tests pass (since i'm suppressing the aria-pressed for those cases).

@Johann-S
Copy link
Member

It seems you let some trailing spaces see : https://travis-ci.org/twbs/bootstrap/jobs/220509346#L220

But your unit test is fine 👍

@patrickhlauke
Copy link
Member Author

@Johann-S cool. trailing spaces killed. will go ahead and merge. thank you :)

@patrickhlauke patrickhlauke removed the request for review from bardiharborow April 10, 2017 12:43
@patrickhlauke patrickhlauke merged commit 3f6e1fa into twbs:v4-dev Apr 10, 2017
@mdo mdo mentioned this pull request Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants