Skip to content

fix(checkbox, radio): click event is triggered once #27508

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

Closed
wants to merge 4 commits into from

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented May 18, 2023

Issue number: Resolves #27506


What is the current behavior?

Modern form controls (ion-radio, ion-checkbox) that associate a label to an input result in an additional click event, when the label is clicked.

e.g.:

<ion-checkbox onClick={() => console.log('clicked')}>
  #shadow-root
  <label>
    <input type="checkbox" />
  </label>
</ion-checkbox>

When the host is clicked, the label is the click target. When a label is associated to an input, it will trigger a click event on the control/input. This causes the event to bubble and a duplicate click event to fire.

What is the new behavior?

  • Prevent the event bubbling from the inner control when the label is clicked for ion-checkbox and ion-radio
  • Click events are emitted a single time for ion-checkbox and ion-radio

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev-build: 7.0.8-dev.11684439870.1ade4229

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label May 18, 2023
@sean-perkins sean-perkins marked this pull request as ready for review May 18, 2023 20:09
@sean-perkins sean-perkins requested a review from a team as a code owner May 18, 2023 20:09
// to the checkbox, which means when the label is clicked, the
// checkbox receives this event. To prevent a duplicate click event,
// we need to stop the event from bubbling.
ev.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pointer-events: none on the native element work here too? We fixed a similar bug in toggle a couple years ago: #23146

I'd prefer a CSS-only approach rather than adding even more JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pointer-events: none on the inner input control continues to result in duplicate clicks. The problem is the label invokes a click event on the input and does not take into consideration the pointer events.

@sean-perkins sean-perkins requested a review from liamdebeasi May 22, 2023 18:22
@liamdebeasi liamdebeasi changed the title fix(checkbox,radio): click event is triggered once fix(checkbox, radio): click event is triggered once May 22, 2023
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes, but do we know why this does not happen with ion-toggle?

@sean-perkins sean-perkins force-pushed the sp/duplicate-click-event branch from ade4229 to 37c9508 Compare May 31, 2023 19:05
@sean-perkins
Copy link
Contributor Author

Removed the tests that I originally introduced. This issue does not reproduce outside of a browser environment (an emulated environment does not result in 2x events).

This issue is not present in ion-toggle because of this line: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/toggle/toggle.tsx#L255. Removing it results in 2x click events.

@sean-perkins sean-perkins requested a review from mapsandapps June 1, 2023 16:03
@sean-perkins
Copy link
Contributor Author

@mapsandapps I'm adding you for a review as well, if you have availability before end of sprint. I wasn't able to take advantage of the same pointer-events: none solution on the label container as you were with ion-select. Curious to your feedback since you worked on a similar problem and if the proposed solution aligns with what you would expect.

@liamdebeasi
Copy link
Contributor

Closing as per #27506 (comment)

@brandyscarney
Copy link
Member

@liamdebeasi Was this PR supposed to be closed too?

@liamdebeasi
Copy link
Contributor

Whoops yes. Thanks!

@sean-perkins sean-perkins deleted the sp/duplicate-click-event branch July 10, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: ion-checkbox fires onclick twice
3 participants