feat(cdk-experimental/listbox): selection logic and testing for listbox.

[nielsr98]

Added the basic logic to keep track of a selected state of options and to toggle that state by clicking on them. The selected state of an option is tied to aria-selected. The CdkListbox also handles providing unique optionIds to its child options. Currently, logic to toggle multi-select in the listbox is not yet included, so by default the listbox has multi-select enabled. Tests were included to listbox. Feedback on testing style is appreciated.

[scriby]

I was thinking about this and had one idea for how we can get rid of the QueryList. I think it's a little unorthodox for Angular, but could still be worth considering if the performance is significantly better.

The basic idea is just to store a reference to the Angular component on the option's DOM element itself. Then, in this click handler we can just access it via $event.target._ngComponent without having to search for it again via the loop.

To store the component reference, I think it'd be as simple as this:

constructor(private readonly elementRef: ElementRef) {}

ngOnInit() {
  this.elementRef.nativeElement._ngComponent = this;
}

This change isn't required to complete the review, I'm just raising it up for discussion.

[nielsr98]

Interesting idea. I was planning on doing some performance comparisons and testing this weekend and refactor it afterwards.

[jelbourn]

One problem here: the option element may not actually be the click target; the option may (likely will) contain other elements. This would mean you'd need to walk up the DOM to get the option. You could do something here like:

updateSelection(event: MouseEvent) {
  const optionElement = getClosestOption(event.target);
  const selectedOption = this._options.find(o => o.id === optionElement.id);
  ...
}

As for attaching the component instance to the DOM- it's not particularly aligned with Angular's development model. The main thing I'd worry about is leaking memory since the DOM element and the component would reference each other.

[scriby]

Agreed on the solution being weird in Angular (although I'd also argue Angular is a little deficient for handling this particular use case), but I don't think memory leaking is an issue as long as the reference is unset in ngOnDestroy.

You might be remembering the circular reference issue from IE6/IE7 (https://stackoverflow.com/questions/10092619/precise-explanation-of-javascript-dom-circular-reference-issue), but this particular one hasn't been an issue for a while :)

[jelbourn]

I guess I'm showing my age

[nielsr98]

Another method is to have the option listen for the click and toggle its selected state on click. Then it uses an injected reference to its parent listbox to trigger the listbox's selectionChange event emitter. However I think this may go against the "read not write" relationship the option should have with its parent listbox.

[jelbourn]

One problem here: the option element may not actually be the click target; the option may (likely will) contain other elements. This would mean you'd need to walk up the DOM to get the option. You could do something here like:

updateSelection(event: MouseEvent) {
  const optionElement = getClosestOption(event.target);
  const selectedOption = this._options.find(o => o.id === optionElement.id);
  ...
}

As for attaching the component instance to the DOM- it's not particularly aligned with Angular's development model. The main thing I'd worry about is leaking memory since the DOM element and the component would reference each other.

[nielsr98]

Updated click handling. Feedback addressed.

[jelbourn]

Looks like there are still a few unresolved comments from the previous round

[jelbourn]

optional: you could also test that all of the IDs are different

[nielsr98]

In making this test, I realized the .includes() method for arrays isn't supported. I think it came with ES7. Is this expected for the current Angular version?

[jelbourn]

I would combine this method and toggleSelected into a single method just called toggle

toggle() {
   this.selected = !this.selected;
  this.listbox._emitChangeEvent(this);
}

[nielsr98]

So for a function as simple as this one, I suppose its not necessary to have a function name that is more descriptive than just "toggle"

[jelbourn]

Yeah, my philosophy is to name methods for what they do rather than how they're called

[nielsr98]

New feedback addressed.

[jelbourn]

Suggested change

	let optionElements: Element[];
	let optionElements: HTMLElement[];

You can typically be a little bit more specific with these and specify HTMLElement

[jelbourn]

Rather than setting up a spy on the internal listbox method, you can have your ListboxWithCdkOptions component below add a (selectionChange) handler to the listbox and assert that that's called. Generally you want to avoid having the test know about the internals of the component unless there's no other practical way to test something.

[nielsr98]

Ok I see. Will change.

[jelbourn]

Suggested change

	class ListboxWithCdkOptions {
	class ListboxWithOptions {

can also omit Cdk form the option part

[jelbourn]

It does need to be the native id property since aria-activedescendant relies on id. You can look at MatCheckbox for an example how we do do this, but by making id an @Input and assigning it an initial value, you ensure that it always has an ID while also letting the user set one if they need to for some reason (but people generally don't want to, that's why they're using a common component).

[nielsr98]

I see I think select does that as well. I'll fix this.

[jelbourn]

I forgot if I mentioned this earlier, but in a follow-up we'll want to change this from emitting CdkOption to emitting an event interface (something like ListboxSelectionChangeEvent)

[nielsr98]

Alright sounds good.

[nielsr98]

I looked into it and it seemed pretty straightforward so I rolled it into a commit in this PR. If I didn't implement it right or you think it should go in separately I can remove it.

[nielsr98]

Addressed new feedback. Ready for review.

[jelbourn]

Okay, this should be the last last comments

[jelbourn]

This is fine for now since we're going to change the event type later anyway, but you could just store the selection here and check that in the test rather than spying on the methods
(it's always better to avoid spies when you can avoid it, since they often end up just testing implementations details instead of intended functionality)

[jelbourn]

This can just be [id]

[jelbourn]

I think this could be simplified to

@Input() id = `cdk-opt-${nextId++}`;

If the user specifies a value, it will override write this after this initial value is set, and you won't need the _uniqueId property at all.

[jelbourn]

I would typically put this above the class where it's used

[nielsr98]

Ah true I used to use it in Listbox and neglected moving it.

[nielsr98]

Ready for review

[jelbourn]

LGTM

[angular-automatic-lock-bot]

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