-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Combobox listbox refactor #20339
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
Combobox listbox refactor #20339
Changes from all commits
bdc7cfd
1148c68
70b63d7
e8e51d3
56ae4c1
2276f61
0042fac
1b7bf87
a9b351d
93c2bb7
4d43cf1
8a05672
3a8cfee
1820b1b
b7ad036
9084381
ee6c9aa
77d7261
e7bdd1f
a808333
602c20d
7ae6ee2
b467789
123cc2d
fbd9333
9e8d569
2c4aa1a
85a01c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,14 @@ | |
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
import {Directive, Inject, InjectionToken, Input, OnInit, Optional} from '@angular/core'; | ||
import { | ||
Directive, | ||
ElementRef, | ||
Inject, | ||
InjectionToken, | ||
Input, | ||
OnInit, | ||
Optional} from '@angular/core'; | ||
import {AriaHasPopupValue, CdkComboboxPanel} from './combobox-panel'; | ||
|
||
export const PANEL = new InjectionToken<CdkComboboxPanel>('CdkComboboxPanel'); | ||
|
@@ -20,7 +27,8 @@ let nextId = 0; | |
'class': 'cdk-combobox-popup', | ||
'[attr.role]': 'role', | ||
'[id]': 'id', | ||
'tabindex': '-1' | ||
'tabindex': '-1', | ||
'(focus)': 'focusFirstElement()' | ||
} | ||
}) | ||
export class CdkComboboxPopup<T = unknown> implements OnInit { | ||
|
@@ -33,11 +41,21 @@ export class CdkComboboxPopup<T = unknown> implements OnInit { | |
} | ||
private _role: AriaHasPopupValue = 'dialog'; | ||
|
||
@Input() | ||
get firstFocus(): HTMLElement { | ||
return this._firstFocusElement; | ||
} | ||
set firstFocus(id: HTMLElement) { | ||
this._firstFocusElement = id; | ||
} | ||
private _firstFocusElement: HTMLElement; | ||
Comment on lines
+44
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite follow this; if this directive is for dialog combobox content, I think the focus element would always be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
@Input() id = `cdk-combobox-popup-${nextId++}`; | ||
|
||
@Input('parentPanel') private readonly _explicitPanel: CdkComboboxPanel; | ||
|
||
constructor( | ||
private readonly _elementRef: ElementRef<HTMLElement>, | ||
@Optional() @Inject(PANEL) readonly _parentPanel?: CdkComboboxPanel<T>, | ||
) { } | ||
|
||
|
@@ -52,4 +70,12 @@ export class CdkComboboxPopup<T = unknown> implements OnInit { | |
this._parentPanel._registerContent(this.id, this._role); | ||
} | ||
} | ||
|
||
focusFirstElement() { | ||
if (this._firstFocusElement) { | ||
this._firstFocusElement.focus(); | ||
} else { | ||
this._elementRef.nativeElement.focus(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
|
||
export type OpenAction = 'focus' | 'click' | 'downKey' | 'toggle'; | ||
export type OpenActionInput = OpenAction | OpenAction[] | string | null | undefined; | ||
|
||
|
@@ -30,7 +31,7 @@ import { | |
} from '@angular/cdk/overlay'; | ||
import {Directionality} from '@angular/cdk/bidi'; | ||
import {BooleanInput, coerceBooleanProperty, coerceArray} from '@angular/cdk/coercion'; | ||
import {DOWN_ARROW, ESCAPE} from '@angular/cdk/keycodes'; | ||
import {DOWN_ARROW, ENTER, ESCAPE, TAB} from '@angular/cdk/keycodes'; | ||
|
||
const allowedOpenActions = ['focus', 'click', 'downKey', 'toggle']; | ||
|
||
|
@@ -58,7 +59,7 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
private _panel: CdkComboboxPanel<T> | undefined; | ||
|
||
@Input() | ||
value: T; | ||
value: T | T[]; | ||
|
||
@Input() | ||
get disabled(): boolean { return this._disabled; } | ||
|
@@ -74,9 +75,16 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
} | ||
private _openActions: OpenAction[] = ['click']; | ||
|
||
/** Whether the textContent is automatically updated upon change of the combobox value. */ | ||
@Input() | ||
get autoSetText(): boolean { return this._autoSetText; } | ||
set autoSetText(value: boolean) { this._autoSetText = coerceBooleanProperty(value); } | ||
private _autoSetText: boolean = true; | ||
Comment on lines
+79
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a JsDoc description that explains what this property does? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah will do. |
||
|
||
@Output('comboboxPanelOpened') readonly opened: EventEmitter<void> = new EventEmitter<void>(); | ||
@Output('comboboxPanelClosed') readonly closed: EventEmitter<void> = new EventEmitter<void>(); | ||
@Output('panelValueChanged') readonly panelValueChanged: EventEmitter<T> = new EventEmitter<T>(); | ||
@Output('panelValueChanged') readonly panelValueChanged: EventEmitter<T[]> | ||
= new EventEmitter<T[]>(); | ||
|
||
private _overlayRef: OverlayRef; | ||
private _panelContent: TemplatePortal; | ||
|
@@ -114,14 +122,28 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
_keydown(event: KeyboardEvent) { | ||
const {keyCode} = event; | ||
|
||
if (keyCode === DOWN_ARROW && this._openActions.indexOf('downKey') !== -1) { | ||
this.open(); | ||
if (keyCode === DOWN_ARROW) { | ||
if (this.isOpen()) { | ||
this._panel?.focusContent(); | ||
} else if (this._openActions.indexOf('downKey') !== -1) { | ||
this.open(); | ||
} | ||
} else if (keyCode === ENTER) { | ||
if (this._openActions.indexOf('toggle') !== -1) { | ||
this.toggle(); | ||
} else if (this._openActions.indexOf('click') !== -1) { | ||
this.open(); | ||
} | ||
|
||
} else if (keyCode === ESCAPE) { | ||
event.preventDefault(); | ||
this.close(); | ||
} else if (keyCode === TAB) { | ||
this.close(); | ||
} | ||
} | ||
|
||
/** Handles click or focus interactions. */ | ||
_handleInteractions(interaction: OpenAction) { | ||
if (interaction === 'click') { | ||
if (this._openActions.indexOf('toggle') !== -1) { | ||
|
@@ -136,6 +158,7 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
} | ||
} | ||
|
||
/** Given a click in the document, determines if the click was inside a combobox. */ | ||
_attemptClose(event: MouseEvent) { | ||
if (this.isOpen()) { | ||
let target = event.composedPath ? event.composedPath()[0] : event.target; | ||
|
@@ -163,7 +186,9 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
this.opened.next(); | ||
this._overlayRef = this._overlayRef || this._overlay.create(this._getOverlayConfig()); | ||
this._overlayRef.attach(this._getPanelContent()); | ||
this._panel?.focusContent(); | ||
if (!this._isTextTrigger()) { | ||
this._panel?.focusContent(); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -189,22 +214,30 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
return this.disabled ? null : '0'; | ||
} | ||
|
||
private _setComboboxValue(value: T) { | ||
private _setComboboxValue(value: T | T[]) { | ||
|
||
const valueChanged = (this.value !== value); | ||
this.value = value; | ||
|
||
if (valueChanged) { | ||
this.panelValueChanged.emit(value); | ||
this._setTextContent(value); | ||
this.panelValueChanged.emit(coerceArray(value)); | ||
if (this._autoSetText) { | ||
this._setTextContent(value); | ||
} | ||
} | ||
} | ||
|
||
private _setTextContent(content: T) { | ||
private _setTextContent(content: T | T[]) { | ||
const contentArray = coerceArray(content); | ||
this._elementRef.nativeElement.textContent = contentArray.join(' '); | ||
} | ||
|
||
private _isTextTrigger() { | ||
// TODO: Should check if the trigger is contenteditable. | ||
const tagName = this._elementRef.nativeElement.tagName.toLowerCase(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a TODO to also test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. |
||
return tagName === 'input' || tagName === 'textarea' ? true : false; | ||
} | ||
|
||
private _getOverlayConfig() { | ||
return new OverlayConfig({ | ||
positionStrategy: this._getOverlayPositionStrategy(), | ||
|
@@ -247,5 +280,6 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit { | |
} | ||
|
||
static ngAcceptInputType_openActions: OpenActionInput; | ||
static ngAcceptInputType_autoSetText: OpenActionInput; | ||
static ngAcceptInputType_disabled: BooleanInput; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that explains that changing the content type after opening isn't allowed?