Skip to content

bug(mat-sort-header): performance & change detection issue with the whole table #19441

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

Closed
woteska opened this issue May 25, 2020 · 7 comments · Fixed by #30057
Closed

bug(mat-sort-header): performance & change detection issue with the whole table #19441

woteska opened this issue May 25, 2020 · 7 comments · Fixed by #30057
Labels
area: material/sort P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance

Comments

@woteska
Copy link

woteska commented May 25, 2020

Reproduction

With Stackblitz I can't reproduce it as dev tools is not working there.

https://ibb.co/XXjZw1r

Steps to reproduce:

  1. Go to official Material page (https://material.angular.io/components/table/overview#sorting)
  2. Press F12
  3. Enable Paint flashing tool
  4. See that if you hover mat-sort-header (not clicking it, just hover), the whole table will be rendered again and again for nothing (even if you leave the sort header).

Expected Behavior

If I have a complex page with Material table, I would not expect performance issues because of re-rendering the whole table on hovering mat-sort-header. Table's data definetely should not connect to mat-sort-header on hover event by default.

Actual Behavior

Causes performance issues.
https://ibb.co/XXjZw1r

Environment

  • Angular: 9 latest
  • CDK/Material: 9 latest
  • Browser(s): Chrome
  • Operating System (e.g. Windows, macOS, Ubuntu): Windows
@woteska woteska added the needs triage This issue needs to be triaged by the team label May 25, 2020
@woteska woteska changed the title bug([mat-header]): [TITLE] bug(mat-sort-header): performance & change detection issue May 25, 2020
@woteska woteska changed the title bug(mat-sort-header): performance & change detection issue bug(mat-sort-header): performance & change detection issue with the whole table May 25, 2020
@andrewseguin
Copy link
Contributor

Here is the stackblitz example as a separate site: https://oemlkyybkmp.angular.stackblitz.io/

If I enable paint flashing, I do not see the entire table flashing green when hovering over the sort headers. Can you confirm that you see the same thing?

@andrewseguin andrewseguin added needs: clarification The issue does not contain enough information for the team to determine if it is a real bug and removed needs triage This issue needs to be triaged by the team labels May 26, 2020
@woteska
Copy link
Author

woteska commented May 30, 2020

Here is the stackblitz example as a separate site: https://oemlkyybkmp.angular.stackblitz.io/

If I enable paint flashing, I do not see the entire table flashing green when hovering over the sort headers. Can you confirm that you see the same thing?

Unfortunately I can't check it, because it does not load in... "Starting dev server"

@wesselvdv
Copy link

Isn't this the same as the mat-tooltip issue, where it was ultimately the cdk-overlay that's being re-drawn but because it covers the whole table it appears as if it's the table itself?

@wagnermaciel wagnermaciel added needs triage This issue needs to be triaged by the team and removed needs: clarification The issue does not contain enough information for the team to determine if it is a real bug labels Aug 13, 2020
@crisbeto
Copy link
Member

I don't see any paint flashing issues either and as mentioned above, the multiple change detections are because we manage the arrow animations inside the NgZone which fires off several events. We should look into doing something similar to #19432 for the sort header, but I suspect that it'll be more difficult since the animation is more complicated.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance and removed needs triage This issue needs to be triaged by the team labels Aug 16, 2020
@AshotAleqs
Copy link

here is the reproductio nexample of this bug:
https://stackblitz.com/edit/stackoverflow-table-example?file=src%2Fapp%2Ftable-basic-example.html
I have opened the issue I didn't saw this one.

#21667

@MeMeMax
Copy link

MeMeMax commented Sep 6, 2022

Just experienced the same and saw this github issue. This also breaks animations of e.g. charts which destroys the look and feel of my page. A solution is really much appreciated. Ofcourse I know you guys have a lot to do.

crisbeto added a commit to crisbeto/material2 that referenced this issue Nov 21, 2024
For a long time the sort header's animation was set up by rendering out 4 `div` elements and then arranging them to look like an arrow. This is somewhat complicated to maintain, difficult to customize, in some cases it leads to weird visual bugs and ends up triggering excessive change detections. On top of that, because it depends on `@angular/animations`, it is prone to memory leaks (see angular/angular#54149).

These changes aim to simplify the component and make it more robust by using an `svg` icon and dealing with the animations.

Fixes angular#9758.
Fixes angular#9844.
Fixes angular#10088.
Fixes angular#15451.
Fixes angular#19441.
crisbeto added a commit to crisbeto/material2 that referenced this issue Nov 21, 2024
For a long time the sort header's animation was set up by rendering out 4 `div` elements and then arranging them to look like an arrow. This is somewhat complicated to maintain, difficult to customize, in some cases it leads to weird visual bugs and ends up triggering excessive change detections. On top of that, because it depends on `@angular/animations`, it is prone to memory leaks (see angular/angular#54149).

These changes aim to simplify the component and make it more robust by using an `svg` icon and dealing with the animations.

Fixes angular#9758.
Fixes angular#9844.
Fixes angular#10088.
Fixes angular#15451.
Fixes angular#19441.
Fixes angular#10242.
crisbeto added a commit that referenced this issue Nov 27, 2024
For a long time the sort header's animation was set up by rendering out 4 `div` elements and then arranging them to look like an arrow. This is somewhat complicated to maintain, difficult to customize, in some cases it leads to weird visual bugs and ends up triggering excessive change detections. On top of that, because it depends on `@angular/animations`, it is prone to memory leaks (see angular/angular#54149).

These changes aim to simplify the component and make it more robust by using an `svg` icon and dealing with the animations.

Fixes #9758.
Fixes #9844.
Fixes #10088.
Fixes #15451.
Fixes #19441.
Fixes #10242.

(cherry picked from commit a08eeeb)
@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 Dec 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: material/sort P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants