fix: change event handling in checkbox and switch

[simonxabris]

🐛 Bug Report

Currently there's no way to determine whether the change event coming from a checkbox/switch was initiated by the user or programatically. Normally with events this could be done with the isTrusted property of en Event, but since the change is a custom event, isTrusted will always be false.

💻 Repro or Code Sample

You can see the described behaviour here on this stackblitz. Open the console and click the checkbox a few times.

🤔 Expected Behavior

The desired behaviour is that when a checkbox is clicked by a user, the change event that comes out has an isTrusted property set to true so that it's differentiable whether it was triggered by a user action or programmatically.

😯 Current Behavior

Currently all change events that come out of the checkbox have their isTrusted property set to false.

💁 Possible Solution

I don't currently know a good solution, maybe the click event could be "re-emitted" somehow because that is generated by a user, but I'd have to look into it more.

🔦 Context

An example scenario is that we have a Form with a couple of checkboxes, clicking a checkbox has some side-effect that is executed in the change event handler. When the form is submitted, but has some validation errors we reset the checkbox values, but we don't want the side-effect to run, we only want it to run on clicks by the user.

Another case is that we are creating a React wrapper around our FAST based components and the React-y way of change events on a checkbox is that the onChange handler runs when the checkbox is checked/unchecked by a user, but not when the checkbox's state is modified by code(not user action). This is currently hard to replicate, we have to listen to the click event on the checkbox and run the onChange handler there when appropriate, which is not the best solution.

🌍 Your Environment

This is a general problem happening everywhere.

[rajsite]

We ran into a similar issue where programmatically setting the checked value fired events. In general I don't expect programmatically setting the value to fire events (just expect them from user interactions). We can create a separate issue if that seems different enough from this issue. FYI @mollykreis

[chrisdholt]

Thanks all for this issue, agree this is a needed improvement. I'm going to do some investigation.

[simonxabris]

@chrisdholt I'm happy to do some digging and take some weight off your shoulders. If you have a direction in mind, I'll explore it!

[chrisdholt]

@chrisdholt I'm happy to do some digging and take some weight off your shoulders. If you have a direction in mind, I'll explore it!

Thanks, that would be very welcome!

[sknoslo]

@chrisdholt I am interested in working on a solution to this because it makes it a challenge to build something like a hierarchy of checkboxes where "child" checkboxes control the state of a "parent" checkbox and vice versa.

It seems pretty straightforward, just moving the event emitter out of the checkedChanged callback and instead emit that event in the mouse and keyboard handlers. This would give it parity with the native checkbox. The change would impact Switch, Checkbox and Radio. Unfortunately, it gets complicated with the RadioGroup component, because it listens for the change event on the individual Radios, which means it requires them to emit even if the change was not through user interaction.

Here are a couple solutions I've thought of:

1. Emit a different "internal" event on any change, and the RadioGroup could listen for that.
2. A Radio could directly call a method on the parent RadioGroup to let it know it was checked, if it belonged to a group.
3. The RadioGroup could register a callback with each child Radio, that would be called on any change (basically the same as 2, but inverting control).

I'd be interested to hear other ideas, or if one of these ideas is particularly good (or particularly bad). If there is a better forum for discussion on the details, let me know.

[rajsite]

Maybe a potential use case for using the W3C WC community context protocol being added to vNext?: #6053
A way to let the radio group respond to interactive changes from child radio buttons without relying on "user facing" events.

[chrisdholt]

@chrisdholt I am interested in working on a solution to this because it makes it a challenge to build something like a hierarchy of checkboxes where "child" checkboxes control the state of a "parent" checkbox and vice versa.

It seems pretty straightforward, just moving the event emitter out of the checkedChanged callback and instead emit that event in the mouse and keyboard handlers. This would give it parity with the native checkbox. The change would impact Switch, Checkbox and Radio. Unfortunately, it gets complicated with the RadioGroup component, because it listens for the change event on the individual Radios, which means it requires them to emit even if the change was not through user interaction.

Here are a couple solutions I've thought of:

1. Emit a different "internal" event on any change, and the RadioGroup could listen for that.
2. A Radio could directly call a method on the parent RadioGroup to let it know it was checked, if it belonged to a group.
3. The RadioGroup could register a callback with each child Radio, that would be called on any change (basically the same as 2, but inverting control).

I'd be interested to hear other ideas, or if one of these ideas is particularly good (or particularly bad). If there is a better forum for discussion on the details, let me know.

Thanks for this @sknoslo - I need to grok a bit more on what's going on and what's going wrong here. I think the main issue with moving event emission to keydown and click handlers is the fact that these can and also do need to change programatically. Definitely happy to have you work on this and take up helping to solve it. Which version are you working with for this just for my own understanding?

[sknoslo]

@chrisdholt well, I'm currently building a design system on the current stable version, so ideally whatever fix is decided on would be backported to that version as well as being fixed on master.

