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

Dialog does not support multiple tabs #2151

Closed
starksds opened this issue Dec 9, 2016 · 37 comments · Fixed by #2411
Closed

Dialog does not support multiple tabs #2151

starksds opened this issue Dec 9, 2016 · 37 comments · Fixed by #2411
Assignees
Labels
P2 The issue is important to a large percentage of users, with a workaround

Comments

@starksds
Copy link

starksds commented Dec 9, 2016

Bug, feature request, or proposal:

Bug: Multiple tabs in a dialog cause chrome to go aw snap

What is the expected behavior?

I should be able to have more than one tab in a dialog

What is the current behavior?

You can render one tab in a dialog but as soon as you add two chrome goes aw snap

What are the steps to reproduce?

Create a dialog. Have the contents of the dialog be tabs. Add one tab, everything works great. Add two or more tabs and chrome will eventually go aw snap.

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

There is no purpose for using tabs if you can only have one

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

Angular 2.2.4
Material 2.0.0-alpha.11-2

Is there anything else we should know?

No

@jelbourn
Copy link
Member

jelbourn commented Dec 9, 2016

@starksds can you share reproduction code?

@starksds
Copy link
Author

starksds commented Dec 9, 2016

@jelbourn I've already rolled back my changes. It works in alpha.9-3 if that helps you narrow down what might have changed.

@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Dec 9, 2016
@jelbourn
Copy link
Member

jelbourn commented Dec 9, 2016

@crisbeto can you investigate this?

@crisbeto
Copy link
Member

I couldn't reproduce it by sticking the tab demos into a dialog. @starksds can you post your HTML?

@starksds
Copy link
Author

@crisbeto our app has a mix of bootstrap 4 and material 2 components since your components weren't finished when we started so that might be causing the issue.

Could you provide a plunker for your test case that I could try to implement in our app?

@crisbeto
Copy link
Member

@chpasha
Copy link

chpasha commented Dec 11, 2016

I have the same issue without using dialog, I have many pages with tabs, but only two of them get stuck with 11-3 AS SOON AS I add more than one tab and all work fine with 10. Chrome tab gets stuck and consumes memory indefinitely until I kill the tab with task manager. I'll try to figure out how those two pages differ from the rest (the rest of pages was stuck as well after upgrade, which was caused by changing the selectedIndex variable, the solution was to change in setTimeout, but not for those two pages, where variable is not changed in code)

@chpasha
Copy link

chpasha commented Dec 11, 2016

I think I found a workaround and possibly a way to reproduce issue. I have a pure (without any code) component which is still stuck when having more than one tab. The only anomaly about this component is that it is in a lazily loaded module. As soon as I place the component in the entry module, everything works fine - so I thought something is not getting initialized when the component is lazily loaded. And then I placed the tab group in the div with *ngIf="isComponentLoaded" which is set to true only in ngAfterViewInit and guess what? It's working. If the variable is set in ngOnInit - the tab is stuck, there is something that is missing until component is fully initialized and after that it is safe to create the tabs, but not before that. I hope this information is enough to somehow reproduce the issue or make a guess about it's cause

@crisbeto
Copy link
Member

crisbeto commented Dec 11, 2016

I still can't reproduce it @chpasha. I'm using a lazy-loaded module, but it still works fine. Here's what the entire setup looks like:

lazy.module.ts

import { NgModule } from '@angular/core';
import { MaterialModule } from '@angular/material';

import { LazyComponent }   from './lazy.component';
import { routing } from './lazy.routing';

@NgModule({
  imports: [routing, MaterialModule.forRoot()],
  declarations: [LazyComponent]
})
export class LazyModule {}

lazy.routing.ts

import { ModuleWithProviders } from '@angular/core';
import { Routes, RouterModule } from '@angular/router';

import { LazyComponent } from './lazy.component';

const routes: Routes = [
  { path: '', component: LazyComponent }
];

export const routing: ModuleWithProviders = RouterModule.forChild(routes);

lazy.component.ts

import { Component } from '@angular/core';

@Component({
  template: `
    <md-tab-group md-dynamic-height>
      <md-tab>
        <template md-tab-label>Tab one</template>
        <!-- some lipsum goes here -->
      </md-tab>

      <md-tab>
        <template md-tab-label>Tab one</template>
        <!-- some lipsum goes here -->
      </md-tab>
    </md-tab-group>
  `
})
export class LazyComponent {}

Can you post what your HTML looks like?

@chpasha
Copy link

chpasha commented Dec 11, 2016

it is pure md-tab-group from documentation without anything, like
<md-tab-group > <md-tab label="one"> </md-tab> <md-tab label="two"> </md-tab> </md-tab-group>

what part of you example makes the module lazy?
this is lazy
{ path: "myurl", loadChildren: 'app/lazy.module#LazyModule' }

@crisbeto
Copy link
Member

It's the loadChildren parameter, instead of the usual component.

@chpasha
Copy link

chpasha commented Dec 11, 2016

you mean you do have loadChildren don't you?

@crisbeto
Copy link
Member

crisbeto commented Dec 11, 2016

Yes, I forgot to paste in that part since it's in the main module, but it looks like this:

