ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.05k stars 13.51k forks source link

bug: Rendering/flickering performance issue related to ion-item and ion-button #26476

Closed tgangso closed 1 year ago

tgangso commented 1 year ago

Prerequisites

Ionic Framework Version

Current Behavior

Isolated this to version 5.4.2 and newer: ion-item's and ion-buttons cause flickering/performance issue. General slower responsiveness in the app and pages with many components cause a few milliseconds of noticable 1 time flickering when toggling on/off or for example changing an array that has an ngFor with many ion-item's and/or ion-buttons elements. Directly compared with version 5.4.1 or older where this does not happend, and it all renders smoothly.

Same behaviour with newest ionic 6.4.0 and angular 15.

Expected Behavior

Elements not flickering like 5.4.1 or older

Steps to Reproduce

Use code repository URL with ionic 5.4.2. See commit notes. Issue starts in version 5.4.2, and is still present on newest ionic v6.

Start app (npm install, npm start), start typing in the search box, for example "Samsung", and you will see the elements in the search result loop flicker. This is smooth and non-flickering on version 5.4.1 or older. Issue not related to algolia/instantsearch used in example, just used to provide a noticable example of the issue.

This is just one example where it was very noticable in my app. But it is noticeable with most ion-item and ion-button elements in head-to-head comparison that they are just slightly slower to render in general all over the app, compared to the smooth behaviour in 5.4.1.

Issue is also "resolved" by removing ion-item and ion-button in the loop when using version 5.4.2.

Code Reproduction URL

https://github.com/tgangso/ionic-test-a

Ionic Info

Ionic:

Ionic CLI : 5.4.16 () Ionic Framework : @ionic/angular 5.4.2 @angular-devkit/build-angular : 12.1.4 @angular-devkit/schematics : 12.2.18 @angular/cli : 12.1.4 @ionic/angular-toolkit : 4.0.0

Utility:

cordova-res : not installed native-run : not installed

System:

NodeJS : v16.18.0 (C:\Program Files\nodejs\node.exe) npm : 8.19.2 OS : Windows 10

Additional Information

Same issue as reported in #22798, but now with example repo.

tgangso commented 1 year ago

@liamdebeasi please check if you can reproduce this issue now.

liamdebeasi commented 1 year ago

