Skip to content

Commit 6c26bf4

Browse files
mmalerbammalerba
committed
fixup! fix(cdk-experimental/listbox): address comments
1 parent c64fe16 commit 6c26bf4

File tree

5 files changed

+166
-29
lines changed

5 files changed

+166
-29
lines changed

‎src/cdk-experimental/listbox/listbox.spec.ts

Lines changed: 102 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -147,17 +147,44 @@ describe('CdkOption and CdkListbox', () => {
147147
testComponent.isMultiselectable = true;
148148
fixture.detectChanges();
149149

150-
dispatchMouseEvent(optionEls[1], 'click', undefined, undefined, undefined, {shift: true});
150+
dispatchMouseEvent(
151+
optionEls[1],
152+
'click',
153+
undefined,
154+
undefined,
155+
undefined,
156+
undefined,
157+
undefined,
158+
{shift: true},
159+
);
151160
fixture.detectChanges();
152161

153162
expect(listbox.value).toEqual(['orange']);
154163

155-
dispatchMouseEvent(optionEls[3], 'click', undefined, undefined, undefined, {shift: true});
164+
dispatchMouseEvent(
165+
optionEls[3],
166+
'click',
167+
undefined,
168+
undefined,
169+
undefined,
170+
undefined,
171+
undefined,
172+
{shift: true},
173+
);
156174
fixture.detectChanges();
157175

158176
expect(listbox.value).toEqual(['orange', 'banana', 'peach']);
159177

160-
dispatchMouseEvent(optionEls[2], 'click', undefined, undefined, undefined, {shift: true});
178+
dispatchMouseEvent(
179+
optionEls[2],
180+
'click',
181+
undefined,
182+
undefined,
183+
undefined,
184+
undefined,
185+
undefined,
186+
{shift: true},
187+
);
161188
fixture.detectChanges();
162189

163190
expect(listbox.value).toEqual(['orange']);
@@ -417,6 +444,36 @@ describe('CdkOption and CdkListbox', () => {
417444

418445
expect(options[2].isActive()).toBeTrue();
419446
});
447+
448+
it('should not skip disabled options when navigating with arrow keys when skipping is turned off', async () => {
449+
const {testComponent, fixture, listbox, listboxEl, options} = await setupComponent(
450+
ListboxWithOptions,
451+
);
452+
testComponent.navigationSkipsDisabled = false;
453+
testComponent.isOrangeDisabled = true;
454+
listbox.focus();
455+
fixture.detectChanges();
456+
457+
expect(options[0].isActive()).toBeTrue();
458+
459+
dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
460+
fixture.detectChanges();
461+
462+
expect(options[1].isActive()).toBeTrue();
463+
});
464+
465+
it('should not select disabled options with CONTROL + A', async () => {
466+
const {testComponent, fixture, listbox, listboxEl} = await setupComponent(ListboxWithOptions);
467+
testComponent.isMultiselectable = true;
468+
testComponent.isOrangeDisabled = true;
469+
fixture.detectChanges();
470+
471+
listbox.focus();
472+
dispatchKeyboardEvent(listboxEl, 'keydown', A, undefined, {control: true});
473+
fixture.detectChanges();
474+
475+
expect(listbox.value).toEqual(['apple', 'banana', 'peach']);
476+
});
420477
});
421478

