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

feat(forms): add support for disabled controls #10994

Merged
merged 1 commit into from Aug 24, 2016

Conversation

Projects
None yet
7 participants
@kara
Contributor

kara commented Aug 22, 2016

In HTML5 forms, controls marked as disabled are not considered when calculating the validity or serialized value of the parent form. However in Angular's current forms API, disabled controls still behave like normal controls and are included in all value/validity checks.

This is confusing because it differs from the HTML5 spec, but it's also confusing because it creates situations where a disabled control can make the whole form invalid, and worse, it's not editable in the UI so this situation cannot be remedied without re-enabling the control.

Changes:

We are adding a new status to controls called "DISABLED". A control can either be:

VALID: control has passed all validation checks
INVALID: control has failed a validation check
PENDING: control is in the midst of conducting a validation check
DISABLED: control is exempt from validation checks

These statuses are mutually exclusive. We are also adding disabled and enabled getters to AbstractControls.

When you disable a control, it's not included in parent validation or value serialization. So if your disabled control is invalid, the parent form can still be valid if it has other valid controls. Group values will also omit the values of disabled controls. Group status is always reduced from the statuses of its children, so if all a group's children are disabled, the group is disabled.

Reactive forms

Using reactive forms, form controls are disabled in the component class. When you disable a control, the disabled attribute is added for you in the DOM (so no need to do this yourself).

You have a few choices for how to disable a control. If you want the control to be disabled from the start, you can pass a boxed value as the first arg when you instantiate your FormControl. This boxed value contains all the form state that cannot be calculated (as validation state, dirtiness, and touched are all derived).

form = new FormGroup({
   'first': new FormControl({value: 'Nancy', disabled: true}, Validators.required),
   'last': new FormControl('Drew', Validators.required)
})

Note that if you don't care to set disabled state, you can just pass in a value without the wrapper object like before.

You can also imperatively enable or disable controls with the enable() and disable() methods:

this.form.disable();                   // disables itself and all children
this.form.get('first').disable();      // disables just this control

Angular 1 style (template-driven) forms

To disable a control in Angular-1 style forms, just add the disabled attribute or bind to the disabled property. The control will be disabled for you by ngModel under the hood.

<form>
   <input name="first" ngModel>
   <input name="last" ngModel required [disabled]="isDisabled">
</form>

Fixes #4460.

Breaking change: this PR also removes the deprecated optionals API, which has significant overlap with the new API. For more information about that and the changes in this PR, there's more info in the proposal doc.

@vsavkin

This comment has been minimized.

Show comment
Hide comment
@vsavkin

vsavkin Aug 23, 2016

Contributor

I don't think you need to use StringMapWrapper.

Contributor

vsavkin commented Aug 23, 2016

I don't think you need to use StringMapWrapper.

@vsavkin

This comment has been minimized.

Show comment
Hide comment
@vsavkin

vsavkin Aug 23, 2016

Contributor

Am I right to assume that because 'reset' does not change controls' statuses we won't have any issues regarding atomicity (e.g., the number of emitted events)?

Contributor

vsavkin commented Aug 23, 2016

Am I right to assume that because 'reset' does not change controls' statuses we won't have any issues regarding atomicity (e.g., the number of emitted events)?

@vsavkin

This comment has been minimized.

Show comment
Hide comment
@vsavkin

vsavkin Aug 23, 2016

Contributor

So controls can only be disabled by Angular. They cannot disable themselves. Is it correct?

Contributor

vsavkin commented Aug 23, 2016

So controls can only be disabled by Angular. They cannot disable themselves. Is it correct?

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Aug 23, 2016

Contributor
  1. Using that because it's faster than Object.keys, but can switch if you prefer
  2. Because emitEvent == false on reset, disable/enable methods don't emit an event in that case, only reset.
  3. Controls can only be disabled using our methods or by adding a disabled attribute (which we then handle as well). The property is a getter that cannot be directly modified by a user.
Contributor

kara commented Aug 23, 2016

  1. Using that because it's faster than Object.keys, but can switch if you prefer
  2. Because emitEvent == false on reset, disable/enable methods don't emit an event in that case, only reset.
  3. Controls can only be disabled using our methods or by adding a disabled attribute (which we then handle as well). The property is a getter that cannot be directly modified by a user.

@bradlygreen bradlygreen added this to the 2.0.0-rc.6 milestone Aug 24, 2016

