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(tabs): maintain selected tab when new tabs are added or removed #9132

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

crisbeto
Copy link
Member

Maintains the selectedIndex of the current tab when a new tab is added or removed. Previously, changing the amount of tabs would shift the array, causing a different tab to be selected.

Fixes #7738.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 27, 2017
@crisbeto crisbeto force-pushed the 7738/tabs-maintain-index branch from fd1827a to 3fb1921 Compare December 27, 2017 08:09
@andrewseguin
Copy link
Contributor

Is there any concern about a race when changing selectedIndex and the set of tabs at the same time? E.g. if tab two is selected, but then add a new tab in the front and set selectedIndex to 0, will the change in tabs cause indexToSelect to be set to 2 rather than the intended 0?

Maintains the `selectedIndex` of the current tab when a new tab is added or removed. Previously, changing the amount of tabs would shift the array, causing a different tab to be selected.

Fixes angular#7738.
@crisbeto crisbeto force-pushed the 7738/tabs-maintain-index branch from 3fb1921 to 3e54a92 Compare April 27, 2018 14:40
@crisbeto
Copy link
Member Author

Sorry @andrewseguin, I just got around to rebasing this PR and saw your comment from a while ago. I think that there shouldn't be an issue with race conditions since the tab changes event will be fired after the selectedIndex has been passed in.

@andrewseguin andrewseguin added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels May 29, 2018
@ngbot
Copy link

ngbot bot commented May 29, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" 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.

@shahraship
Copy link

This fix seems to have broken existing expected functionality.

On this page: https://material.angular.io/components/tabs/examples
There is one example: Tag group with dynamically changing tabs
If you follow the example on stackblitz you'll see that "Select tab after adding" checkbox functionality doesn't work anymore.

Let me know if you want me to open a new bug for this.

devversion added a commit to devversion/material2 that referenced this pull request Jul 17, 2018
…removed

Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (angular#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from angular#9132).

In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of angular#9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example)

Fixes angular#12038
devversion added a commit to devversion/material2 that referenced this pull request Jul 17, 2018
…removed

Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (angular#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from angular#9132).

In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of angular#9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example)

Fixes angular#12038
josephperrott pushed a commit that referenced this pull request Jul 17, 2018
…removed

Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from #9132).

In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of #9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example)

Fixes #12038
devversion added a commit to devversion/material2 that referenced this pull request Jul 20, 2018
…removed

Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (angular#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from angular#9132).

In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of angular#9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example)

Fixes angular#12038
josephperrott pushed a commit that referenced this pull request Jul 20, 2018
…removed

Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from #9132).

In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of #9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example)

Fixes #12038
@garrettH3S
Copy link

garrettH3S commented Jul 30, 2018

This still doesn't seem to be working ( https://stackblitz.com/angular/rkdrmvvyyjn?file=app%2Ftab-group-dynamic-example.ts v6.4.1). The focus after open doesn't seem to function correctly.

I fix this wrapping the index setter method in a setTimeout.

addTab(selectAfterAdding: boolean) {
    this.tabs.push('New');
    if (selectAfterAdding) {
      setTimeout( () => {
        this.selected.setValue(this.tabs.length - 1);
      })
    }
  }

@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] Selection changes when tabs added/removed
7 participants