From 48d666b111563539cb827393f40f034362082d5f Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 25 Oct 2022 10:14:15 +0200 Subject: [PATCH] fix(cdk/listbox): remove incorrect usage of validator (#25856) 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 3d5ad3f917f23efd20687ece5795620b8ae969b1) --- src/cdk/listbox/listbox.spec.ts | 36 +++-------- src/cdk/listbox/listbox.ts | 88 +++++---------------------- tools/public_api_guard/cdk/listbox.md | 7 +-- 3 files changed, 26 insertions(+), 105 deletions(-) diff --git a/src/cdk/listbox/listbox.spec.ts b/src/cdk/listbox/listbox.spec.ts index 4aa473686eda..257d499e921b 100644 --- a/src/cdk/listbox/listbox.spec.ts +++ b/src/cdk/listbox/listbox.spec.ts @@ -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.'); }); }); }); diff --git a/src/cdk/listbox/listbox.ts b/src/cdk/listbox/listbox.ts index c6189270a1f9..bdaa61d904db 100644 --- a/src/cdk/listbox/listbox.ts +++ b/src/cdk/listbox/listbox.ts @@ -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. */ @@ -256,16 +247,9 @@ export class CdkOption implements ListKeyManagerOption, Highlightab useExisting: forwardRef(() => CdkListbox), multi: true, }, - { - provide: NG_VALIDATORS, - useExisting: forwardRef(() => CdkListbox), - multi: true, - }, ], }) -export class CdkListbox - implements AfterContentInit, OnDestroy, ControlValueAccessor, Validator -{ +export class CdkListbox implements AfterContentInit, OnDestroy, ControlValueAccessor { /** The id of the option's host element. */ @Input() get id() { @@ -416,9 +400,6 @@ export class CdkListbox /** 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[]>).pipe( @@ -438,40 +419,6 @@ export class CdkListbox /** 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(); @@ -614,6 +561,19 @@ export class CdkListbox */ 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.'); + } + } } /** @@ -625,23 +585,6 @@ export class CdkListbox this.disabled = isDisabled; } - /** - * Validate the given control - * @docs-private - */ - validate(control: AbstractControl): 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(); @@ -901,7 +844,6 @@ export class CdkListbox const selected = this.selectionModel.selected; this._invalid = (!this.multiple && selected.length > 1) || !!this._getInvalidOptionValues(selected).length; - this._onValidatorChange(); this.changeDetectorRef.markForCheck(); } diff --git a/tools/public_api_guard/cdk/listbox.md b/tools/public_api_guard/cdk/listbox.md index 13062fe0933d..b143c78cf4a0 100644 --- a/tools/public_api_guard/cdk/listbox.md +++ b/tools/public_api_guard/cdk/listbox.md @@ -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'; @@ -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 implements AfterContentInit, OnDestroy, ControlValueAccessor, Validator { +export class CdkListbox 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)); @@ -59,7 +56,6 @@ export class CdkListbox 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): void; protected selectionModel: ListboxSelectionModel; selectValue(value: T): void; @@ -72,7 +68,6 @@ export class CdkListbox implements AfterContentInit, OnDestroy, Con protected triggerRange(trigger: CdkOption | null, from: number, to: number, on: boolean): void; get useActiveDescendant(): boolean; set useActiveDescendant(shouldUseActiveDescendant: BooleanInput); - validate(control: AbstractControl): ValidationErrors | null; get value(): readonly T[]; set value(value: readonly T[]); readonly valueChange: Subject>;