{path: 'lazy', loadChildren: 'lazy/lazy.module#LazyModule'}

@chpasha
Copy link

chpasha commented Dec 11, 2016

then there could be something else involved, I'm using angular-cli 1.0.0-beta.21 with webpack. Fact is - it was working just fine in the previous beta. It must be something in tab group that doesn't get loaded yet when the component is rendered since like I said, ngIf hack works, so the component is working just fine if created after the module and component are loaded. I will try to reproduce the issue with smallest possible setup

@chpasha
Copy link

chpasha commented Dec 11, 2016

P. S. the other components where tab group is working have following difference: the tab-group there is not declared directly in html layout but rather defined as standalone angular component (because of some business logic). So this still looks somehow like the matter of when the component is rendered (and besides that I see the Uncaught TypeError: Cannot read property 'elementRef' of undefined error for those components with a stacktrace ending at MdTabHeader._alignInkBarToSelectedTab).

@chpasha
Copy link

chpasha commented Dec 11, 2016

Ok, I got it. So, I have a component which shows md-progress-circle while route is loading.

@Component({
  selector: 'load-route-progress',
  templateUrl: './load-route-progress.component.html',
})
export class LoadRouteProgressComponent implements OnDestroy {

  private subscription: Subscription;
  public loading: Subject<boolean> = new Subject();

  public constructor(private router: Router) {
    this.subscription = router.events.subscribe(s => {
      if (s instanceof NavigationStart) {
        this.loading.next(true);
      }
      if (s instanceof NavigationEnd) {
        this.loading.next(false);
      }
    });
  }
  ngOnDestroy(): void {
    this.subscription.unsubscribe();
  }
}

it has following template and I think the devil hides here

<div [ngSwitch]="loading | async">
    <template [ngSwitchCase]="true">
      <md-progress-circle mode="indeterminate"></md-progress-circle>
    </template>
    <template ngSwitchDefault>
        <ng-content></ng-content>
    </template>
</div>

and then we have the app.html looking like

<load-route-progress>
  <router-outlet></router-outlet>
</load-route-progress>

