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

Set the for attribute on the labeled-radio-button #22

Merged
merged 1 commit into from
Jul 8, 2015
Merged

Set the for attribute on the labeled-radio-button #22

merged 1 commit into from
Jul 8, 2015

Conversation

poteto
Copy link
Contributor

@poteto poteto commented Jul 8, 2015

This PR is to allow the checking of a radio input by clicking on the
label that has its input id as its for attribute.

@poteto
Copy link
Contributor Author

poteto commented Jul 8, 2015

Apologies for the failed builds and rebases. I believe the issue is with Travis being on Phantom 1.9, and there being an existing bug with clicking on labels not triggering their for inputs – see ariya/phantomjs#12032.

The tests pass fine locally on PhantomJS 2.0. I'll try and investigate to figure out a workaround...

This PR is to allow the checking of a radio input by clicking on the
label that has its input id as its `for` attribute.
@raycohen
Copy link
Contributor

raycohen commented Jul 8, 2015

I was under the impression that we did not need to put the for attribute on the label because by wrapping the input in the label they are implicitly associated. Does a unit test that triggers click on the label without these changes not work?

@poteto
Copy link
Contributor Author

poteto commented Jul 8, 2015

I updated Travis to use PhantomJS 2 – see travis-ci/travis-ci#3225

@raycohen The change is primarily so we can pass the id to the radio-button-input from both the labeled-radio-button and radio-button component. In our use case, we have it so the input is not nested inside of the label, and this PR allows that to work.

Having the for attribute on the label is also a requirement for accessibility.

@raycohen
Copy link
Contributor

raycohen commented Jul 8, 2015

Can we add an id to the nested input and still use the implicit association to bypass adding the for? That should allow associating non-wrapping label elements.

@poteto
Copy link
Contributor Author

poteto commented Jul 8, 2015

I'm curious why you would prefer not to have it on the label, explicitly adding the for attribute is recommended for accessibility, and we don't really lose anything by adding it. The id is being passed into the input in this PR.

@raycohen
Copy link
Contributor

raycohen commented Jul 8, 2015

This convinced me that the use of for is not always redundant (in our wrapped usage):

some assistive technologies do not correctly handle implicit labels

http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H44.html

Thanks!

raycohen added a commit that referenced this pull request Jul 8, 2015
Set the `for` attribute on the labeled-radio-button
@raycohen raycohen merged commit 5f85ad7 into yapplabs:master Jul 8, 2015
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.

2 participants