Can you provide a reproduction using Ionic 6? We do not provide bug fixes for Ionic 5 anymore. (You can do this in a separate branch if that's easiest)

tgangso commented 1 year ago

I can, but here you can easily a-b test it. I would suggest you look into this example also. Will get you a repo with v6 also.

tgangso commented 1 year ago

@liamdebeasi Upgraded the repo with ionic v6.4.0

Same issue

liamdebeasi commented 1 year ago

Thanks for the providing the repo. I can reproduce the reported behavior, but this does not appear to be a bug in Ionic. There is no stable identity provided for each item in the loop, so Angular re-creates the elements every time the hits array is updated. In Ionic 5.4.2 we updated to Stencil 2 which uses a new task queue strategy. This made the performance issue more apparent due to the flicker.

Assigning a stable identity by using trackBy resolved the issue for me: https://ionicframework.com/docs/angular/performance

There may be instances where the flicker persists (such as when Angular creates new elements when it should). Ionic's components are lazily loaded, so this behavior should be improved when we move away from this lazy initialization in the future. This work is being tracked in https://github.com/ionic-team/ionic-framework/issues/25646.

tgangso commented 1 year ago

Thanks for the quick response.

I have now done some testing, and adding the trackBy definitely helps, and the issue in the sample seems "fixed" at first sight. But it does not remove the flicker completely, and the production application as a whole is less smooth. For example if the elements in the search changes significantly based on the filtering, it will re-create elements anyway, and you get the quick flicker of all the elements. This is completely smooth before the Stencil 2 task queue strategy. This is more noticable in my production application where there are more data in the elements. (Other places in my app where it is noticable is where for example buttons show/hide inside ngIf, they will noticable flicker when toggled to display or not)

I would argue that the underlying flickering/performance degradation caused by the Stencil 2 task queue strategy is only masked by your "solution". Not calling the issue an ionic bug is false, as the only change in the application is the ionic version, there is a real performance/rendering degradation here caused by the stencil change.

The example in the repo is ment to exaggerate and showcase the issue in a way that is visible to the eye, and by omitting the trackBy it showcases the degradation that in my opinion, you should work to improve. You could also measure the issue in milliseconds with profiling, and you will see a significant increase in the render time, but here you can see it easily in plain sight.

Is there any way to configure or change this behaviour? Is there any chance you can look into improving this?

(yes I am sensitive to flicker and things not beeing as smooth as they can be, as I want the ionic application to feel as close to native as possible)

sean-perkins commented 1 year ago

@tgangso thanks for the follow-up. There are a variety of factors in this thread, I think there is value in isolating them to help identify steps forward.

Using trackBy:

As observed, introducing trackBy improves the reproduction example. Angular without specifying a trackBy will re-create the element nodes each time the source is modified in the ngFor.

It does not remove the flicker completely, and the production application as a whole is less smooth.

Is this with or without using trackBy? What does less smooth mean? Can you isolate the criteria of your production app into a minimal reproduction so that the team can diagnose and observe the issue?

Angular recommends using trackBy for these situations. While it might incrementally improve your problem, it is not masking a solution, but contributing towards the solution. You should absolutely be using trackBy if you are iterating over a list of items in Angular.

For example if the elements in the search changes significantly based on the filtering, it will re-create elements anyway

Can you elaborate more on this? Why would the elements rendered as a result of searching, be rendered differently based on filtering? Angular should be able to reuse the template reference by associating it to the item reference in the array. Are you creating a new instance of the array?

Stencil 2 task queues

We currently are not tracking any development changes to the task queue implementation. There is one open issue in Stencil that may overlap with this: https://github.com/ionic-team/stencil/issues/1835.

As Liam mentioned, we are tracking a significant change to @ionic/angular here: https://github.com/ionic-team/ionic-framework/issues/25646. However it is unlikely to land in the v7 improvements. Moving to the custom elements build should eliminate a portion of the initialization cost of the component.

In v7 we are removing usage of a Stencil flag that initialized elements in the next frame. We could test the reproduction against a dev-build of these changes to see if the problem is incrementally improved.

Next steps

In order for the team to diagnose the issue further, we need a minimal reproduction that takes advantage of view recycling with trackBy and demonstrates the flickering/performance issues. Alternatively if this is reproducible in React/Vue with their key tracking, that would also suffice. All extra criteria (Algolia, etc.) needs to be removed/reduced. If it does not contribute to the problem, it does not belong in the reproduction.

With a narrowed reproduction both teams (Stencil & Framework) can better test changes against the problem and diagnose what else could be contributing to the problem.

We also are very passionate in building UI that feels as responsive and native as possible. We appreciate you reporting this issue and helping us try to narrow the scope so that we can keep improving.

tgangso commented 1 year ago

@tgangso thanks for the follow-up. There are a variety of factors in this thread, I think there is value in isolating them to help identify steps forward.

Using trackBy:

As observed, introducing trackBy improves the reproduction example. Angular without specifying a trackBy will re-create the element nodes each time the source is modified in the ngFor.

It does not remove the flicker completely, and the production application as a whole is less smooth.

Is this with or without using trackBy? What does less smooth mean? Can you isolate the criteria of your production app into a minimal reproduction so that the team can diagnose and observe the issue?

By less smooth, I mean there are more noticeable minor flickers when using the app, on elements where using ion-item and ion-button. Issue not present on 5.4.1. Upgrading the ionic version feels like a downgrade.

Angular recommends using trackBy for these situations. While it might incrementally improve your problem, it is not masking a solution, but contributing towards the solution. You should absolutely be using trackBy if you are iterating over a list of items in Angular.

For example if the elements in the search changes significantly based on the filtering, it will re-create elements anyway

Can you elaborate more on this? Why would the elements rendered as a result of searching, be rendered differently based on filtering? Angular should be able to reuse the template reference by associating it to the item reference in the array. Are you creating a new instance of the array?

If the new filter/search return new elements that were not in the array before it will flicker as the new object(s) is introduced. I am not sure if this flickering issue will be fixed with any of the other cases/issues you are referring to in the future, or if this will remain because of the Stencil 2 task queues. In any case, this flicker is not present in 5.4.1 and older, independent of whether trackBy is used. You can easily test this in the repo, and if compared directly to 5.4.1 you will notice the difference.

Stencil 2 task queues

We currently are not tracking any development changes to the task queue implementation. There is one open issue in Stencil that may overlap with this: ionic-team/stencil#1835.

As Liam mentioned, we are tracking a significant change to @ionic/angular here: #25646. However it is unlikely to land in the v7 improvements. Moving to the custom elements build should eliminate a portion of the initialization cost of the component.

In v7 we are removing usage of a Stencil flag that initialized elements in the next frame. We could test the reproduction against a dev-build of these changes to see if the problem is incrementally improved.

This is interesting, I would like to test the v7 and see if it is improved. How would I go about doing this?

Next steps

In order for the team to diagnose the issue further, we need a minimal reproduction that takes advantage of view recycling with trackBy and demonstrates the flickering/performance issues. Alternatively if this is reproducible in React/Vue with their key tracking, that would also suffice. All extra criteria (Algolia, etc.) needs to be removed/reduced. If it does not contribute to the problem, it does not belong in the reproduction.

With a narrowed reproduction both teams (Stencil & Framework) can better test changes against the problem and diagnose what else could be contributing to the problem.

We also are very passionate in building UI that feels as responsive and native as possible. We appreciate you reporting this issue and helping us try to narrow the scope so that we can keep improving.

I could help provide better samples, but the current one displays the issue perfectly in my opinion. The issue is not related to ngFor or algolia (it is just used to save some time), but the ion-item and ion-buttons, but keeping the elements in an ngFor, makes the issue more apparent. But even toggling on/off a few buttons with an ngIf will create more noticable flicker as compared to version 5.4.1, but will be more difficult to notice unless you do a direct comparison.

I would be interested in testing v7 before spending too much effort on this though. Hope we can improve this as it is not ideal to be stuck at 5.4.1

liamdebeasi commented 1 year ago

Apologies for the delay. Here is a dev build of Ionic 7:

npm install @ionic/angular@6.4.2-dev.11673981545.1e612253

Note that this dev build is subject to the Ionic 7 Breaking Changes.

I did a test using the sample application, and the behavior seems to be significantly improved. Can you give this a try and let me know if you can reproduce the improved behavior?

tgangso commented 1 year ago

Apologies for the delay. Here is a dev build of Ionic 7:

npm install @ionic/angular@6.4.2-dev.11673981545.1e612253

Note that this dev build is subject to the Ionic 7 Breaking Changes.

I did a test using the sample application, and the behavior seems to be significantly improved. Can you give this a try and let me know if you can reproduce the improved behavior?

I just tried this build with my production app and the issue is gone, even when I am not using trackBy. Must be the "Stencil flag that initialized elements in the next frame" I am noticing as flicker. Please make sure this change makes it to the final release.

This makes me very happy, when can we expect version 7 to release?

liamdebeasi commented 1 year ago

I just tried this build with my production app and the issue is gone, even when I am not using trackBy. Must be the "Stencil flag that initialized elements in the next frame" I am noticing as flicker. Please make sure this change makes it to the final release.

Glad to hear the issue is resolved. The work for this has been merged, so it should make the final release.

This makes me very happy, when can we expect version 7 to release?

We do not have an ETA for Ionic 7 at the moment. However, we will have a beta in the near future. We will announce it on our blog and our Twitter when this happens.

The work required to resolve this issue has been completed, so I am going to close this. Let me know if you have any questions.

ionitron-bot[bot] commented 1 year ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.