Skip to content

Commit

Permalink
fix(cdk/listbox): remove incorrect usage of validator (#25856)
Browse files Browse the repository at this point in the history 
Currently the CDK listbox is providing itself as a validator so that it can validate that programmatically-assigned values are valid. This is incorrect, because validators are meant to check user-assigned values and the checks that are currently being performed can't happen purely through the UI.

These changes remove the `Validator` implementation and replace the checks with runtime errors.

(cherry picked from commit 3d5ad3f)
  • Loading branch information
@crisbeto
crisbeto committed Oct 25, 2022
1 parent 1655a69 commit 48d666b
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 105 deletions.
36 changes: 10 additions & 26 deletions src/cdk/listbox/listbox.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -852,44 +852,28 @@ describe('CdkOption and CdkListbox', () => {
subscription.unsubscribe();
});

it('should have FormControl error when multiple values selected in single-select listbox', () => {
it('should throw when multiple values selected in single-select listbox', () => {
const {testComponent, fixture} = setupComponent(ListboxWithFormControl, [
ReactiveFormsModule,
]);
testComponent.formControl.setValue(['orange', 'banana']);
fixture.detectChanges();

expect(testComponent.formControl.hasError('cdkListboxUnexpectedMultipleValues')).toBeTrue();
expect(testComponent.formControl.hasError('cdkListboxUnexpectedOptionValues')).toBeFalse();
expect(() => {
testComponent.formControl.setValue(['orange', 'banana']);
fixture.detectChanges();
}).toThrowError('Listbox cannot have more than one selected value in multi-selection mode.');
});

it('should have FormControl error when non-option value selected', () => {
it('should throw when an invalid value is selected', () => {
const {testComponent, fixture} = setupComponent(ListboxWithFormControl, [
ReactiveFormsModule,
]);
testComponent.isMultiselectable = true;
testComponent.formControl.setValue(['orange', 'dragonfruit', 'mango']);
fixture.detectChanges();

expect(testComponent.formControl.hasError('cdkListboxUnexpectedOptionValues')).toBeTrue();
expect(testComponent.formControl.hasError('cdkListboxUnexpectedMultipleValues')).toBeFalse();
expect(testComponent.formControl.errors?.['cdkListboxUnexpectedOptionValues']).toEqual({
'values': ['dragonfruit', 'mango'],
});
});

it('should have multiple FormControl errors when multiple non-option values selected in single-select listbox', () => {
const {testComponent, fixture} = setupComponent(ListboxWithFormControl, [
ReactiveFormsModule,
]);
testComponent.formControl.setValue(['dragonfruit', 'mango']);
fixture.detectChanges();

expect(testComponent.formControl.hasError('cdkListboxUnexpectedOptionValues')).toBeTrue();
expect(testComponent.formControl.hasError('cdkListboxUnexpectedMultipleValues')).toBeTrue();
expect(testComponent.formControl.errors?.['cdkListboxUnexpectedOptionValues']).toEqual({
'values': ['dragonfruit', 'mango'],
});
expect(() => {
testComponent.formControl.setValue(['orange', 'dragonfruit', 'mango']);
fixture.detectChanges();
}).toThrowError('Listbox has selected values that do not match any of its options.');
});
});
});
Expand Down
88 changes: 15 additions & 73 deletions src/cdk/listbox/listbox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,7 @@ import {BooleanInput, coerceArray, coerceBooleanProperty} from '@angular/cdk/coe
import {SelectionModel} from '@angular/cdk/collections';
import {defer, merge, Observable, Subject} from 'rxjs';
import {filter, map, startWith, switchMap, takeUntil} from 'rxjs/operators';
import {
AbstractControl,
ControlValueAccessor,
NG_VALIDATORS,
NG_VALUE_ACCESSOR,
ValidationErrors,
Validator,
ValidatorFn,
Validators,
} from '@angular/forms';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
import {Directionality} from '@angular/cdk/bidi';

/** The next id to use for creating unique DOM IDs. */
Expand Down Expand Up @@ -256,16 +247,9 @@ export class CdkOption<T = unknown> implements ListKeyManagerOption, Highlightab
useExisting: forwardRef(() => CdkListbox),
multi: true,
},
{
provide: NG_VALIDATORS,
useExisting: forwardRef(() => CdkListbox),
multi: true,
},
],
})
export class CdkListbox<T = unknown>
implements AfterContentInit, OnDestroy, ControlValueAccessor, Validator
{
export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, ControlValueAccessor {
/** The id of the option's host element. */
@Input()
get id() {
Expand Down Expand Up @@ -416,9 +400,6 @@ export class CdkListbox<T = unknown>
/** Callback called when the listbox value changes */
private _onChange: (value: readonly T[]) => void = () => {};

/** Callback called when the form validator changes. */
private _onValidatorChange = () => {};

/** Emits when an option has been clicked. */
private _optionClicked = defer(() =>
(this.options.changes as Observable<CdkOption<T>[]>).pipe(
Expand All @@ -438,40 +419,6 @@ export class CdkListbox<T = unknown>
/** A predicate that does not skip any options. */
private readonly _skipNonePredicate = () => false;

/**
* Validator that produces an error if multiple values are selected in a single selection
* listbox.
* @param control The control to validate
* @return A validation error or null
*/
private _validateUnexpectedMultipleValues: ValidatorFn = (control: AbstractControl) => {
const controlValue = this._coerceValue(control.value);
if (!this.multiple && controlValue.length > 1) {
return {'cdkListboxUnexpectedMultipleValues': true};
}
return null;
};

/**
* Validator that produces an error if any selected values are not valid options for this listbox.
* @param control The control to validate
* @return A validation error or null
*/
private _validateUnexpectedOptionValues: ValidatorFn = (control: AbstractControl) => {
const controlValue = this._coerceValue(control.value);
const invalidValues = this._getInvalidOptionValues(controlValue);
if (invalidValues.length) {
return {'cdkListboxUnexpectedOptionValues': {'values': invalidValues}};
}
return null;
};

/** The combined set of validators for this listbox. */
private _validators = Validators.compose([
this._validateUnexpectedMultipleValues,
this._validateUnexpectedOptionValues,
])!;

ngAfterContentInit() {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
this._verifyNoOptionValueCollisions();
Expand Down Expand Up @@ -614,6 +561,19 @@ export class CdkListbox<T = unknown>
*/
writeValue(value: readonly T[]): void {
this._setSelection(value);

if (typeof ngDevMode === 'undefined' || ngDevMode) {
const selected = this.selectionModel.selected;
const invalidValues = this._getInvalidOptionValues(selected);

if (!this.multiple && selected.length > 1) {
throw Error('Listbox cannot have more than one selected value in multi-selection mode.');
}

if (invalidValues.length) {
throw Error('Listbox has selected values that do not match any of its options.');
}
}
}

/**
Expand All @@ -625,23 +585,6 @@ export class CdkListbox<T = unknown>
this.disabled = isDisabled;
}

/**
* Validate the given control
* @docs-private
*/
validate(control: AbstractControl<any, any>): ValidationErrors | null {
return this._validators(control);
}

/**
* Registers a callback to be called when the form validator changes.
* @param fn The callback to call
* @docs-private
*/
registerOnValidatorChange(fn: () => void) {
this._onValidatorChange = fn;
}

/** Focus the listbox's host element. */
focus() {
this.element.focus();
Expand Down Expand Up @@ -901,7 +844,6 @@ export class CdkListbox<T = unknown>
const selected = this.selectionModel.selected;
this._invalid =
(!this.multiple && selected.length > 1) || !!this._getInvalidOptionValues(selected).length;
this._onValidatorChange();
this.changeDetectorRef.markForCheck();
}

Expand Down
7 changes: 1 addition & 6 deletions tools/public_api_guard/cdk/listbox.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
```ts

import { AbstractControl } from '@angular/forms';
import { ActiveDescendantKeyManager } from '@angular/cdk/a11y';
import { AfterContentInit } from '@angular/core';
import { BooleanInput } from '@angular/cdk/coercion';
Expand All @@ -17,11 +16,9 @@ import { OnDestroy } from '@angular/core';
import { QueryList } from '@angular/core';
import { SelectionModel } from '@angular/cdk/collections';
import { Subject } from 'rxjs';
import { ValidationErrors } from '@angular/forms';
import { Validator } from '@angular/forms';

// @public (undocumented)
export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, ControlValueAccessor, Validator {
export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, ControlValueAccessor {
protected readonly changeDetectorRef: ChangeDetectorRef;
get compareWith(): undefined | ((o1: T, o2: T) => boolean);
set compareWith(fn: undefined | ((o1: T, o2: T) => boolean));
Expand Down Expand Up @@ -59,7 +56,6 @@ export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, Con
set orientation(value: 'horizontal' | 'vertical');
registerOnChange(fn: (value: readonly T[]) => void): void;
registerOnTouched(fn: () => {}): void;
registerOnValidatorChange(fn: () => void): void;
select(option: CdkOption<T>): void;
protected selectionModel: ListboxSelectionModel<T>;
selectValue(value: T): void;
Expand All @@ -72,7 +68,6 @@ export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, Con
protected triggerRange(trigger: CdkOption<T> | null, from: number, to: number, on: boolean): void;
get useActiveDescendant(): boolean;
set useActiveDescendant(shouldUseActiveDescendant: BooleanInput);
validate(control: AbstractControl<any, any>): ValidationErrors | null;
get value(): readonly T[];
set value(value: readonly T[]);
readonly valueChange: Subject<ListboxValueChangeEvent<T>>;
Expand Down

0 comments on commit 48d666b

Please sign in to comment.