Skip to content

avoid preventing input event onclick#30992

Merged
XhmikosR merged 4 commits intotwbs:v4-devfrom
Lausselloic:fix-label-input-onclick
Jun 10, 2020
Merged

avoid preventing input event onclick#30992
XhmikosR merged 4 commits intotwbs:v4-devfrom
Lausselloic:fix-label-input-onclick

Conversation

@Lausselloic
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

XhmikosR commented Jun 9, 2020

Do we need a PR against master too?

@Lausselloic
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@XhmikosR
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@patrickhlauke
Copy link
Copy Markdown
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