fix(radio): update MatRadioButton disabled setter to trigger change detection #11056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
tinayuangao
reviewed
Apr 30, 2018
| @@ -410,7 +410,11 @@ export class MatRadioButton extends _MatRadioButtonMixinBase | |||
| return this._disabled || (this.radioGroup !== null && this.radioGroup.disabled); | |||
| } | |||
| set disabled(value: boolean) { | |||
| this._disabled = coerceBooleanProperty(value); | |||
| const newDisabledState = coerceBooleanProperty(value); | |||
There was a problem hiding this comment.
set disabled should trigger change detection with @Input()
There was a problem hiding this comment.
This does not appears to be the case. Demo: https://stackblitz.com/edit/angular-material-feiydh?file=app%2Fapp.component.html.
@input appears to cause for the setter to trigger change detection when tripped during template binding, but it doesn't appear to rewrite the setter implementation to always call change detection, so direct accesses to the component instance don't trigger change detection. Sometimes (e.g., when using ng-content), direct instance access is unavoidable, and there needs to be some way to disable a radio button. If there's a better way that I missed, let me know.
Potential reference (unresolved issue in Angular): angular/angular#20611
stevenyxu
force-pushed
the
master
branch
2 times, most recently
from
02587e5 to
5c47d56
Compare
stevenyxu
changed the title
stevenyxu
added a commit
to stevenyxu/components
that referenced
this pull request
May 1, 2018
…etection This is related to angular#11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
stevenyxu
added a commit
to stevenyxu/components
that referenced
this pull request
May 1, 2018
…etection This is related to angular#11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
… change detection Fixes MatRadioButton.disabled setter to trigger change detection. MatRadioButton uses ChangeDetectionStrategy.OnPush, which works fine with disabled is bound in a template that uses <mat-radio-button>. However, if a parent directly accesses the MatRadioButton component instance (e.g., using ViewChild, as in the provided test case), there is otherwise no way for the parent to trigger change detection in the MatRadioButton due to the OnPush strategy. OnPush was added in 97a9bdc, and this same technique was added to some but not all setters. This commit also adds test coverage for directly setting disabled on MatRadioButton in general, which previously was only indirectly covered in the MatRadioGroup tests.
|
I didn't mean to close this. Got my branches mixed up. |
tinayuangao
added
pr: lgtm
action: merge
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
tinayuangao
added
the
target: patch
josephperrott
pushed a commit
that referenced
this pull request
May 2, 2018
…etection This is related to #11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
josephperrott
pushed a commit
that referenced
this pull request
May 2, 2018
…etection This is related to #11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
josephperrott
pushed a commit
that referenced
this pull request
May 4, 2018
… change detection (#11056)
jelbourn
pushed a commit
that referenced
this pull request
May 8, 2018
…etection (#11098) This is related to #11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
jelbourn
pushed a commit
to jelbourn/components
that referenced
this pull request
May 14, 2018
…etection (angular#11098) This is related to angular#11056, which is a similar fix for MatRadioButton. The MatCheckbox component instance may be accessed directly by a parent component using ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property directly currently doesn't cause change detection leading to a stale or broken state. Accessing it through a template binding does correctly trigger change detection on MatCheckbox (there are already test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will never get kicked if disabled is set directly on the component instance. Related open Angular bug: angular/angular#20611
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes MatRadioButton.disabled setter to trigger change detection. MatRadioButton uses
ChangeDetectionStrategy.OnPush, which works fine with disabled is bound in a template that uses
. However, if a parent directly accesses the MatRadioButton component instance
(e.g., using ViewChild, as in the provided test case), there is otherwise no way for the parent to
trigger change detection in the MatRadioButton due to the OnPush strategy.
OnPush was added in 97a9bdc, and this same technique was added to some but not all setters.
This commit also adds test coverage for directly setting disabled on MatRadioButton in general,
which previously was only indirectly covered in the MatRadioGroup tests.
Related open Angular bug: angular/angular#20611