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

[Checkbox] "set checked" is triggering .change event #5777

Open
pbarney opened this issue Sep 11, 2017 · 9 comments
Open

[Checkbox] "set checked" is triggering .change event #5777

pbarney opened this issue Sep 11, 2017 · 9 comments

Comments

@pbarney
Copy link

pbarney commented Sep 11, 2017

Semantic UI's "set checked" fires a .change() trigger. This should not be the case according to the documentation

JSBin Example

Why this is bad: .click() doesn't work for keyboard interactions on the checkbox, alienating many users, including those with visual disabilities.

@awgv
Copy link
Member

awgv commented Sep 12, 2017

Hi @pbarney, what the documentation means is that set checked doesn’t trigger the component’s internal callbacks like onChange. I didn’t understand why set checked shouldn’t be triggering the change event, could you please elaborate?

@pbarney
Copy link
Author

pbarney commented Sep 12, 2017

Thanks for responding! Although, I don't agree with your interpretation of the documentation. What it says is:

Calling a behavior like check will trigger an elements callbacks, however using set checked will adjust the checkbox state without triggering callbacks

It doesn't say "component's internal callbacks" it says "an element's callbacks." It couldn't be clearer. This is why set checked doesn't trigger .click(). I think that it was simply an oversight that it triggers .change()

As an example:

Suppose you have a toggle checkbox indicating that a user is allowed to log in. But if the user has active projects, you aren't allowed to disable his login.

  1. A user clicks the "allow login" toggle checkbox.
  2. This fires a .change() event on the checkbox which does an asynchronous request from the back end to see if it is disallowed.
  3. If it is disallowed, we notify the user and then manually set checked the checkbox back to it's original state.
  4. But doing this fires the .change() event again, incorrectly repeating the async request.

.change() is preferrable to .click() for accessibility reasons, such as screen readers.

Please see the JSBin Example I linked to above for an implementation.

@awgv awgv added this to the Needs Milestone milestone Sep 12, 2017
@awgv
Copy link
Member

awgv commented Sep 12, 2017

@pbarney Thank you, the use case makes sense.

@awgv awgv changed the title Checkbox "set checked" is triggering .change event [Checkbox] "set checked" is triggering .change event Sep 12, 2017
@stale
Copy link

stale bot commented Feb 23, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 23, 2018
@pbarney
Copy link
Author

pbarney commented Feb 23, 2018

Does "stale" mean that this won't be addressed? Just curious.

@stale stale bot removed the stale label Feb 23, 2018
@stale
Copy link

stale bot commented May 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 24, 2018
@brodycj
Copy link

brodycj commented May 24, 2018

ping

@stale stale bot removed the stale label May 24, 2018
@neutraali
Copy link

neutraali commented Dec 4, 2018

We're controlling checkboxes manually, like a "reset to defaults" -type of scenario where we need to reset the state of several checkboxes at once. Ideally, we'd loop through the checkboxes and call an onChange -handler after that. Unfortunately, now the onChange -handler obviously triggers for each checkbox because the docs mislead us.

This is in addition to each 'set unchecked' -call as well, since we obviously need to uncheck all values that don't reflect the previous state.

So yeah, maybe the docs needs a better explanation or this could do with a fix? What is the difference between checked vs. set checked in the first place, for example, if onChange here isn't an "internal function"?

@lubber-de

This comment was marked as spam.

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

No branches or pull requests

5 participants