Skip to content

fix(cdk-experimental/listbox): initial listbox focus state #30764

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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 32 additions & 4 deletions src/cdk-experimental/listbox/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@
*/

import {
AfterViewInit,
booleanAttribute,
computed,
contentChildren,
Directive,
effect,
ElementRef,
inject,
input,
model,
signal,
} from '@angular/core';
import {ListboxPattern, OptionPattern} from '../ui-patterns';
import {Directionality} from '@angular/cdk/bidi';
Expand All @@ -29,9 +32,9 @@ import {_IdGenerator} from '@angular/cdk/a11y';
*
* ```html
* <ul cdkListbox>
* <li cdkOption>Item 1</li>
* <li cdkOption>Item 2</li>
* <li cdkOption>Item 3</li>
* <li [value]="1" cdkOption>Item 1</li>
* <li [value]="2" cdkOption>Item 2</li>
* <li [value]="3" cdkOption>Item 3</li>
* </ul>
* ```
*/
Expand All @@ -49,9 +52,10 @@ import {_IdGenerator} from '@angular/cdk/a11y';
'[attr.aria-activedescendant]': 'pattern.activedescendant()',
'(keydown)': 'pattern.onKeydown($event)',
'(pointerdown)': 'pattern.onPointerdown($event)',
'(focusin)': 'onFocus()',
},
})
export class CdkListbox<V> {
export class CdkListbox<V> implements AfterViewInit {
/** The directionality (LTR / RTL) context for the application (or a subtree of it). */
private readonly _directionality = inject(Directionality);

Expand Down Expand Up @@ -105,6 +109,29 @@ export class CdkListbox<V> {
items: this.items,
textDirection: this.textDirection,
});

/** Whether the listbox has received focus yet. */
private _touched = signal(false);

/** Whether the options in the listbox have been initialized. */
private _isViewInitialized = signal(false);

constructor() {
effect(() => {
if (this._isViewInitialized() && !this._touched()) {
const index = this.items().findIndex(i => this.value().includes(i.value()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird to have these signal-reads knowing that it causes this effect to be run a lot. I guess it doesn't matter because of the initial conditional check, so its a fast function. But I wish there was a way for Angular to know this should really only be run based on the two conditional signals

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree. Having to have a signal for isViewInitialized feels very janky. This was the only way I could get around Angular throwing an error because a required signal hasn't been set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the this.items(), this.value(), i.value() - these will all trigger the effect right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes but those are needed to trigger the effect in order to figure out the correct start index. The effect should rerun any time those signals change until the listbox is touched (focused for the first time)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I wish there was a way for Angular to know this should really only be run based on the two conditional signals

I think you can wrap the rest of the reads in untracked(() => ...) to indicate that (though it sounds like maybe you do need it to react to all of them here, just wanted to give an fyi that the feature exists)

this.activeIndex.set(Math.max(index, 0));
}
});
}

ngAfterViewInit() {
this._isViewInitialized.set(true);
}

onFocus() {
this._touched.set(true);
}
}

/** A selectable option in a CdkListbox. */
Expand Down Expand Up @@ -133,6 +160,7 @@ export class CdkOption<V> {
/** A unique identifier for the option. */
protected id = computed(() => this._generatedId);

/** The value of the option. */
protected value = input.required<V>();

// TODO(wagnermaciel): See if we want to change how we handle this since textContent is not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@
<mat-checkbox [formControl]="readonly">Readonly</mat-checkbox>
<mat-checkbox [formControl]="skipDisabled">Skip Disabled</mat-checkbox>

<mat-form-field subscriptSizing="dynamic" appearance="outline">
<mat-label>Selection</mat-label>
<mat-select [(value)]="selection" multiple>
@for (fruit of fruits; track fruit) {
<mat-option [value]="fruit">{{fruit}}</mat-option>
}
</mat-select>
</mat-form-field>

<mat-form-field subscriptSizing="dynamic" appearance="outline">
<mat-label>Orientation</mat-label>
<mat-select [(value)]="orientation">
Expand Down Expand Up @@ -34,6 +43,7 @@
<ul
cdkListbox
[wrap]="wrap.value"
[value]="selection"
[multi]="multi.value"
[readonly]="readonly.value"
[disabled]="disabled.value"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export class CdkListboxExample {
focusMode: 'roving' | 'activedescendant' = 'roving';
selectionMode: 'explicit' | 'follow' = 'explicit';

selection = [];

wrap = new FormControl(true, {nonNullable: true});
multi = new FormControl(false, {nonNullable: true});
disabled = new FormControl(false, {nonNullable: true});
Expand Down
Loading