@@ -175,6 +185,39 @@ export abstract class AbstractControl {
}
}
disable({onlySelf, emitEvent}: {onlySelf?: boolean, emitEvent?: boolean} = {}): void {
emitEvent = isPresent(emitEvent) ? emitEvent : true;

This comment has been minimized.

@vsavkin

vsavkin Aug 24, 2016

Contributor

since we do not compile to Dart anymore, you should be able to use the default value

@vsavkin

vsavkin Aug 24, 2016

Contributor

since we do not compile to Dart anymore, you should be able to use the default value

This comment has been minimized.

@kara

kara Aug 24, 2016

Contributor

Edit: we realized that it needs to be this way to ensure something like {onlySelf: true} doesn't set emitEvent to false.

@kara

kara Aug 24, 2016

Contributor

Edit: we realized that it needs to be this way to ensure something like {onlySelf: true} doesn't set emitEvent to false.

@@ -562,6 +626,14 @@ export class FormGroup extends AbstractControl {
this._updateTouched({onlySelf: onlySelf});
}
getRawValue(): Object {

This comment has been minimized.

@vsavkin

vsavkin Aug 24, 2016

Contributor

Maybe use this type for the return value {[k:name]:any}?

@vsavkin

vsavkin Aug 24, 2016

Contributor

Maybe use this type for the return value {[k:name]:any}?

This comment has been minimized.

@vsavkin

vsavkin Aug 24, 2016

Contributor

Also, didn't we talk about adding it later?

@vsavkin

vsavkin Aug 24, 2016

Contributor

Also, didn't we talk about adding it later?

This comment has been minimized.

@kara

kara Aug 24, 2016

Contributor

We did, but after some thought, changed my mind. Want to make sure the transition is smooth if anyone is using disabled for controls that can't be readonly, like checkboxes.

@kara

kara Aug 24, 2016

Contributor

We did, but after some thought, changed my mind. Want to make sure the transition is smooth if anyone is using disabled for controls that can't be readonly, like checkboxes.

describe('disabled controls', () => {
it('should not consider disabled controls in value or validation', fakeAsync(() => {
const fixture = TestBed.createComponent(NgModelGroupForm);
fixture.debugElement.componentInstance.isDisabled = false;

This comment has been minimized.

@vsavkin

vsavkin Aug 24, 2016

Contributor

Just curious if you considered:

Object.assign(fixture.debugElement.componentInstance, {isDisabled: false, first: '', last: 'Drew', email: 'some email'})
@vsavkin

vsavkin Aug 24, 2016

Contributor

Just curious if you considered:

Object.assign(fixture.debugElement.componentInstance, {isDisabled: false, first: '', last: 'Drew', email: 'some email'})

@vicb vicb merged commit 2b313e4 into angular:master Aug 24, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcalj

This comment has been minimized.

Show comment
Hide comment
@marcalj

marcalj Sep 1, 2016

How we should set a conditional disabled form control using Reactive Forms to simulate [disabled]="condition" ? Thanks!

marcalj commented Sep 1, 2016

How we should set a conditional disabled form control using Reactive Forms to simulate [disabled]="condition" ? Thanks!

@Krisa

This comment has been minimized.

Show comment
Hide comment
@Krisa

Krisa Sep 1, 2016

Wondering the same. My natural feeling was to do something like that:
new FormControl({disabled: this.Variable}) but it's not doing it

Krisa commented Sep 1, 2016

Wondering the same. My natural feeling was to do something like that:
new FormControl({disabled: this.Variable}) but it's not doing it

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Sep 1, 2016

Contributor

@marcalj @Krisa You need to pass in the full boxed value, with both value and disabled.

new FormControl({value: '', disabled: true})

Looking only for the disabled property is too generic and we could end up eating form control values that happen to have it. For that reason, we require setting exactly those two properties.

Contributor

kara commented Sep 1, 2016

@marcalj @Krisa You need to pass in the full boxed value, with both value and disabled.

new FormControl({value: '', disabled: true})

Looking only for the disabled property is too generic and we could end up eating form control values that happen to have it. For that reason, we require setting exactly those two properties.

@Krisa

This comment has been minimized.

Show comment
Hide comment
@Krisa

Krisa Sep 1, 2016

Thanks @kara for the super quick answer. My question was at least how to bind a dynamic disabled. In RC5, it was possible, like @marcalj shown before to do something like:
[disabled]="isDisabled && isAnotherVariable || isYetSomethingElse" and if that evaluates to true, then it's dynamically disabled. Follow-up issue created here: #11271

While this is eventually something else, your specific example seems to fail, I've reported just before this issue:

#11253

Krisa commented Sep 1, 2016

Thanks @kara for the super quick answer. My question was at least how to bind a dynamic disabled. In RC5, it was possible, like @marcalj shown before to do something like:
[disabled]="isDisabled && isAnotherVariable || isYetSomethingElse" and if that evaluates to true, then it's dynamically disabled. Follow-up issue created here: #11271

While this is eventually something else, your specific example seems to fail, I've reported just before this issue:

#11253

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