angulardart / angular_components

The official Material Design components for AngularDart. Used at Google in production apps.
https://pub.dev/packages/angular_components
374 stars 123 forks source link

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

Open whesse opened 4 years ago

whesse commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.