Combobox listbox refactor #20339
Combobox listbox refactor #20339
Conversation
…impler for this PR.
…ade aria-active descendant not the default.
…ultiselect listbox to be compatible.
…le mode in a combobox.
googlebot
added
the
cla: yes
| if (this.contentType && this.contentType !== contentType) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add a comment that explains that changing the content type after opening isn't allowed?
| @Input() | ||
| get firstFocus(): HTMLElement { | ||
| return this._firstFocusElement; | ||
| } | ||
| set firstFocus(id: HTMLElement) { | ||
| this._firstFocusElement = id; | ||
| } | ||
| private _firstFocusElement: HTMLElement; |
There was a problem hiding this comment.
I don't quite follow this; if this directive is for dialog combobox content, I think the focus element would always be this.elementRef.nativeElement
There was a problem hiding this comment.
My thought process was that if the popup is a dialog, then this.elementRef.nativeElement.focus() will just focus the entire popup. With this input, the user could choose to immediately focus some interior element like an input or listbox inside the dialog.
There was a problem hiding this comment.
We should be able to take care of that with the focus trap from cdk/a11y without having to add direct support for it to the pop-up here (in fact CdkDialog would take care of that automatically).
| @Input() | ||
| get autoSetText(): boolean { return this._autoSetText; } | ||
| set autoSetText(value: boolean) { this._autoSetText = coerceBooleanProperty(value); } | ||
| private _autoSetText: boolean = true; |
There was a problem hiding this comment.
Add a JsDoc description that explains what this property does?
| const contentArray = coerceArray(content); | ||
| this._elementRef.nativeElement.textContent = contentArray.join(' '); | ||
| } | ||
|
|
||
| private _isTextTrigger() { | ||
| const tagName = this._elementRef.nativeElement.tagName.toLowerCase(); |
There was a problem hiding this comment.
I would add a TODO to also test for contenteditable (I think supporting that will take some more thought before doing it)
| const isArrow = (keyCode === UP_ARROW | ||
| || keyCode === DOWN_ARROW | ||
| || keyCode === LEFT_ARROW | ||
| || keyCode === RIGHT_ARROW); |
There was a problem hiding this comment.
Should this always be all four, or should it depend on the orientation?
There was a problem hiding this comment.
The isArrow variable is true if any arrow key is pressed. In the following if statement, a selection change will only occur if there is a new active option which the listKeyManager keeps track of. The listKeyManager knows the orientation, so if RIGHT is pressed while I vertical mode, the active item won't switch and even though isArrow is true no action is taken.
jelbourn
added
action: merge
|
You can add merge-ready when ready |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Added fixes based on feedback from Friday's test session.