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

avoid preventing input event onclick #30992

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

Lausselloic
Copy link
Contributor

instead of stopping event if onclick is triggered on input, call toggle method only if its not on checkbox inside a label

fix #30849 following regression introduced by #30388

…le method only if its not on checkbox inside a label
@Lausselloic Lausselloic requested a review from a team as a code owner June 9, 2020 14:23
@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2020

@Lausselloic thanks! Would it be possible to add a test case? Because this wasn't caught there.

@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2020

Do we need a PR against master too?

@Lausselloic
Copy link
Contributor Author

I don't make the initial PR to the master branch, so I need to PR the initial fix and that one to master

@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2020

OK, if it applies to master too yeah. But we need a test case for both either way to make sure this doesn't happen again :)

@Lausselloic
Copy link
Contributor Author

unit test added, need to port patrick fix (24abed1) and my PR's to master

@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2020

Are you sure the unit test covers the opposite too?

Thanks for the patches, and waiting for the master PR later :)

@Lausselloic
Copy link
Contributor Author

no it just test if event is propagate to the input wrong unit test title. I could add one to ensure event is also emit on label (I will update my PR)

@patrickhlauke
Copy link
Member

assume this was tested thoroughly to ensure keyboard triggering still works properly? I seem to remember the prevent stuff was necessary to make it work for kbd while also stopping Firefox from double-triggering on mouse interaction.

@XhmikosR
Copy link
Member

I tested as good as I could, but please confirm too.

@patrickhlauke
Copy link
Member

checked https://deploy-preview-30992--twbs-bootstrap.netlify.app/docs/4.5/components/buttons/#checkbox-and-radio-buttons in chrome, firefox and even IE11 ... all seems to be working fine. good stuff!

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

Successfully merging this pull request may close these issues.

3 participants