422479
describe('compare with', () => {
@@ -611,6 +668,39 @@ describe('CdkOption and CdkListbox', () => {
611668

612669
expect(listbox.value).toEqual([]);
613670
});
671+
672+
it('should wrap navigation when wrapping is enabled', async () => {
673+
const {fixture, listbox, listboxEl, options} = await setupComponent(ListboxWithOptions);
674+
listbox.focus();
675+
dispatchKeyboardEvent(listboxEl, 'keydown', END);
676+
fixture.detectChanges();
677+
678+
expect(options[options.length - 1].isActive()).toBeTrue();
679+
680+
dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
681+
fixture.detectChanges();
682+
683+
expect(options[0].isActive()).toBeTrue();
684+
});
685+
686+
it('should not wrap navigation when wrapping is not enabled', async () => {
687+
const {testComponent, fixture, listbox, listboxEl, options} = await setupComponent(
688+
ListboxWithOptions,
689+
);
690+
testComponent.navigationWraps = false;
691+
fixture.detectChanges();
692+
693+
listbox.focus();
694+
dispatchKeyboardEvent(listboxEl, 'keydown', END);
695+
fixture.detectChanges();
696+
697+
expect(options[options.length - 1].isActive()).toBeTrue();
698+
699+
dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
700+
fixture.detectChanges();
701+
702+
expect(options[options.length - 1].isActive()).toBeTrue();
703+
});
614704
});
615705