And if I could summarize the problem, it's that I do not expect a change event to be emitted when a checkbox (or radio or switch) is checked programmatically. So checkbox.checked = true; should not emit a change event. This is how native elements work, and it is how we would like the components in our design system to work. web.dev says it is a best practice to "...not dispatch events in response to the host setting a property (downward data flow)."

I think in most cases it is pretty harmless, but in the particular scenario I'm working on (a hierarchy of checkboxes) it would require some rather unfortunate workarounds. I could write up an example in Stackblitz or something if that would be helpful.

I should also note, as @rajsite pointed out a while back, there may be two separate issues here, but it sounds to me that the core of what @simonxabris was getting at is that there is no way to ignore events that happened because a property was set programmatically rather than through user interaction. And that would be solved by just not emitting the event, the same as native elements.

[chrisdholt]

@chrisdholt well, I'm currently building a design system on the current stable version, so ideally whatever fix is decided on would be backported to that version as well as being fixed on master.

And if I could summarize the problem, it's that I do not expect a change event to be emitted when a checkbox (or radio or switch) is checked programmatically. So checkbox.checked = true; should not emit a change event. This is how native elements work, and it is how we would like the components in our design system to work. web.dev says it is a best practice to "...not dispatch events in response to the host setting a property (downward data flow)."

I think in most cases it is pretty harmless, but in the particular scenario I'm working on (a hierarchy of checkboxes) it would require some rather unfortunate workarounds. I could write up an example in Stackblitz or something if that would be helpful.

I should also note, as @rajsite pointed out a while back, there may be two separate issues here, but it sounds to me that the core of what @simonxabris was getting at is that there is no way to ignore events that happened because a property was set programmatically rather than through user interaction. And that would be solved by just not emitting the event, the same as native elements.

Makes incredible sense - thanks for putting it so clearly as I'm coming back from some time off. :)

I think this would be a great improvement; perhaps we can start with figuring out a good "RFC" approach for radio group and how it can best support this pattern? That seems like the best way to unblock the work. If you're up for it, once we have a good plan of attack for that change I think this would be a welcome update. We should likely audit to ensure we make this change holistically beyond just these Checkable form associated elements.

[chrisdholt]

@simonxabris assigning you to this as well :)

[sknoslo]

@chrisdholt I think that sounds like a good plan. Just to be clear, are you suggesting that I create an RFC, or is that something the FAST team will do at some point? I could take a crack at it if that is what you were suggesting. I took a look at the contributor documentation but didn't see anything about RFCs. Is there any particular template or process that needs to be followed, or just file an issue with the details and suggestions?

[chrisdholt]

@chrisdholt I think that sounds like a good plan. Just to be clear, are you suggesting that I create an RFC, or is that something the FAST team will do at some point? I could take a crack at it if that is what you were suggesting. I took a look at the contributor documentation but didn't see anything about RFCs. Is there any particular template or process that needs to be followed, or just file an issue with the details and suggestions?

I don't think it needs to be formal necessarily - we could leverage this issue. I guess to be super clear, I'd love to have this as a contribution. My hope would be that we start w/ something like a "spec" so that our team can "sign-off" on the proposed new approach before we get too deep on implementing. I always feel bad when folks do a bunch of good work and then we need to provide some key feedback that requires a lot of "rework". The mini-spec/rfc can also provide a good paper trail for "why" we made certain choices, etc. To be clear, you can certainly jump right in to writing code if need be, but we prefer to have an idea of the proposed implementation written up to avoid a bunch of churn by folks :). Does that help?

[sknoslo]

I don't think it needs to be formal necessarily - we could leverage this issue. I guess to be super clear, I'd love to have this as a contribution. My hope would be that we start w/ something like a "spec" so that our team can "sign-off" on the proposed new approach before we get too deep on implementing. I always feel bad when folks do a bunch of good work and then we need to provide some key feedback that requires a lot of "rework". The mini-spec/rfc can also provide a good paper trail for "why" we made certain choices, etc. To be clear, you can certainly jump right in to writing code if need be, but we prefer to have an idea of the proposed implementation written up to avoid a bunch of churn by folks :). Does that help?

Very helpful, thanks. Sounds like a good plan. I'll start giving it some more thought.

[simonxabris]

@chrisdholt Sorry for disappearing here 😄 But this issue came up again in our design system as some teams are migrating from other libraries where checkable elements do behave like native checkboxes, radio input etc (not emitting events on programmatic changes). I've read the discussion I currently fail to see why the approach @sknoslo suggested couldn't work, I'm not sure what this

We should likely audit to ensure we make this change holistically beyond just these Checkable form associated elements.

means in this context as this is specifically an issue with the checkable form elements. Could you elaborate on this please? Maybe this time I could actually come up with something, if I understood this a bit better 😄.

[janechu]

Unfortunately @microsoft/fast-components has been deprecated for some time.