Skip to content

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

Merged
merged 28 commits into from
Aug 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
bdc7cfd
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
1148c68
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
70b63d7
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
e8e51d3
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
56ae4c1
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
2276f61
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
0042fac
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
1b7bf87
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
a9b351d
feat(dev-app/listbox): added cdk listbox example to the dev-app.
nielsr98 Jul 15, 2020
93c2bb7
fix(listbox): removed duplicate dep in dev-app build file.
nielsr98 Jul 22, 2020
4d43cf1
fix(listbox): deleted unused files.
nielsr98 Aug 4, 2020
8a05672
refactor(combobox): changed names and made coerceOpenActionProperty s…
nielsr98 Aug 10, 2020
3a8cfee
fix(combobox): fixed trailing whitespace.
nielsr98 Aug 10, 2020
1820b1b
refactor(listbox): added default classes to listbox and option, and m…
nielsr98 Aug 17, 2020
b7ad036
refactor(listbox): added focus management for focusing listbox.
nielsr98 Aug 17, 2020
9084381
feat(listbox): added support for horizontal listbox.
nielsr98 Aug 17, 2020
ee6c9aa
refactor(combobox): added support for array of values which enables m…
nielsr98 Aug 17, 2020
77d7261
feat(combobox): let the user determine what element is focused first …
nielsr98 Aug 17, 2020
e7bdd1f
feat(combobox): pressing down when focused on text trigger will move …
nielsr98 Aug 17, 2020
a808333
refactor(listbox): updated listbox key management to handle horizonta…
nielsr98 Aug 17, 2020
602c20d
refactor(listbox): prevented listbox from auto closing when in multip…
nielsr98 Aug 17, 2020
7ae6ee2
fix(combobox): fixed lint errors.
nielsr98 Aug 17, 2020
b467789
refactor(combobox): made updates to focus management and keyboard han…
nielsr98 Aug 17, 2020
123cc2d
fix(combobox): fixed keyboard logic bug that only handled down arrow.
nielsr98 Aug 17, 2020
fbd9333
fix(combobox): close popup on Tab.
nielsr98 Aug 17, 2020
9e8d569
refactor(listbox): added comments and todos throughout.
nielsr98 Aug 17, 2020
2c4aa1a
fix(listbox): fixed listbox id to be cdk-listbox instead of cdk-option.
nielsr98 Aug 17, 2020
85a01c1
fix(listbox): prevent SPACE from scrolling scren.
nielsr98 Aug 17, 2020
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
9 changes: 7 additions & 2 deletions src/cdk-experimental/combobox/combobox-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {Subject} from 'rxjs';
})
export class CdkComboboxPanel<T = unknown> {

valueUpdated: Subject<T> = new Subject<T>();
valueUpdated: Subject<T | T[]> = new Subject<T | T[]>();
contentIdUpdated: Subject<string> = new Subject<string>();
contentTypeUpdated: Subject<AriaHasPopupValue> = new Subject<AriaHasPopupValue>();

Expand All @@ -32,7 +32,7 @@ export class CdkComboboxPanel<T = unknown> {
) {}

/** Tells the parent combobox to close the panel and sends back the content value. */
closePanel(data?: T) {
closePanel(data?: T | T[]) {
this.valueUpdated.next(data);
}

Expand All @@ -44,6 +44,11 @@ export class CdkComboboxPanel<T = unknown> {

/** Registers the content's id and the content type with the panel. */
_registerContent(contentId: string, contentType: AriaHasPopupValue) {
// If content has already been registered, no further contentIds are registered.
if (this.contentType && this.contentType !== contentType) {
return;
}
Comment on lines +48 to +50
Copy link
Member

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?


this.contentId = contentId;
if (contentType !== 'listbox' && contentType !== 'dialog') {
throw Error('CdkComboboxPanel currently only supports listbox or dialog content.');
Expand Down
30 changes: 28 additions & 2 deletions src/cdk-experimental/combobox/combobox-popup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 this.elementRef.nativeElement

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 CdkDialog would take care of that automatically).


@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>,
) { }

Expand All @@ -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();
}
}
}
54 changes: 44 additions & 10 deletions src/cdk-experimental/combobox/combobox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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'];

Expand Down Expand Up @@ -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; }
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Add a JsDoc description that explains what this property does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}
}
}

Expand All @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

I would add a TODO to also test for contenteditable (I think supporting that will take some more thought before doing it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(),
Expand Down Expand Up @@ -247,5 +280,6 @@ export class CdkCombobox<T = unknown> implements OnDestroy, AfterContentInit {
}

static ngAcceptInputType_openActions: OpenActionInput;
static ngAcceptInputType_autoSetText: OpenActionInput;
static ngAcceptInputType_disabled: BooleanInput;
}
10 changes: 6 additions & 4 deletions src/cdk-experimental/listbox/listbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
} from '@angular/cdk/testing/private';
import {A, DOWN_ARROW, END, HOME, SPACE} from '@angular/cdk/keycodes';
import {FormControl, FormsModule, ReactiveFormsModule} from '@angular/forms';
import {CdkCombobox, CdkComboboxModule} from '@angular/cdk-experimental/combobox';
import {CdkCombobox, CdkComboboxModule, CdkComboboxPanel} from '@angular/cdk-experimental/combobox';


describe('CdkOption and CdkListbox', () => {
Expand Down Expand Up @@ -853,7 +853,7 @@ describe('CdkOption and CdkListbox', () => {
expect(comboboxInstance.value).toBe('solar');
});

it('should not update combobox if listbox is in multiple mode', () => {
it('should not close panel if listbox is in multiple mode', () => {
expect(comboboxInstance.value).toBeUndefined();
expect(comboboxInstance.isOpen()).toBeFalse();

Expand All @@ -875,10 +875,11 @@ describe('CdkOption and CdkListbox', () => {

listboxInstance.setActiveOption(optionInstances[1]);
dispatchKeyboardEvent(listboxElement, 'keydown', SPACE);
testComponent.panel.closePanel(testComponent.listbox.getSelectedValues());
fixture.detectChanges();

expect(comboboxInstance.isOpen()).toBeTrue();
expect(comboboxInstance.value).toBeUndefined();
expect(comboboxInstance.isOpen()).toBeFalse();
expect(comboboxInstance.value).toEqual(['solar']);
});
});
});
Expand Down Expand Up @@ -1009,6 +1010,7 @@ class ListboxInsideCombobox {
isDisabled: boolean = false;
isMultiselectable: boolean = false;
@ViewChild(CdkListbox) listbox: CdkListbox<string>;
@ViewChild('panel') panel: CdkComboboxPanel<string>;

onSelectionChange(event: ListboxSelectionChangeEvent<string>) {
this.changedOption = event.option;
Expand Down
Loading