fix(cdk/a11y): clear active item in key manager if it is removed from the list

[crisbeto]

The active item in the ListKeyManager gets updated on keyboard events, but if the list changes between them, we may end up in a state where it's pointing to an item that's not in the DOM anymore. This can cause something like aria-activedescendant to point to an invalid element.

These changes add some logic that clear the active element if it's removed from the list and there's nothing at its new index.

[devversion]

LGTM. Related question: do you know why the activeItemIndex getter is typed to return null while we use -1 here?

[crisbeto]

I don't remember, it might be because activeItem can be null too. I went for -1, because it's what we use in other places when we want to clear the active item.

[jelbourn]

LGTM

[vanessanschmitt]

Thank you @crisbeto for fixing this!! @jelbourn Can this be merged soon? My team needs this logic.

[jelbourn]

@vanessanschmitt looks like there are ~5 failing tap targets that need debugging for this one

[vanessanschmitt]

Oh shoot, okay! Thank you for letting me know, and for bumping the priority!

[br-kwon]

LGTM. Related question: do you know why the activeItemIndex getter is typed to return null while we use -1 here?

I actually had a similar question! Seems like activeItemIndex is always a number from initialization... except for on QueryList.changes. What confuses me is that both QueryList.toArray() and QueryList.toArray().indexOf() returns any. 🤷‍♂️

[mmalerba]

I did another presubmit of this, and it seems like there's a change after check error in some cases:

ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'aria-activedescendant: mat-option-136'. Current value: 'aria-activedescendant: mat-option-139'

That team's logic is a little hard to follow, there's some observable that causes the options to change. Maybe if we just set aria-activedescendant directly instead of through a binding it would help?

[crisbeto]

Setting it directly in their code should help, but is it the only place where it fails like this? I wonder whether it isn't a symptom of a deeper issue.

[andrewseguin]

I've tried debugging the ~5 failures but have not been able to figure out the issue. For some reason, the updateActiveItem seems to be called during change detection. If I change it to setTimeout(() => this.updateActiveItem(this._activeItemIndex)); then things pass again.

[crisbeto]

Is it happening for a specific component?

[andrewseguin]

Yeah its for autocomplete - I tried to make a reproduction but couldn't manage it. We have a different change detection strategy in Google that makes it hard to simulate it in our spec

[crisbeto]

It's worth noting that the autocomplete is also a bit of a special case, because it hides some logic behind a Promise.resolve which has to be flushed in tests.

[andrewseguin]

Hmm interesting, I wonder if that's causing the syncing issue? I tried adding detect changes in several places, especially throughout the tests but it didnt work

[crisbeto]

Flushing those promises requires calls to tick as well. There are some examples in the autocomplete tests: https://github.com/angular/components/blob/master/src/material/autocomplete/autocomplete.spec.ts#L250.

[andrewseguin]

Unfortunately adding tick and flush after each line of the test didn't work

[andrewseguin]

Would it be crazy to just get this in by making it use a timeout to escape the change detection? We could leave a note saying its suboptimal but necessary to just get it in. Is it possible we could get into bad race conditions if we did that?

[crisbeto]

My concern with the timeout is that it could cause even more fakeAsync tests to fail since they'll have a new timeout that needs to be flushed.

[andrewseguin]

This may be a good example for future team discussion on how we should handle PRs that are too time-consuming to merge. I'd love to spend more time debugging what the issue is, but it's hard to prioritize that work over other stuff =\

[andrewseguin]

EDIT: The following suggestion doesn't work :( it was okay because the code isn't emitting changes. This should be emitting setActiveItem instead of updateActiveItem, which causes changes to emit again and introduces back the expression issue.

This has the same issues as #14471 - there is a chance of an ExpressionChangedAfterItHasBeenCheckedError for components using the active option for aria-activedescendent (autocomplete trigger, select, others?). This is because components directly bind to the active item in the key manager and it appears to be changing during change detection.

It looks like a way to fix this is to save the active item when the key manager emits the active item index in the change stream.

For example, in autocomplete.ts add something like this:

  /** The latest active option set by the key manager. */
  _activeOption: MatOption | null;

  ngAfterContentInit() {
    ...
    this._activeOptionChanges = this._keyManager.change.subscribe(index => {
      if (this.isOpen) {
        this._activeOption = this.options.toArray()[index] || null;
        ...
      }
    }); 
    ...
  }

And then in the autocomplete-trigger.ts, change get activeOption so that it uses this property instead of directly accessing the key manager's values:

  /** The currently active option, coerced to MatOption type. */
  get activeOption(): MatOption | null {
    return this.autocomplete?._activeOption || null;
  }

There's a few more tests to debug but this gets rid of the difficult expression issues

[andrewseguin]

Unfortunately after several attempts of fixes, this PR just won't be able to land. Initially this presents some ExpressionChangedAfterItHasBeenCheckedError errors when autocomplete and select update the aria-activedescendants attribute. Changing this to a manual setter fixes those, but reveals that theres another issue with MatOption's active and selected class state. Even with those all changed to direct setters through the DOM, many tests fail because of assumptions about the component states during change detection.

Also its not clear whether updateActiveItem or setActiveItem should be set. If its the latter, some clients might have receive unexpected events where the select changes even though the user didn't actually change something.

The root issue here seems to be that it is not safe to change the active item during change detection, which this may be doing. Changes to the value could have side effects to bindings that have already been checked in another view template.

My recommendation is that this PR becomes the addition of a comment in the ListKeyManager that this is a known issue and does not have a good solution.