the part with lazy module and tabs is identical (but I don't think it has to do with lazy, I think it has to do with ngSwitch hiding router-outlet. So if you navigate to the lazy module, the page with tabs will stuck. If you remove load-route-progress component from markup, everything is fine. So the question is, is it a bug or feature? Maybe it is not allowed to remove router-outlet from dom (it is working as expecting with anything else though until beta 11-3) or is it really some bug in Material Tabs?

P. S. if you still cannot reproduce the problem, I will upload minimal project to github

@crisbeto
Copy link
Member

Thanks, this looks promising. I'll try it out a bit later.

@crisbeto
Copy link
Member

Thanks @chpasha, now it happened! I actually had to slow down the hiding of the progress since it doesn't break if it hides too fast. On one hand it seems weird to have a ngSwitch over a router outlet, however I'm not sure that's the cause. I put in your example in our demo app and the only component that breaks is the tabs. Another interesting thing is that the loading has to be slow in order for it to happen, e.g. if I slow it down 500ms it doesn't happen, however after 1000ms and 2000ms it does. For reference, here's what I changed the route subscriber to:

this.subscription = router.events.subscribe(s => {
  if (s instanceof NavigationStart) {
    this.loading.next(true);
  }
  if (s instanceof NavigationEnd) {
    setTimeout(() => this.loading.next(false), 1000);
  }
});

As a side-note, my initial hunch was that it may also be the circular progressbar causing it, but if I replace it with plain text it still happens.

@chpasha
Copy link

chpasha commented Dec 12, 2016

Great that you could reproduce the problem. I still think it has to do with the lifecycle. I replaced switch with two divs with [hidden] attribute and the problem was gone. And don't forget those experiments with ngIf when adding tabs after the main component has rendered, it also worked fine.

@crisbeto
Copy link
Member

True, but in that case any other components would break as well. I'll take a better look into this today.

@tomchovanec
Copy link

tomchovanec commented Dec 14, 2016

There must be some serious problem between versions 2.0.0-alpha.11-2 and 2.0.0-alpha.11-3.

Scenario: dialog contains 5 tabs.

In version 2.0.0-alpha.11-3

  • tabs are empty, no content at all. Event header has no labels. But tabs can be switched (still no content, only tabs without labels)

In version 2.0.0-alpha.11-2

  • everything works good, only md-select has wrong list position on a first show (on a 5th tab).

@crisbeto
Copy link
Member

For reference, I tracked this down to the @translateTab animation in the tab-body which ends up getting into an infinite loop. @matsko looked into it and apparently it has something to do with the router triggering change detection too early. To reproduce it:

  1. Download this snapshot of my fork and do an npm install.
  2. Run the test server with npm run demo-app.
  3. Open a browser and go to localhost:4200/tabs.

Note: if the infinite loop gets fixed, the tabs would probably end up looking broken. This is expected since I had to do some changes in order to narrow the issue down. We can revert those changes when we find a fix.

CC @jelbourn

@crisbeto
Copy link
Member

crisbeto commented Dec 18, 2016

The issue seems to be resolved in the newer Angular versions (I've confirmed with 2.3.1 of Angular and 3.3.1 of the router, as well as in 4.0.0-beta). @chpasha can you give it a try?

That being said, while it doesn't go into an infinite loop anymore, Angular stills throws an error. I'll check if there's a way to work around it.

@chpasha
Copy link

chpasha commented Dec 18, 2016

I'm stuck with angular 2.2.4 and angular cli beta 21 for now. the last time I tried angular 2.3+ there were some issues with angular-cli being not compatible with it, and the newest version of angular cli (besides being not compatible with angular 2.3) had also some breaking changes so that ng serve didn't work for me at all. It's not easy to be an early adopter :-D . I will have to count on that it's fixed for now, since I have a workaround and especially if you say that it's an angular issue. Thank you anyway 👍

@starksds
Copy link
Author

starksds commented Dec 19, 2016

@crisbeto @jelbourn I just upgraded to angular 2.3.1 with material alpha 11.3 and the infinite loop isn't happening anymore... but my tabs are still broken, ie, not showing any content.

@crisbeto
Copy link
Member

Does it throw an error @starksds?

@starksds
Copy link
Author

@crisbeto yeah, let me rebuild and get you a screenshot

@starksds
Copy link
Author

@crisbeto Here are two errors that I wasn't getting in alpha 9-3...

When the page loads
material1

When I open the dialog and click a tab
material2

Here is the tab markup:

<md-tab-group [(selectedIndex)]="selectedIndex" #tabGroup>
    <md-tab *ngIf="tabs > 0">
        <template md-tab-label>
            <ng-content select=".ba-tab-title-1"></ng-content>
        </template>
        <template>
            <ng-content select=".ba-tab-content-1"></ng-content>
        </template>
    </md-tab>
    <md-tab *ngIf="tabs > 1">
        <template md-tab-label>
            <ng-content select=".ba-tab-title-2"></ng-content>
        </template>
        <template>
            <ng-content select=".ba-tab-content-2"></ng-content>
        </template>
    </md-tab>
    <md-tab *ngIf="tabs > 2">
        <template md-tab-label>
            <ng-content select=".ba-tab-title-3"></ng-content>
        </template>
        <template>
            <ng-content select=".ba-tab-content-3"></ng-content>
        </template>
    </md-tab>
</md-tab-group>

@crisbeto
Copy link
Member

Alright, that's the error I'm getting as well. It's a little tricky to solve since the framework is trying to animate something that isn't in the DOM yet.

@starksds
Copy link
Author

@crisbeto Is this error coming from angular or material? Do you have a workaround or way to trap the error?

@crisbeto
Copy link
Member

The error is from Angular @starksds, however we may be able to work around it in Material. I'm still looking for a way to do it.

@starksds
Copy link
Author

@crisbeto does this still break in the new beta version of material?

@crisbeto
Copy link
Member

It does. I have a fix but I haven't had the time to write it down.

crisbeto added a commit to crisbeto/material2 that referenced this issue Dec 26, 2016
Prevents the tabs from either crashing Chrome (in Angular < 2.3) or throwing an animation error (in Angular >= 2.3). This was happening when the animations get triggered before the element is inserted into the DOM and thus doesn't have a computed `transform` value.

Fixes angular#2151.
crisbeto added a commit to crisbeto/material2 that referenced this issue Dec 26, 2016
Prevents the tabs from either crashing Chrome (in Angular < 2.3) or throwing an animation error (in Angular >= 2.3). This was happening when the animations get triggered before the element is inserted into the DOM and thus doesn't have a computed `transform` value.

Fixes angular#2151.
@matsko
Copy link
Contributor

matsko commented Dec 27, 2016

@starksds @crisbeto remember to use 1.4.1 since we fixed the exception that is thrown within animations due to the nature of tabs and <ng-content></ng-content>.

@matsko
Copy link
Contributor

matsko commented Dec 27, 2016

@starksds @crisbeto sorry guys it turns out the error isn't caught in the right way. I'll work on a fix right away.

@crisbeto
Copy link
Member

Can confirm that this is now fixed in Angular 2.4.2 via angular/angular@21030e9

mmalerba pushed a commit that referenced this issue Jan 18, 2017
* fix(tabs): crashing on chrome under certain conditions

Prevents the tabs from either crashing Chrome (in Angular < 2.3) or throwing an animation error (in Angular >= 2.3). This was happening when the animations get triggered before the element is inserted into the DOM and thus doesn't have a computed `transform` value.

Fixes #2151.

* Fix IE errors.

* Add a TODO.

* Revert.

* Add TODO for removing the workaround.
kara pushed a commit to kara/material2 that referenced this issue Jan 20, 2017
* fix(tabs): crashing on chrome under certain conditions

Prevents the tabs from either crashing Chrome (in Angular < 2.3) or throwing an animation error (in Angular >= 2.3). This was happening when the animations get triggered before the element is inserted into the DOM and thus doesn't have a computed `transform` value.

Fixes angular#2151.

* Fix IE errors.

* Add a TODO.

* Revert.

* Add TODO for removing the workaround.
@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 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants