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

fix(radio-button): Radio buttons are not tab stops in Safari #436

Closed
wants to merge 5 commits into from
Closed

fix(radio-button): Radio buttons are not tab stops in Safari #436

wants to merge 5 commits into from

Conversation

trik
Copy link
Contributor

@trik trik commented May 13, 2016

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label May 13, 2016
@trik
Copy link
Contributor Author

trik commented May 13, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels May 13, 2016
@robertmesserle
Copy link
Contributor

Thanks for your contribution! However, this issue was fixed as part of this PR: #415

@trik
Copy link
Contributor Author

trik commented May 13, 2016

@robertmesserle what about this one? d56946a

sorry, i had to make a different pr, it refers #327

@robertmesserle
Copy link
Contributor

@trik We do still need a fix for this, but when I tried applying that commit to master and running the tests, it doesn't appear to fix the bug. Were there other changes in one of the other commits required to make this work?

@trik
Copy link
Contributor Author

trik commented May 13, 2016

@robertmesserle no, only that one.. why it doesn't fix? in then unit test i set a value for the radio group model and i get one button selected, then i set the model to null and all buttons are de-selected

@robertmesserle
Copy link
Contributor

The failure I get is on the test you added. It needed the following line:

this._radios.forEach(radio => radio.checked = false);

Without that, it never actually unchecked any of the child radio buttons.

@robertmesserle
Copy link
Contributor

Ah, I see why your tests weren't failing. Your tests were never running.

There was a bug in the radio button tests where fakeAsync was wrapped around a function, which returns a new function - but that new function is never called. So your tests would have passed regardless of what they checked. (This was an issue with all tests in radio-button and is being fixed now by @jelbourn).

There were some other changes required to make this work as well. I'll open a PR with the working changes and mention you in it.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants