fix(cdk-experimental/listbox): initial listbox focus state #30764
Conversation
wagnermaciel
commented
Apr 1, 2025
wagnermaciel
commented
- The first option to receive focus in a listbox should be either the first selected option or the first option in the list if no option is selected.
wagnermaciel
added
the
target: major
* The first option to receive focus in a listbox should be either the first selected option or the first option in the list if no option is selected.
wagnermaciel
force-pushed
the
ng-aria-listbox
branch
from
82798bd to
c2019dc
Compare
wagnermaciel
requested review from
crisbeto and
mmalerba
and removed request for
a team
| constructor() { | ||
| effect(() => { | ||
| if (this._isViewInitialized() && !this._touched()) { | ||
| const index = this.items().findIndex(i => this.value().includes(i.value())); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I mean the this.items(), this.value(), i.value() - these will all trigger the effect right?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