616706
describe('with roving tabindex', () => {
@@ -818,14 +908,17 @@ describe('CdkOption and CdkListbox', () => {
818908
[cdkListboxDisabled]="isListboxDisabled"
819909
[cdkListboxUseActiveDescendant]="isActiveDescendant"
820910
[cdkListboxOrientation]="orientation"
911+
[cdkListboxKeyboardNavigationWraps]="navigationWraps"
912+
[cdkListboxKeyboardNavigationSkipsDisabled]="navigationSkipsDisabled"
821913
(cdkListboxValueChange)="onSelectionChange($event)">
822914
<div cdkOption="apple"
823-
[cdkOptionDisabled]="isAppleDisabled"
824-
[id]="appleId"
825-
[tabindex]="appleTabindex">
915+
[cdkOptionDisabled]="isAppleDisabled"
916+
[id]="appleId"
917+
[tabindex]="appleTabindex">
826918
Apple
827919
</div>
828-
<div cdkOption="orange" [cdkOptionDisabled]="isOrangeDisabled">Orange</div>
920+
<div cdkOption="orange" [cdkOptionDisabled]="isOrangeDisabled">Orange
921+
</div>
829922
<div cdkOption="banana">Banana</div>
830923
<div cdkOption="peach">Peach</div>
831924
</div>
@@ -838,6 +931,8 @@ class ListboxWithOptions {
838931
isOrangeDisabled = false;
839932
isMultiselectable = false;
840933
isActiveDescendant = false;
934+
navigationWraps = true;
935+
navigationSkipsDisabled = true;
841936
listboxId: string;
842937
listboxTabindex: number;
843938
appleId: string;

‎src/cdk-experimental/listbox/listbox.ts

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,6 @@ import {Directionality} from '@angular/cdk/bidi';
5252
/** The next id to use for creating unique DOM IDs. */
5353
let nextId = 0;
5454

55-
// TODO(mmalerba):
56-
// - should listbox wrap be configurable?
57-
// - should skipping disabled options be configurable?
58-
5955
/**
6056
* An implementation of SelectionModel that internally always represents the selection as a
6157
* multi-selection. This is necessary so that we can recover the full selection if the user
@@ -360,6 +356,33 @@ export class CdkListbox<T = unknown>
360356
this.selectionModel.compareWith = fn;
361357
}
362358

359+
/**
360+
* Whether the keyboard navigation should wrap when the user presses arrow down on the last item
361+
* or arrow up on the first item.
362+
*/
363+
@Input('cdkListboxKeyboardNavigationWraps')
364+
get keyboardNavigationWraps() {
365+
return this._keyboardNavigationWraps;
366+
}
367+
set keyboardNavigationWraps(wrap: BooleanInput) {
368+
this._keyboardNavigationWraps = coerceBooleanProperty(wrap);
369+
this.listKeyManager?.withWrap(this._keyboardNavigationWraps);
370+
}
371+
private _keyboardNavigationWraps = true;
372+
373+
/** Whether keyboard navigation should skip over disabled items. */
374+
@Input('cdkListboxKeyboardNavigationSkipsDisabled')
375+
get keyboardNavigationSkipsDisabled() {
376+
return this._keyboardNavigationSkipsDisabled;
377+
}
378+
set keyboardNavigationSkipsDisabled(skip: BooleanInput) {
379+
this._keyboardNavigationSkipsDisabled = coerceBooleanProperty(skip);
380+
this.listKeyManager?.skipPredicate(
381+
this._keyboardNavigationSkipsDisabled ? this._skipDisabledPredicate : this._skipNonePredicate,
382+
);
383+
}
384+
private _keyboardNavigationSkipsDisabled = true;
385+
363386
/** Emits when the selected value(s) in the listbox change. */
364387
@Output('cdkListboxValueChange') readonly valueChange = new Subject<ListboxValueChangeEvent<T>>();
365388

@@ -409,6 +432,12 @@ export class CdkListbox<T = unknown>
409432
/** The directionality of the page. */
410433
private readonly _dir = inject(Directionality, InjectFlags.Optional);
411434

435+
/** A predicate that skips disabled options. */
436+
private readonly _skipDisabledPredicate = (option: CdkOption<T>) => option.disabled;
437+
438+
/** A predicate that does not skip any options. */
439+
private readonly _skipNonePredicate = () => false;
440+
412441
/**
413442
* Validator that produces an error if multiple values are selected in a single selection
414443
* listbox.
@@ -663,12 +692,10 @@ export class CdkListbox<T = unknown>
663692
}
664693
this._lastTriggered = trigger;
665694
const isEqual = this.compareWith ?? Object.is;
666-
const updateValues = this.options
667-
.map(option => option.value)
668-
.slice(
669-
Math.max(0, Math.min(from, to)),
670-
Math.min(this.options.length, Math.max(from, to) + 1),
671-
);
695+
const updateValues = [...this.options]
696+
.slice(Math.max(0, Math.min(from, to)), Math.min(this.options.length, Math.max(from, to) + 1))
697+
.filter(option => !option.disabled)
698+
.map(option => option.value);
672699
const selected = [...this.value];
673700
for (const updateValue of updateValues) {
674701
const selectedIndex = selected.findIndex(selectedValue =>
@@ -835,10 +862,15 @@ export class CdkListbox<T = unknown>
835862
/** Initialize the key manager. */
836863
private _initKeyManager() {
837864
this.listKeyManager = new ActiveDescendantKeyManager(this.options)
838-
.withWrap()
865+
.withWrap(this._keyboardNavigationWraps)
839866
.withTypeAhead()
840867
.withHomeAndEnd()
841-
.withAllowedModifierKeys(['shiftKey']);
868+
.withAllowedModifierKeys(['shiftKey'])
869+
.skipPredicate(
870+
this._keyboardNavigationSkipsDisabled
871+
? this._skipDisabledPredicate
872+
: this._skipNonePredicate,
873+
);
842874

843875
if (this.orientation === 'vertical') {
844876
this.listKeyManager.withVerticalOrientation();

‎src/dev-app/cdk-experimental-listbox/cdk-listbox-demo.html

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,11 @@ <h2>formControl</h2>
55
[cdkListboxMultiple]="multiSelectable"
66
[cdkListboxUseActiveDescendant]="activeDescendant"
77
[cdkListboxCompareWith]="compare"
8+
[cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
89
[formControl]="fruitControl">
910
<li cdkOption="apple">Apple</li>
1011
<li cdkOption="orange">Orange</li>
11-
<li cdkOption="grapefruit">Grapefruit</li>
12+
<li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
1213
<li cdkOption="peach">Peach</li>
1314
<li cdkOption="kiwi">Kiwi</li>
1415
</ul>
@@ -30,10 +31,11 @@ <h2>ngModel</h2>
3031
[cdkListboxUseActiveDescendant]="activeDescendant"
3132
[cdkListboxCompareWith]="compare"
3233
[cdkListboxDisabled]="fruitControl.disabled"
34+
[cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
3335
[(ngModel)]="fruit">
3436
<li cdkOption="apple">Apple</li>
3537
<li cdkOption="orange">Orange</li>
36-
<li cdkOption="grapefruit">Grapefruit</li>
38+
<li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
3739
<li cdkOption="peach">Peach</li>
3840
</ul>
3941
<select [multiple]="multiSelectable" [(ngModel)]="nativeFruit">
@@ -53,11 +55,12 @@ <h2>value binding</h2>
5355
[cdkListboxUseActiveDescendant]="activeDescendant"
5456
[cdkListboxCompareWith]="compare"
5557
[cdkListboxDisabled]="fruitControl.disabled"
58+
[cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
5659
[cdkListboxValue]="fruit"
5760
(cdkListboxValueChange)="fruit = $event.value">
5861
<li cdkOption="apple">Apple</li>
5962
<li cdkOption="orange">Orange</li>
60-
<li cdkOption="grapefruit">Grapefruit</li>
63+
<li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
6164
<li cdkOption="peach">Peach</li>
6265
</ul>
6366
<select [multiple]="multiSelectable" (change)="onNativeFruitChange($event)">
@@ -83,3 +86,4 @@ <h2>value binding</h2>
8386
}}</button>
8487
<br>
8588
<button (click)="toggleDumbCompare()">Compare With: {{this.compare ? 'Apple === Orange' : 'Standard'}}</button>
89+
<button (click)="toggleSkipDisabled()">Skip Disabled: {{this.skipDisabled ? 'Yes' : 'No'}}</button>

‎src/dev-app/cdk-experimental-listbox/cdk-listbox-demo.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ function dumbCompare(o1: string, o2: string) {
2727
export class CdkListboxDemo {
2828
multiSelectable = false;
2929
activeDescendant = true;
30+
skipDisabled = true;
3031
compare?: (o1: string, o2: string) => boolean;
3132
fruitControl = new FormControl();
3233
nativeFruitControl = new FormControl();
@@ -67,6 +68,10 @@ export class CdkListboxDemo {
6768
this.compare = this.compare ? undefined : dumbCompare;
6869
}
6970

71+
toggleSkipDisabled() {
72+
this.skipDisabled = !this.skipDisabled;
73+
}
74+
7075
onNativeFruitChange(event: Event) {
7176
this.nativeFruit = Array.from(
7277
(event.target as HTMLSelectElement).selectedOptions,

‎tools/public_api_guard/cdk/collections.md

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,20 +71,21 @@ export interface SelectionChange<T> {
7171

7272
// @public
7373
export class SelectionModel<T> {
74-
constructor(_multiple?: boolean, initiallySelectedValues?: T[], _emitChanges?: boolean, _compareWith?: ((o1: T, o2: T) => boolean) | undefined);
74+
constructor(_multiple?: boolean, initiallySelectedValues?: T[], _emitChanges?: boolean, compareWith?: ((o1: T, o2: T) => boolean) | undefined);
7575
readonly changed: Subject<SelectionChange<T>>;
76-
clear(): void;
77-
deselect(...values: T[]): void;
76+
clear(flushEvent?: boolean): boolean;
77+
// (undocumented)
78+
compareWith?: ((o1: T, o2: T) => boolean) | undefined;
79+
deselect(...values: T[]): boolean;
7880
hasValue(): boolean;
7981
isEmpty(): boolean;
8082
isMultipleSelection(): boolean;
8183
isSelected(value: T): boolean;
82-
select(...values: T[]): void;
84+
select(...values: T[]): boolean;
8385
get selected(): T[];
84-
// (undocumented)
85-
setSelection(...values: T[]): void;
86+
setSelection(...values: T[]): boolean;
8687
sort(predicate?: (a: T, b: T) => number): void;
87-
toggle(value: T): void;
88+
toggle(value: T): boolean;
8889
}
8990

9091
// @public

0 commit comments

Comments
 (0)