fixup! fix(cdk-experimental/listbox): address comments

Author: mmalerba

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

@@ -147,17 +147,44 @@ describe('CdkOption and CdkListbox', () => {
       testComponent.isMultiselectable = true;
       fixture.detectChanges();
 
-      dispatchMouseEvent(optionEls[1], 'click', undefined, undefined, undefined, {shift: true});
+      dispatchMouseEvent(
+        optionEls[1],
+        'click',
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        {shift: true},
+      );
       fixture.detectChanges();
 
       expect(listbox.value).toEqual(['orange']);
 
-      dispatchMouseEvent(optionEls[3], 'click', undefined, undefined, undefined, {shift: true});
+      dispatchMouseEvent(
+        optionEls[3],
+        'click',
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        {shift: true},
+      );
       fixture.detectChanges();
 
       expect(listbox.value).toEqual(['orange', 'banana', 'peach']);
 
-      dispatchMouseEvent(optionEls[2], 'click', undefined, undefined, undefined, {shift: true});
+      dispatchMouseEvent(
+        optionEls[2],
+        'click',
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        undefined,
+        {shift: true},
+      );
       fixture.detectChanges();
 
       expect(listbox.value).toEqual(['orange']);
@@ -417,6 +444,36 @@ describe('CdkOption and CdkListbox', () => {
 
       expect(options[2].isActive()).toBeTrue();
     });
+
+    it('should not skip disabled options when navigating with arrow keys when skipping is turned off', async () => {
+      const {testComponent, fixture, listbox, listboxEl, options} = await setupComponent(
+        ListboxWithOptions,
+      );
+      testComponent.navigationSkipsDisabled = false;
+      testComponent.isOrangeDisabled = true;
+      listbox.focus();
+      fixture.detectChanges();
+
+      expect(options[0].isActive()).toBeTrue();
+
+      dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
+      fixture.detectChanges();
+
+      expect(options[1].isActive()).toBeTrue();
+    });
+
+    it('should not select disabled options with CONTROL + A', async () => {
+      const {testComponent, fixture, listbox, listboxEl} = await setupComponent(ListboxWithOptions);
+      testComponent.isMultiselectable = true;
+      testComponent.isOrangeDisabled = true;
+      fixture.detectChanges();
+
+      listbox.focus();
+      dispatchKeyboardEvent(listboxEl, 'keydown', A, undefined, {control: true});
+      fixture.detectChanges();
+
+      expect(listbox.value).toEqual(['apple', 'banana', 'peach']);
+    });
   });
 
   describe('compare with', () => {
@@ -611,6 +668,39 @@ describe('CdkOption and CdkListbox', () => {
 
       expect(listbox.value).toEqual([]);
     });
+
+    it('should wrap navigation when wrapping is enabled', async () => {
+      const {fixture, listbox, listboxEl, options} = await setupComponent(ListboxWithOptions);
+      listbox.focus();
+      dispatchKeyboardEvent(listboxEl, 'keydown', END);
+      fixture.detectChanges();
+
+      expect(options[options.length - 1].isActive()).toBeTrue();
+
+      dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
+      fixture.detectChanges();
+
+      expect(options[0].isActive()).toBeTrue();
+    });
+
+    it('should not wrap navigation when wrapping is not enabled', async () => {
+      const {testComponent, fixture, listbox, listboxEl, options} = await setupComponent(
+        ListboxWithOptions,
+      );
+      testComponent.navigationWraps = false;
+      fixture.detectChanges();
+
+      listbox.focus();
+      dispatchKeyboardEvent(listboxEl, 'keydown', END);
+      fixture.detectChanges();
+
+      expect(options[options.length - 1].isActive()).toBeTrue();
+
+      dispatchKeyboardEvent(listboxEl, 'keydown', DOWN_ARROW);
+      fixture.detectChanges();
+
+      expect(options[options.length - 1].isActive()).toBeTrue();
+    });
   });
 
   describe('with roving tabindex', () => {
@@ -818,14 +908,17 @@ describe('CdkOption and CdkListbox', () => {
          [cdkListboxDisabled]="isListboxDisabled"
          [cdkListboxUseActiveDescendant]="isActiveDescendant"
          [cdkListboxOrientation]="orientation"
+         [cdkListboxKeyboardNavigationWraps]="navigationWraps"
+         [cdkListboxKeyboardNavigationSkipsDisabled]="navigationSkipsDisabled"
          (cdkListboxValueChange)="onSelectionChange($event)">
       <div cdkOption="apple"
-          [cdkOptionDisabled]="isAppleDisabled"
-          [id]="appleId"
-          [tabindex]="appleTabindex">
+           [cdkOptionDisabled]="isAppleDisabled"
+           [id]="appleId"
+           [tabindex]="appleTabindex">
         Apple
       </div>
-      <div cdkOption="orange" [cdkOptionDisabled]="isOrangeDisabled">Orange</div>
+      <div cdkOption="orange" [cdkOptionDisabled]="isOrangeDisabled">Orange
+      </div>
       <div cdkOption="banana">Banana</div>
       <div cdkOption="peach">Peach</div>
     </div>
@@ -838,6 +931,8 @@ class ListboxWithOptions {
   isOrangeDisabled = false;
   isMultiselectable = false;
   isActiveDescendant = false;
+  navigationWraps = true;
+  navigationSkipsDisabled = true;
   listboxId: string;
   listboxTabindex: number;
   appleId: string;

src/cdk-experimental/listbox/listbox.ts

@@ -52,10 +52,6 @@ import {Directionality} from '@angular/cdk/bidi';
 /** The next id to use for creating unique DOM IDs. */
 let nextId = 0;
 
-// TODO(mmalerba):
-//   - should listbox wrap be configurable?
-//   - should skipping disabled options be configurable?
-
 /**
  * An implementation of SelectionModel that internally always represents the selection as a
  * 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>
     this.selectionModel.compareWith = fn;
   }
 
+  /**
+   * Whether the keyboard navigation should wrap when the user presses arrow down on the last item
+   * or arrow up on the first item.
+   */
+  @Input('cdkListboxKeyboardNavigationWraps')
+  get keyboardNavigationWraps() {
+    return this._keyboardNavigationWraps;
+  }
+  set keyboardNavigationWraps(wrap: BooleanInput) {
+    this._keyboardNavigationWraps = coerceBooleanProperty(wrap);
+    this.listKeyManager?.withWrap(this._keyboardNavigationWraps);
+  }
+  private _keyboardNavigationWraps = true;
+
+  /** Whether keyboard navigation should skip over disabled items. */
+  @Input('cdkListboxKeyboardNavigationSkipsDisabled')
+  get keyboardNavigationSkipsDisabled() {
+    return this._keyboardNavigationSkipsDisabled;
+  }
+  set keyboardNavigationSkipsDisabled(skip: BooleanInput) {
+    this._keyboardNavigationSkipsDisabled = coerceBooleanProperty(skip);
+    this.listKeyManager?.skipPredicate(
+      this._keyboardNavigationSkipsDisabled ? this._skipDisabledPredicate : this._skipNonePredicate,
+    );
+  }
+  private _keyboardNavigationSkipsDisabled = true;
+
   /** Emits when the selected value(s) in the listbox change. */
   @Output('cdkListboxValueChange') readonly valueChange = new Subject<ListboxValueChangeEvent<T>>();
 
@@ -409,6 +432,12 @@ export class CdkListbox<T = unknown>
   /** The directionality of the page. */
   private readonly _dir = inject(Directionality, InjectFlags.Optional);
 
+  /** A predicate that skips disabled options. */
+  private readonly _skipDisabledPredicate = (option: CdkOption<T>) => option.disabled;
+
+  /** 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.
@@ -663,12 +692,10 @@ export class CdkListbox<T = unknown>
     }
     this._lastTriggered = trigger;
     const isEqual = this.compareWith ?? Object.is;
-    const updateValues = this.options
-      .map(option => option.value)
-      .slice(
-        Math.max(0, Math.min(from, to)),
-        Math.min(this.options.length, Math.max(from, to) + 1),
-      );
+    const updateValues = [...this.options]
+      .slice(Math.max(0, Math.min(from, to)), Math.min(this.options.length, Math.max(from, to) + 1))
+      .filter(option => !option.disabled)
+      .map(option => option.value);
     const selected = [...this.value];
     for (const updateValue of updateValues) {
       const selectedIndex = selected.findIndex(selectedValue =>
@@ -835,10 +862,15 @@ export class CdkListbox<T = unknown>
   /** Initialize the key manager. */
   private _initKeyManager() {
     this.listKeyManager = new ActiveDescendantKeyManager(this.options)
-      .withWrap()
+      .withWrap(this._keyboardNavigationWraps)
       .withTypeAhead()
       .withHomeAndEnd()
-      .withAllowedModifierKeys(['shiftKey']);
+      .withAllowedModifierKeys(['shiftKey'])
+      .skipPredicate(
+        this._keyboardNavigationSkipsDisabled
+          ? this._skipDisabledPredicate
+          : this._skipNonePredicate,
+      );
 
     if (this.orientation === 'vertical') {
       this.listKeyManager.withVerticalOrientation();

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

@@ -5,10 +5,11 @@ <h2>formControl</h2>
     [cdkListboxMultiple]="multiSelectable"
     [cdkListboxUseActiveDescendant]="activeDescendant"
     [cdkListboxCompareWith]="compare"
+    [cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
     [formControl]="fruitControl">
   <li cdkOption="apple">Apple</li>
   <li cdkOption="orange">Orange</li>
-  <li cdkOption="grapefruit">Grapefruit</li>
+  <li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
   <li cdkOption="peach">Peach</li>
   <li cdkOption="kiwi">Kiwi</li>
 </ul>
@@ -30,10 +31,11 @@ <h2>ngModel</h2>
     [cdkListboxUseActiveDescendant]="activeDescendant"
     [cdkListboxCompareWith]="compare"
     [cdkListboxDisabled]="fruitControl.disabled"
+    [cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
     [(ngModel)]="fruit">
   <li cdkOption="apple">Apple</li>
   <li cdkOption="orange">Orange</li>
-  <li cdkOption="grapefruit">Grapefruit</li>
+  <li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
   <li cdkOption="peach">Peach</li>
 </ul>
 <select [multiple]="multiSelectable" [(ngModel)]="nativeFruit">
@@ -53,11 +55,12 @@ <h2>value binding</h2>
     [cdkListboxUseActiveDescendant]="activeDescendant"
     [cdkListboxCompareWith]="compare"
     [cdkListboxDisabled]="fruitControl.disabled"
+    [cdkListboxKeyboardNavigationSkipsDisabled]="skipDisabled"
     [cdkListboxValue]="fruit"
     (cdkListboxValueChange)="fruit = $event.value">
   <li cdkOption="apple">Apple</li>
   <li cdkOption="orange">Orange</li>
-  <li cdkOption="grapefruit">Grapefruit</li>
+  <li cdkOption="grapefruit" cdkOptionDisabled>Grapefruit</li>
   <li cdkOption="peach">Peach</li>
 </ul>
 <select [multiple]="multiSelectable" (change)="onNativeFruitChange($event)">
@@ -83,3 +86,4 @@ <h2>value binding</h2>
 }}</button>
 <br>
 <button (click)="toggleDumbCompare()">Compare With: {{this.compare ? 'Apple === Orange' : 'Standard'}}</button>
+<button (click)="toggleSkipDisabled()">Skip Disabled: {{this.skipDisabled ? 'Yes' : 'No'}}</button>

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

@@ -27,6 +27,7 @@ function dumbCompare(o1: string, o2: string) {
 export class CdkListboxDemo {
   multiSelectable = false;
   activeDescendant = true;
+  skipDisabled = true;
   compare?: (o1: string, o2: string) => boolean;
   fruitControl = new FormControl();
   nativeFruitControl = new FormControl();
@@ -67,6 +68,10 @@ export class CdkListboxDemo {
     this.compare = this.compare ? undefined : dumbCompare;
   }
 
+  toggleSkipDisabled() {
+    this.skipDisabled = !this.skipDisabled;
+  }
+
   onNativeFruitChange(event: Event) {
     this.nativeFruit = Array.from(
       (event.target as HTMLSelectElement).selectedOptions,