Skip to content
This repository has been archived by the owner on May 20, 2023. It is now read-only.

material-checkbox emits wrong event when binding both indeterminate and checked inputs. #434

Open
whesse opened this issue Sep 5, 2019 · 2 comments

Comments

@whesse
Copy link

whesse commented Sep 5, 2019

I have found a bug where binding two inputs of material-checkbox gives an event stream with a false event on it. If the checked and indeterminate inputs are changed from (false, true) to (true, false), then the stream "change" emits a "false" event indicating the state (false, false) of the two properties. I am hitting this in an actual case, and can't find a workaround for it. Note that it is invalid to set both of these properties to true, but I am not doing this - the case I am hitting is a valid transition between two states (from "mixed" to "checked").

Looking at the source code, setting the [indeterminate] input property on a material-checkbox will always set [checked] to false, even if the indeterminate internal variable was already false.
This shouldn't usually happen, since only changed inputs are sent, but it could cause problems if both "[checked]" and "[indeterminate]" are changed in the same update cycle.

This causes a problem when both the [checked] and [indeterminate] input properties are bound on this component. If the two inputs change from {checked: false, indeterminate: true} to
{checked: true, indeterminate: false}, then the component's behavior depends on the order in which the two setters are called. The final state, and the checkedChange and indeterminateChange streams, are the same whichever order the setters are called in, but the 'change' stream will output two events: 'false', 'true' if the indeterminate setter is called first, but only one event: 'true' if the checked setter is called first.

This means I cannot distinguish between my internal logic setting a checkbox from mixed to checked using the input properties, and a user action setting that checked checkbox to unchecked.

The article about designing components suggests

"Intercept input property changes with ngOnChanges()
Detect and act upon changes to input property values with the ngOnChanges() method of the OnChanges lifecycle hook interface.
You may prefer this approach to the property setter when watching multiple, interacting input properties."

I need to listen to these streams to detect user actions. The checkedChange stream which doesn't have the problem does not distinguish between a box changing from checked to indeterminate and a box changing from checked to unchecked.

A workaround for this is that the model class for this view component can track the last received ARIA state from the "change" stream, and until it receives a state from that stream that agrees with the state it has internally, it will ignore other state messages. So since I have changed my model state from mixed to checked, it will ignore the "unchecked" event that comes before the "checked" event.

Additional comments on the documentation:
The documentation for checkedChange Stream says:
Fired when checkbox is checked or unchecked, but not when set indeterminate. Sends the state of checked.

This is misleading, because when setting indeterminate changes checked from true to false, an event is emitted on this stream.

The field indeterminateToChecked bool says:
Determines the state to go into when indeterminate state is toggled.

This field is only used by the UI toggle event, not by setting indeterminate to false.

@whesse
Copy link
Author

whesse commented Sep 6, 2019

If you want me to create a fix CL I can. This would replace the @input setters with an ngOnChanges hook. I don't see any alternative design, that would keep the same API with a boolean checked input.

@TedSander
Copy link
Contributor

First off thanks for reporting. This code definitely assumes you are only going to change one input at a time which is an invalid assumption.

I do think moving to ngAfterChanges may work here (ngOnChanges was removed for performance reasons.) It is going to make the component a bit more complicated because you'll have to save away the new values (and the previous.) Hopefully no one is relying on a weird edge case of the old logic.

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

No branches or pull requests

2 participants