Skip to content

fix(cdk/listbox): remove incorrect usage of validator #25856

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 25, 2022
Merged

fix(cdk/listbox): remove incorrect usage of validator #25856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 10 additions & 26 deletions src/cdk/listbox/listbox.spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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