Cdk listbox: multipleselect support and active descendant #19929

nielsr98 · 2020-07-09T19:32:43Z

Added support for toggling whether listbox can have multiple options selected. Also added aria active descendant support and the ability to let the user choose whether to use active descendant or to let the focus follow the active item.

…iles.

…descendant.

scriby · 2020-07-09T21:33:07Z

src/cdk-experimental/listbox/listbox.ts

@@ -89,19 +99,46 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
    this._active = false;
  }

+  /** Applies focus to the option. */
+  focus() {
+    console.log('focusing option');


Yep, thanks.

scriby · 2020-07-09T21:35:48Z

src/cdk-experimental/listbox/listbox.ts

+  private _multiple: boolean = false;
+  private _useActiveDescendant: boolean = true;
+  private _activeOption: CdkOption;
+  private readonly _destroy = new Subject<void>();


I think this is supposed to be a ReplaySubject(1)?

I'm not sure what the difference is. I'll look into it.

We don't actually use ReplaySubject anywhere in the library, I'd err on not pulling in the extra code unless we wanted to do this more broadly

scriby · 2020-07-09T21:37:04Z

src/cdk-experimental/listbox/listbox.ts

  ngAfterContentInit() {
    this._listKeyManager = new ActiveDescendantKeyManager(this._options)
      .withWrap().withVerticalOrientation().withTypeAhead();
+
+    this._listKeyManager.change.subscribe(() => {


This one should also have takeUntil(this._destroy$)?

scriby · 2020-07-09T21:38:20Z

src/cdk-experimental/listbox/listbox.ts

+    this._selectionModel.changed.pipe(takeUntil(this._destroy))
+        .subscribe((event: SelectionChange<CdkOption>) => {
+
+      event.added.forEach((option: CdkOption) => {


Do you know if for/of loops should be used here? That's the Google internal style

Yeah I've used them elsewhere, I forgot for this one. Thanks for the catch.

scriby · 2020-07-09T21:38:48Z

src/cdk-experimental/listbox/listbox.ts

+    this._activeOption = this._listKeyManager.activeItem;
+    this._activeOption.activate();
+
+    console.log('here in update active option');


scriby · 2020-07-09T21:39:49Z

src/cdk-experimental/listbox/listbox.spec.ts

+  template: `
+    <div cdkListbox
+         [useActiveDescendant]="isActiveDescendant"
+    >


I'm not sure which is the standard, but looks like this closing tag is on the next line, but in the previous template it's on the previous line

Yeah I think it should be on the previous line.

scriby · 2020-07-09T21:39:58Z

src/cdk-experimental/listbox/listbox.spec.ts

+@Component({
+  template: `
+    <div cdkListbox
+         [multiple]="isMultiselectable"


There's an extra space here?

Actually I think the (selectionChange) on the next line needs a space.

scriby · 2020-07-09T21:41:22Z

src/cdk-experimental/listbox/listbox.spec.ts

+        expect(option.getAttribute('aria-selected')).toBe('true');
+      }
+
+      expect(fixture.componentInstance.changedOption).toBeDefined();


I typically avoid toBeDefined since it returns true for "null", which can be a little confusing. ".not.toBe(undefined);" is less mistakable. May not matter for this usage, up to you.

Since I am checking changedOption.id directly after, do you think it is even necessary to check that changedOption is defined?

…anged for each loop to for of loop.

…toggle function.

jelbourn

Got part of the way through before my next meeting, will do another pass in a bit

jelbourn · 2020-07-10T00:33:41Z

src/cdk-experimental/listbox/listbox.ts

+    for (let i = 0; i < element.children.length; i++) {
+      const node = element.children[i];
+      if (this._isIcon(node)) {
+        element.removeChild(node);
+      } else {
+        this._removeIcons(node);
+      }
+    }


Suggested change

for (let i = 0; i < element.children.length; i++) {

const node = element.children[i];

if (this._isIcon(node)) {

element.removeChild(node);

} else {

this._removeIcons(node);

}

}

for (const icon of Array.from(element.querySelectorAll('mat-icon, .material-icons'))) {

icon.parentNode.removeChild(icon);

}

I would also add a TODO in the code to make the matching for ligature icons configurable

jelbourn · 2020-07-10T15:37:48Z

src/cdk-experimental/listbox/listbox.ts

+  private _multiple: boolean = false;
+  private _useActiveDescendant: boolean = true;
+  private _activeOption: CdkOption;
+  private readonly _destroy = new Subject<void>();


We don't actually use ReplaySubject anywhere in the library, I'd err on not pulling in the extra code unless we wanted to do this more broadly

jelbourn · 2020-07-10T15:40:08Z

src/cdk-experimental/listbox/listbox.ts

  ngAfterContentInit() {
    this._listKeyManager = new ActiveDescendantKeyManager(this._options)
      .withWrap().withVerticalOrientation().withTypeAhead();
+


I would break up the content of ngAfterContentInit here to another function (or multiple functions). In general, when you have a lot of code in a lifecycle method, there's nothing that communicates what the purpose/intent of that code is, only when it gets called.

Sure that makes sense.

jelbourn · 2020-07-10T15:41:15Z

src/cdk-experimental/listbox/listbox.ts

+    if (this.multiple && !value) {
+      this.setAllSelected(false);
+    } else if (!this.multiple && value) {
+      this._selectionModel = new SelectionModel<CdkOption>(value, this._selectionModel.selected);
+    }
+    this._multiple = coerceBooleanProperty(value);


I would probably move this code into its own function

Yeah will do.

jelbourn · 2020-07-10T15:43:38Z

src/cdk-experimental/listbox/listbox.ts

+    return this._multiple;
+  }
+  set multiple(value: boolean) {
+    if (this.multiple && !value) {


I would add a comment that explains the intent behind clearing the selection from going from multiple to single

jelbourn · 2020-07-10T16:50:32Z

src/cdk-experimental/listbox/listbox.ts

+  }
+
+  /** Remove any child from the given element which can be identified as an icon. */
+  private _removeIcons(element: Element) {


This could be a module-level function rather than a member on Option

jelbourn · 2020-07-10T16:56:04Z

src/cdk-experimental/listbox/listbox.ts

+    return this._useActiveDescendant;
+  }
+  set useActiveDescendant(shouldUseActiveDescendant: boolean) {
+    this._useActiveDescendant = shouldUseActiveDescendant;


coerceBooleanProperty here

jelbourn · 2020-07-10T16:56:49Z

src/cdk-experimental/listbox/listbox.ts

+  private _multiple: boolean = false;
+  private _useActiveDescendant: boolean = true;
+  private _activeOption: CdkOption;
+  private readonly _destroy = new Subject<void>();


Suggested change

private readonly _destroy = new Subject<void>();

private readonly _destroyed = new Subject<void>();

jelbourn · 2020-07-10T17:57:13Z

src/cdk-experimental/listbox/listbox.ts

+    if (this._useActiveDescendant
+        && this._listKeyManager && this._listKeyManager.activeItem) {
+      return this._listKeyManager.activeItem.id;


Suggested change

if (this._useActiveDescendant

&& this._listKeyManager && this._listKeyManager.activeItem) {

return this._listKeyManager.activeItem.id;

return this._useActiveDescendant ? this._listKeyManager?.activeItem?.id : null;

jelbourn · 2020-07-10T17:58:18Z

src/cdk-experimental/listbox/listbox.ts

+  }
+
+  /** Updates the activeOption and the active and focus properties of the option. */
+  updateActiveOption() {


This looks like it should be a private method?

jelbourn · 2020-07-10T23:59:47Z

src/cdk-experimental/listbox/listbox.ts

  }

  /** Toggles the selected state, emits a change event through the injected listbox. */
  toggle() {
    if (!this._isInteractionDisabled()) {
      this.selected = !this.selected;
      this.listbox._emitChangeEvent(this);
+      this.listbox._updateSelectionModel(this);


This is probably something we want to avoid, since now the option is mutating the listbox state. I would change this in one of two ways:

Clicking the option emits on a stream, which the listbox listens to and updates its own state

Handle clicks completely in the listbox instead of on the option

…tters.

nielsr98 · 2020-07-13T19:33:04Z

Feedback addressed. Ready for review.

jelbourn · 2020-07-13T22:36:49Z

src/cdk-experimental/listbox/listbox.ts

  /** Returns true if the option or listbox are disabled, and false otherwise. */
  _isInteractionDisabled(): boolean {
    return (this.listbox.disabled || this._disabled);
  }

+  private _emitSelectionChange(isUserInput = false) {
+    this.selectionChange.emit(new OptionSelectionChangeEvent(this, isUserInput));


Rather than making this a concrete type, I would make this event type an interface and just return an object literal here that conforms to that interface. This gives us more flexibility to change the event object over time.

Okay makes sense. I'm attempting this but might not be exactly what you're suggesting. Let me know if it needs changing when I push it.

I guess this makes sense for the Listbox event as well. I'm adding it to my next push but if you think that's a bad idea let me know.

src/cdk-experimental/listbox/listbox.ts

jelbourn · 2020-07-13T22:45:35Z

src/cdk-experimental/listbox/listbox.ts

+   * Whether the listbox allows multiple options to be selected.
+   * When multiple switches from true to false, all options are deselected.
+   * This is done rather than randomly choosing one of the selected options
+   * to remain selected.


Suggested change

* Whether the listbox allows multiple options to be selected.

* When multiple switches from true to false, all options are deselected.

* This is done rather than randomly choosing one of the selected options

* to remain selected.

* Whether the listbox allows multiple options to be selected.

* If `multiple` switches from `true` to `false`, all options are deselected.

I would remove the rationale for the change behavior from the JsDoc description and instead put it in an inline comment in the setter. Also, you would use the word "arbitrarily" here rather than "randomly".

jelbourn · 2020-07-13T22:54:34Z

src/cdk-experimental/listbox/listbox.ts

+    }
+  }
+
+  private _handleMultipleSwitch(value: boolean) {


Suggested change

private _handleMultipleSwitch(value: boolean) {

/** Updates option selection state for when the `multiple` selection option changes. */

private _updateSelectionForMultiSelectionChange(updatedMultiple: boolean) {

I would change the method name here to be more explicit about what the function is doing, and add a JsDoc since "multiSelection" is potentially ambiguous (e.g. it could be interpreted as multiple options changing at once).

Yeah I had a hard time thinking of a concise name but you're right that its better to just make it clear.

jelbourn · 2020-07-13T22:58:31Z

src/cdk-experimental/listbox/listbox.ts

+        this._updateSelectionModel(option);
+      }
+
+      console.log(option.selected);


jelbourn · 2020-07-13T23:08:47Z

src/cdk-experimental/listbox/listbox.ts

+  private _initSelectionChange() {
+    this.optionSelectionChanges = defer(() => {


Does this need to be a function that gets called, or could optionSelectionChanges be set just as part of its definition?

Yeah it can be part of the definition. I can also make it readonly then. I'll change.

jelbourn · 2020-07-13T23:13:49Z

src/cdk-experimental/listbox/listbox.ts

+    if (previouslyActive) {
+      previouslyActive.deactivate();
+    }


Suggested change

if (previouslyActive) {

previouslyActive.deactivate();

}

previouslyActive?.deactivate();

jelbourn · 2020-07-13T23:19:27Z

src/cdk-experimental/listbox/listbox.spec.ts

+    });
+  });
+
+  describe('multiselectable tests', () => {


Suggested change

describe('multiselectable tests', () => {

describe('with multiple selection', () => {

Prefer naming describe sub-blocks like this so that the end result reads like a sentence, e.g.

listbox with muliple selection should select all options using the select all method

I see, noted.

nielsr98 · 2020-07-14T02:25:15Z

Feedback addressed. Ready for review.

jelbourn

Looking good, just a few last minor comments

jelbourn · 2020-07-14T18:58:36Z

src/cdk-experimental/listbox/listbox.ts

-    // TODO: improve to method to handle more complex combinations of elements and text
-    return this._elementRef.nativeElement.textContent;
+  /** Remove any child from the given element which can be identified as an icon. */
+  // TODO: make this a configurable function that can removed any desired type of node.


The TODO has to go inside the function above the for. If it's here, it breaks the connection between the function and its JsDoc block

jelbourn · 2020-07-14T19:10:23Z

src/cdk-experimental/listbox/listbox.ts

@@ -89,19 +93,58 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
    this._active = false;


Just noticed this incidentally while looking at something else: activate() checks _isInteractionDisabled, but deactivate doesn't

jelbourn · 2020-07-14T19:12:46Z

src/cdk-experimental/listbox/listbox.ts

@@ -89,19 +93,58 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
    this._active = false;
  }

+  select() {


Add JsDoc for public API methods (here and below)

jelbourn · 2020-07-14T19:13:55Z

src/cdk-experimental/listbox/listbox.ts

+    const changeEvent = {
+      source: this,
+      isUserInput: isUserInput
+    };
+    this.selectionChange.emit(changeEvent);


Suggested change

const changeEvent = {

source: this,

isUserInput: isUserInput

};

this.selectionChange.emit(changeEvent);

this.selectionChange.emit({

source: this,

isUserInput: isUserInput

});

it's fine to inline the object literal

Okay will change.

nielsr98 · 2020-07-14T23:02:10Z

Feedback addressed. Ready for another review.

jelbourn

LGTM

…pport (angular#19929)

angular-automatic-lock-bot · 2020-08-16T00:08:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

nielsr98 added 9 commits July 9, 2020 15:19

build: Added required files to listbox directory.

1f14151

build: added listbox option directive and renamed listbox directive f…

378ca05

…iles.

build: Added required files to listbox directory.

d5a4b19

build: added listbox option directive and renamed listbox directive f…

e11ce60

…iles.

build: Added required files to listbox directory.

bf9dadb

build: added listbox option directive and renamed listbox directive f…

a2e8339

…iles.

build: Added required files to listbox directory.

f248a82

build: added listbox option directive and renamed listbox directive f…

f1b291b

…iles.

feat(listbox): added support for non-multiple listbox and aria active…

057c921

…descendant.

nielsr98 requested a review from jelbourn as a code owner July 9, 2020 19:32

nielsr98 added the target: minor This PR is targeted for the next minor release label Jul 9, 2020

nielsr98 added 2 commits July 9, 2020 15:38

fix(listbox): formatted BUILD.bazel.

5e67e9a

fix(listbox): fixed lint errors.

637f1b5

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 9, 2020

nielsr98 added 2 commits July 9, 2020 16:15

fix(listbox): removed unused variable and fix lint error.

c7fb815

fix(listbox): removed unused import and fix docs style.

0678a25

nielsr98 requested a review from scriby July 9, 2020 21:29

scriby reviewed Jul 9, 2020

View reviewed changes

nielsr98 added 3 commits July 10, 2020 11:31

fix(listbox): removed leftover console logs, fixed formatting, and ch…

e27aaa9

…anged for each loop to for of loop.

refactor(listbox): removed toggleViaKeyboard.

4efbd05

refactor(listbox): changed listbox keyboard handling to use internal …

571d5eb

…toggle function.

jelbourn reviewed Jul 10, 2020

View reviewed changes

nielsr98 added 4 commits July 13, 2020 14:37

refactor(listbox): subscribe to stream of option selection change emi…

15fdf79

…tters.

nit(listbox): member naming.

c417d73

fix(listbox): coverted query list to array before iteration.

18c9faa

nit(listbox): fixed jsdoc format.

59205ef

jelbourn reviewed Jul 13, 2020

View reviewed changes

nielsr98 added 2 commits July 13, 2020 21:53

refactor(listbox): improved jsdocs, changed change events to interfaces.

c5ac3d5

fix(listbox): added semicolons.

6b51456

jelbourn reviewed Jul 14, 2020

View reviewed changes

refactor(listbox): add jsdoc comments and made event objects inline.

5790b14

jelbourn approved these changes Jul 15, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker labels Jul 15, 2020

jelbourn merged commit 7c49399 into angular:master Jul 16, 2020

ngwattcos pushed a commit to ngwattcos/components that referenced this pull request Jul 20, 2020

feat(cdk-experimental/listbox): multi-select and active descendant su…

f41f7ac

…pport (angular#19929)

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 16, 2020

	private readonly _destroy = new Subject<void>();
	private readonly _destroyed = new Subject<void>();

	private _handleMultipleSwitch(value: boolean) {
	/** Updates option selection state for when the `multiple` selection option changes. */
	private _updateSelectionForMultiSelectionChange(updatedMultiple: boolean) {

		private _initSelectionChange() {
		this.optionSelectionChanges = defer(() => {

	describe('multiselectable tests', () => {
	describe('with multiple selection', () => {

		@@ -89,19 +93,58 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
		this._active = false;

Cdk listbox: multipleselect support and active descendant #19929

Cdk listbox: multipleselect support and active descendant #19929

Conversation

nielsr98 commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsr98 commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsr98 commented Jul 14, 2020

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsr98 commented Jul 14, 2020

jelbourn left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Aug 16, 2020