Performance issue: modals & popups

[Ploppy3]

Bug, feature request, or proposal:

Bug

What is the expected behavior?

There should be no performance hit when using popups and modals

What is the current behavior?

There is a performance hit when using popups and modals

What are the steps to reproduce?

http://plnkr.co/edit/OeUkGO1w4suNVLCQceEL

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

Angular Material has several performance issues which I think should be addressed.
The web is already slow, no need to make it any slower.

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

@angular/material: ^2.0.0-beta.10
Chrome 61

Is there anything else we should know?

There is a big performance problem related to the modals & popups, I can't figure out what it is. You can often see that animations are slightly stuttering on any device & chrome version after you open a modal/popup. The performance often goes back to it's normal state after a while and sometimes don't which is a real problem, on lower-end device this performance hit is NOT negligible at all.

This bug was probably introduced when the popup/modal system was added which is quite old.

[crisbeto]

Seems related to #6713.

[Ploppy3]

@crisbeto Ok, you now have a way to reproduce it, do you have any ETA for a fix?

[giacomocerquone]

The exact same problem here.
It's really annoying, I think I'll switch to bootstrap for modals for now

[Ploppy3]

@giacomocerquone It seems to be an angular bug related to event listeners. They said it's fixed, the fix will land on ng5, probably even sooner in ng4.4.

[Ploppy3]

@crisbeto You should really do something about the plunkers, It takes time to code them and they always break within a few days. Should I open an issue for this?

[giacomocerquone]

@Ploppy3 Wooa, this is a great news actually! Thank you very much 😄

[giacomocerquone]

@crisbeto True about the plunkers! I can tell you that it is always like that, every plunker I find online is somehow broken

[Ploppy3]

@giacomocerquone Unfortunately it did not ship in 4.4.x.

[giacomocerquone]

@Ploppy3 Yes but my point is, why this package for the modal works? https://github.com/dougludlow/ng2-bs3-modal

I mean, wouldn't it be better to implement the modal like that?

[Ploppy3]

@crisbeto Hello, I'm confused, I was told the issue was solved on the Angular side but you implemented your own fix for it. Or is it just some perf improvements?

Also I don't wanna open an issue for this and I'm pretty sure you are aware that Angular 5 is incompatible with Material, when should we expect a compatible version to be released?

[crisbeto]

There were a couple of components to the issue:

1. Angular was triggering change detection for scroll events in some cases. This was fixed in core.
2. Material was attaching a global scroll listener that it wasn't removing. That's what perf: remove persistent global scroll listener #7560 addresses.

As for Angular 5 support, it'll be available when Angular 5 stable is released.

[giacomocerquone]

Thank you very much for your work, when will be available this fix in material?

[Ploppy3]

@crisbeto Not fixed, reopen if you agree.

Tested on angular@5.0.0 with angular/material@5.0.0-rc0

[crisbeto]

That was my best guess based on known performance issues @Ploppy3, it's hard to investigate without an example of something that is considered "slow".

[Ploppy3]

@crisbeto That's why I created the plunker, so that you can all see the problem, it's here, that's for sure. I don't know how Angular/material works so I can't help much but the problem is related.

I realized something new, scrolling slowly (if you remember how the plunker worked) doesn't break performance.

BTW I tried to create a stackblitz to replace the plunker.
https://stackblitz.com/edit/angular-material2-issue-oomxar?file=polyfills.ts
But the dependencies our outdated and the compiler breaks somehow. Just so many problems. I really want this bug to be fixed, If I can help, please ask.

[Ploppy3]

@crisbeto Ok, found something even better: removing the cdk-overlay-container from dom in chrome dev tools fixes the problem, so it's in fact the overlay container causing a problem, as expected. But if by simply removing it it fixes the problem, does it mean it's caused by a listener on it?

Performance hit seen by chrome:
(2 times on left without cdk-overlay-container, 2 times on right with cdk-overlay-container)

[Ploppy3]

@crisbeto Can you please reopen? Thank you.

Edit, even more investigation:

• After a fresh app start, everything works: no cdk-overlay-container visible in DOM.
• Opening a maat-menu shows cdk-overlay-container in DOM with a cdk-overlay-0 child.
• Closing menu does not remove cdk-overlay-0 from DOM, something is wrong at this point, it should be removed.
• If scrolling at this point in time: the scroll will stop (freeze) at a not so random moment, because when it freezes, the cdk-overlay-0 container will be removed from DOM.

This is the best I can do so far to help identify the problem.

[crisbeto]

Regarding "Closing menu does not remove cdk-overlay-0 from DOM, something is wrong at this point" this works as expected, the overlay will be removed when you navigate to a new route.

[crisbeto]

First of all here's a working Plunkr for reference: http://plnkr.co/edit/QFl2PT?p=preview. Second, I really don't see a difference when comparing scrolling before opening the menu and after. Third, there a lot of other things in your code that can be performance bottlenecks:

