angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.36k stars 6.74k forks source link

CDK: ListKeyManager activeItem has incorrect reference after QueryList changes #14345

Open kherock opened 5 years ago

kherock commented 5 years ago

Bug, feature request, or proposal:

Bug/proposal

What is the expected behavior?

The active item and and the item at the active index should always refer to the same item.

What is the current behavior?

To illustrate the problem, with the FocusKeyManager, if a component implementing FocusableOption is removed from the view and destroyed, keyManager.activeItem will still have a reference to the destroyed component.

What are the steps to reproduce?

Providing a StackBlitz reproduction is the best way to share your issue.
StackBlitz: https://stackblitz.com/edit/angular-material2-issue-h8nz1b

What is the use-case or motivation for changing an existing behavior?

Currently in one of my components, I have a setup similar to how the MatStepHeader component sets tabindex to 0 or -1 depending on if a step is the activeItem in the FocusKeyManager. If this list changes such that the active item is removed from the DOM, all the tabindexes in the list are -1 since activeItem still points to the destroyed instance. I can also think of a few other cases where having mismatched activeItemIndex and activeItem references could cause problems.

Now I could watch for changes on the QueryList myself, however the FocusKeyManager doesn't provide an interface for updating the active item without setting the focus. This is problematic if the list isn't currently focused and changing the active item would potentially steal focus away from an input field. To workaround this without accessing the private members directly, I would need to essentially recreate and track most of the key manager's state myself in the component.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Angular 7.1 Material 7.1 TS: 3.1

Is there anything else we should know?

I feel like this is just an unhandled case at https://github.com/angular/material2/blob/b1045eecd97f2beb2d4ba2d2025a582a9cdad390/src/cdk/a11y/key-manager/list-key-manager.ts#L69-L71

The behavior I'm looking for is

          // preserve index in the case where the item was removed
          if (newIndex > -1) {
            this._activeItemIndex = newIndex;
          }
          if (this._activeItemIndex > -1) {
            const itemArray = this._getItemsArray();
            this._activeItem = itemArray[this._activeItemIndex];
          } else {
            this._activeItem = undefined;
          }
angular-automatic-lock-bot[bot] commented 5 years ago

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.