Skip to content
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

fix(cdk/a11y): activeItem out of date if active index is removed from ListKeyManager #14471

Conversation

crisbeto
Copy link
Member

Fixes the activeItem on the ListKeyManager not matching the item at the activeItemIndex, if the activeItem is removed from the list.

Fixes #14345.

Note: this is a resubmit of #14407.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Dec 11, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 11, 2018
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Dec 11, 2018
@ngbot
Copy link

ngbot bot commented Dec 11, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: e2e_tests" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mmalerba
Copy link
Contributor

Caretaker note: caused change after checked errors in one test in g3

@crisbeto crisbeto force-pushed the 14345/list-key-manager-active-item-resubmit branch 2 times, most recently from 53486b5 to 3ee6b9d Compare December 17, 2018 21:54
@mmalerba mmalerba added the Accessibility This issue is related to accessibility (a11y) label Apr 16, 2019
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019
@mmalerba mmalerba removed their assignment Aug 12, 2019
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@yuriyward
Copy link
Contributor

yuriyward commented Apr 1, 2021

@crisbeto Hi, can you explain why ListKeyManager keeps the old activeItemIndex when it's not present in an array of new values for QueryList? Why this index is not set as -1? (from test list-key-manager.spec.ts:96 it looks as desired behavior)

Update: I found this PR, probably it will be changed soon. It causes a lot of problems when it's used together with virtual-scroll and searching functionality

@devversion devversion removed their request for review August 18, 2021 13:02
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@andrewseguin
Copy link
Contributor

I was able to get a passing test suite with these changes:

          if (newIndex !== this._activeItemIndex) {
            setTimeout(() => {
              this.setActiveItem(
                  newIndex > -1 ? newIndex : this._activeItemIndex);
            }, 0);
  1. Use setTimeout to avoid changed after check errors
  2. Call setActiveItem for clients depending on the this.change.next event output

@crisbeto crisbeto changed the title fix(a11y): activeItem out of date if active index is removed from ListKeyManager fix(cdk/a11y): activeItem out of date if active index is removed from ListKeyManager Jan 19, 2022
@crisbeto crisbeto force-pushed the 14345/list-key-manager-active-item-resubmit branch from 3ee6b9d to 7b8ef4d Compare January 19, 2022 13:10
… ListKeyManager

Fixes the `activeItem` on the `ListKeyManager` not matching the item at the `activeItemIndex`, if the `activeItem` is removed from the list.

Fixes angular#14345.
@crisbeto crisbeto force-pushed the 14345/list-key-manager-active-item-resubmit branch from 7b8ef4d to 719e2d8 Compare January 19, 2022 13:16
@andrewseguin andrewseguin merged commit e61c2fa into angular:master Jan 19, 2022
andrewseguin pushed a commit that referenced this pull request Jan 19, 2022
… ListKeyManager (#14471)

Fixes the `activeItem` on the `ListKeyManager` not matching the item at the `activeItemIndex`, if the `activeItem` is removed from the list.

Fixes #14345.

(cherry picked from commit e61c2fa)
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jan 19, 2022
Follow-up to angular#14471. Uses `setActiveItem` instead of `updateActiveItem` which was missed when addressing the feedback.
andrewseguin pushed a commit that referenced this pull request Jan 19, 2022
)

Follow-up to #14471. Uses `setActiveItem` instead of `updateActiveItem` which was missed when addressing the feedback.

(cherry picked from commit 0b436c4)
andrewseguin pushed a commit that referenced this pull request Jan 19, 2022
)

Follow-up to #14471. Uses `setActiveItem` instead of `updateActiveItem` which was missed when addressing the feedback.
@andrewseguin
Copy link
Contributor

Rolling back because it caused real issues in Google, not clear why yet

andrewseguin added a commit to andrewseguin/components that referenced this pull request Jan 19, 2022
andrewseguin added a commit that referenced this pull request Jan 19, 2022
…ved from ListKeyManager (#14471)" (#24233)

This reverts commit e61c2fa.

(cherry picked from commit 8504c77)
andrewseguin added a commit that referenced this pull request Jan 19, 2022
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDK: ListKeyManager activeItem has incorrect reference after QueryList changes
5 participants