1. You're measuring the element's height, scroll position etc. on scroll which cause a reflow. These are the purple parts of your graph. See https://gist.github.com/paulirish/5d52fb081b3570c81e3a
2. You're running all of this code in the Angular zone which will cause it to run change detection for every pixel that you've scrolled. This is where all the yellow parts of your graph come from.
3. All of the work that you're doing is going through Angular's data bindings which are fast, but still have a little bit of overhead which becomes apparent when you're hitting them while scrolling. This also means that you can't just start running them outside the Angular zone (related to point 2). You can work around some of it by setting the translateY and height directly using inline styles, but there's no getting around the update event that has to be dispatched in order to update the repeater. In general this isn't a very efficient way of doing virtual scrolling because one way or another the bottleneck will be the change detection. A better approach could be to instantiate the elements yourself using an ng-template and a ViewContainerRef.

[Ploppy3]

First, Thank you for the plunker.

cdk-overlay-0 is removed from DOM after scrolling stutters (this issue), not only after navigating to a new route.

If you wanna see the stutter: don't "swipe-hold" but "swipe and let-go" (like throwing the list away, so that it keeps scrolling without you touching it, not sure how to explain this properly), you will obviously see it, tested by friends and on many devices/browsers, so it's a fact not something that I'm imagining :'( . Something caused by the overlay stops the scrolling somehow and As I said simply removing the cdk-overlay-container from DOM fixes it.

I'm not familiar with ng-template and ViewContainerRef but I'll work on that when I have time. Thank you very much for taking time to explain what's going on.

[Ploppy3]

@crisbeto Well, I found what the causing the problem.

By mocking a cdk-overlay-container I managed to reproduce the issue.

It is related to pointer-events: none;, used to keep the ability to interact with the app through this overlay.

Though I have no idea why this would be incompatible with my code. Any idea?

[willshowell]

@Ploppy3 FWIW making the overlay-container's height 0 does seem to return to initial behavior.

I don't know about the root issue, but you might get away with overriding the css to keep its height at 0 and setting .cdk-overlay-backdrop { position: fixed }.

[Ploppy3]

@willshowell But I use the overlay, I can't just move it away or hide it.

[Ploppy3]

It works properly on Firefox, couldn't test on IE 11 because Angular 5 or Material 5rc0 is incompatible with it. It also works on Edge.

May I ask you ( @crisbeto ) why not remove the cdk-overlay-container when it's empty?
It would fix the problem! Alternatively you could also set display: none;. Both of these are solutions to the problem. Hurrah :)

[crisbeto]

Sorry for the delay, but I was away the past few days. @Ploppy3 I'm still having a hard time seeing any difference in performance between toggling the display of the overlay container. I think there should at least be some concrete evidence in order to justify making changes.

[Ploppy3]

@crisbeto No problem. I can try to explain it one more time differently but I'm running out of idea.

Try holding the middle mouse button and move the mouse up or down to scroll. The scroll will stop randomly once the cdk-overlay-container is present in DOM (once a component that requires the overlay is instantiated). Don't forget that this bug is only visible in Chrome and is caused by the pointer-events: none;. Removing the container or hiding with display: none; fixes the problem.

[Ploppy3]

Ok, I just found an handy tool to create gif, so here is the demo:

Working:

Broken:

[crisbeto]

I definitely see what you mean, but I don't think it's something that applies to all scrolling. Here's a forked Plunkr where I replaced the virtual scrolling with a regular scrollable div and I don't see the issue anymore.

[Ploppy3]

Yeah, it does not apply to all scrolling, but if it happened to me it will happen to others. I do understand that it seems like a localized issue and is not related to @angular/material especially but I don't have the knowledge on Chrome nor Angular to go more in-depth an see what is causing it, I'm not even sure if this will be fixed one day. What I do know is that my code, despite not being really great, should work properly.

I would really like it to be fixed and despite not knowing how Material works internally, I think that the easiest way to do it right now is to hide the overlay when it is unused with display: none;.

[crisbeto]

I'll check if we can do it safely without any regressions, but even then it may be a while until it makes it into a release. Meanwhile it should be pretty safe to add something like this to your CSS:

.cdk-overlay-container {
  width: auto;
  height: auto;
}

.cdk-overlay-backdrop {
  position: fixed;
}

[Ploppy3]

@crisbeto Thank you very much!

This slightly modified version did the trick:

.cdk-overlay-container {
  width: auto !important;
  height: auto !important;
}

.cdk-overlay-backdrop, .cdk-global-overlay-wrapper, .cdk-overlay-pane {
  position: fixed !important;
}

[Ploppy3]

@crisbeto By using a custom scroll event listener I managed to run most the logic outside of angular zone, only the template's update triggers an angular check.

[Ploppy3]

@crisbeto Thank you very much for your hard work 👍

[angular-automatic-lock-bot]

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.}