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

Animation on page transition on iOS is broken #17649

Closed
aces-tm opened this issue Mar 1, 2019 · 83 comments
Closed

Animation on page transition on iOS is broken #17649

aces-tm opened this issue Mar 1, 2019 · 83 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@aces-tm
Copy link

aces-tm commented Mar 1, 2019

Bug Report

Ionic version:

[x] 4.x

Current behavior:
Page transition is breaking on iOS. Android is working good.

ezgif com-video-to-gif

Expected behavior:
Transition smooth onto next page.

Steps to reproduce:

Related code:

Typically I'd use this.navCtrl.navigateForward(['route', id]); to navigate to the next page, but 90% of time the animation would stop.

Other information:

Ionic info:

Ionic:

   ionic (Ionic CLI)             : 4.10.3 (/Users/andjelicnikola/.nvm/versions/node/v10.7.0/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.0.2
   @angular-devkit/build-angular : 0.11.4
   @angular-devkit/schematics    : 7.2.2
   @angular/cli                  : 7.2.2
   @ionic/angular-toolkit        : 1.2.1

Cordova:

   cordova (Cordova CLI) : 7.1.0
   Cordova Platforms     : android 7.1.0, ios 4.4.0
   Cordova Plugins       : cordova-plugin-ionic-keyboard 2.1.3, cordova-plugin-ionic-webview 3.1.2, (and 23 other plugins)

System:

   ios-deploy : 2.0.0
   NodeJS     : v10.7.0 (/Users/andjelicnikola/.nvm/versions/node/v10.7.0/bin/node)
   npm        : 6.8.0
   OS         : macOS Mojave
   Xcode      : Xcode 10.1 Build version 10B61
@ionitron-bot ionitron-bot bot added the triage label Mar 1, 2019
@jayordway
Copy link

There may be an open PR for this already:
#17224

@liamdebeasi
Copy link
Contributor

Hi there,

Thanks for the issue! Would you be able to provide a repository with the code required to reproduce this issue? Additionally, which devices/iOS versions have you been testing on?

Thanks!

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 7, 2019
@ionitron-bot ionitron-bot bot removed the triage label Mar 7, 2019
@aces-tm
Copy link
Author

aces-tm commented Mar 7, 2019

It seems like upgrade to Ionic 4.1.0 solved the issue.

If it means anything I tested on iPhone 8 Plus iOS 12.1.4

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 7, 2019
@rossholdway
Copy link
Contributor

rossholdway commented Mar 7, 2019

I'm still seeing this on 4.1.0. You can sometimes see the issue if you limit the CPU power and navigate into a page using Chrome DevTools. Finding it hard to consistently reproduce the issue though.

@aces-tm
Copy link
Author

aces-tm commented Mar 7, 2019

I am willing to help but I'd have a problem reproducing error now that my code works. Maybe if @rossholdway shares code and I can see if there is a difference to my code.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 8, 2019
@ionitron-bot ionitron-bot bot removed the triage label Mar 8, 2019
@aces-tm
Copy link
Author

aces-tm commented Mar 8, 2019

I just wanted to confirm that it started to happen again... not sure why it was working good for 1 day. I will try to create repository that reproduces the problem.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 8, 2019
@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 8, 2019
@ionitron-bot ionitron-bot bot removed the triage label Mar 8, 2019
@aces-tm
Copy link
Author

aces-tm commented Mar 8, 2019

First I added RouterModule.forRoot(routes, { preloadingStrategy: PreloadAllModules }) to app-routing.module.ts.

When loading pages I usually protect components from showing up before data is loaded with variable isLoading: number and I place the variable inside component like this: *ngIf="!isLoading".

When I removed *ngIf statement or replaced it with [hidden] it stopped flickering.

It'd be great if @rossholdway or somebody else to test this. I am not able to reproduce it on a fresh app yet, but I did not try to make API call. I tried to simulate loading with setTimeout which is not the same.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 8, 2019
@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 11, 2019
@ionitron-bot ionitron-bot bot removed the triage label Mar 11, 2019
@jayordway
Copy link

jayordway commented Mar 19, 2019

@liamdebeasi I can substitute ion-buttons to be just a div and the problem persists. I would like to think that any element within the toolbar should transition with the rest of the elements in that toolbar. It looks peculiar to see a header and the rest of the ion-content do a slide transition while the buttons appear with no slide and sit statically while the transitions occurs.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 19, 2019
@aces-tm
Copy link
Author

aces-tm commented Mar 19, 2019

@liamdebeasi @jayordway It's not a problem just with toolbar. when I put <my-custom-component *ngIf="expression"></my-custom-component> anywhere inside ion-content the transition breaks.

@jayordway
Copy link

@aces-tm Are you testing with the new development branch of ionic though?

@aces-tm
Copy link
Author

aces-tm commented Mar 20, 2019

@jayordway no, should I update to @latest or something else?

@jayordway
Copy link

Ah, I was looking at the proposed PR for this and read this note:
#17224 (comment)

@liamdebeasi
Copy link
Contributor

Hi everyone,

We have shipped some updates to the page transitions on iOS. Please try updating to Ionic 4.1.2 (npm i @ionic/core@latest @ionic/angular@latest), and let me know if you are still running into any issues.

Thanks!

@aces-tm
Copy link
Author

aces-tm commented Oct 11, 2019

ionViewDidEnter works very good and for me, there is no need to look at anything else.

@chriswep
Copy link

@liamdebeasi i'm not really convinced that this is the real issue here. this was working before in the same app without any issue (can't tell for sure now when it started, it definitely still worked with Ionic 3). also it doesn't look like only being about performance - it looks more like the animation is interrupted at some point, like if some events or animations interfere with each other. additionally the issue doesn't go away on a super fast dev machine.

using *ngIf on a transitioning page should work and did work. it's not just about preloading data but about how we setup angular templates and components. delaying observables, async-pipes / ngIf until a page has finished entering (or even being forced to not use those basic things anymore if you want a fast UI) can't really be the solution for Ionic going forward.

@iherger
Copy link

iherger commented Oct 11, 2019

@liamdebeasi , thank you for the detailed response.

I understand the general concept, but I still think there might be an issue here. In my case, I am e.g. switching from a list of clients to the detailed view of a client. Client data (about 20 datapoints, so nothing major) is fetched in a GraphQL query. However, as soon as the data is received, the animation interrupts (if I using observables and the async pipe). The client detail template is not large, and there is exactly one *ngIf which subscribes to the observable.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 11, 2019

@chriswep So for some context as to what's happening under the hood -- We are using CSS Animations/Web Animations (depending on browser support) to create these navigation animations. We then pass that work off to the browser to run/render. We do not run these animations in a requestAnimationFrame loop like some animation libraries do, so there's really not a lot of "stuff" that Ionic is doing apart from creating the animation and telling the browser to play it.

Additionally, the iOS navigation transition animates the transform and opacity properties which are properties that can be handled on separate threads (usually called "compositor threads" in Chrome). I'm not sure what else we could do to optimize these animations as we are already passing off most of the work to the browser.

I agree that there's a lot more than just using *ngIf/a hidden class. My examples were a subset of different optimizations you can make (I don't remember them all off the top of my head 🙂).

If you can provide a working example of this running well in an Ionic 3 app I'm more than happy to take a look.

edit: I also wanted to clarify that I didn't intend to suggest you never use things like *ngIf, I just meant that there's a time and a place for everything. For example, if you don't need an element on the page it's a great idea to use *ngIf to remove it from the DOM; however, the act of removing an element from the DOM is an expensive operation by itself so it's important to be mindful of what you're doing as well as how often you're doing it.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 11, 2019

@iherger Can you provide a Timeline profile from your app? It's hard to say what's going on without seeing the data itself. In Safari Dev Tools, click the "Timelines" tab and click the red circle in the top left to start recording. When done, click the square button in the top left. Then, click the "Export" button and attach the file to this thread.

I'm mostly curious to see if there are any additional optimizations we could make in Ionic Framework apart from the animations.

Of course a working reproduction would be ideal, but it sounds like there's a server side component here so I'm not sure if that would be possible from your end.

@iherger
Copy link

iherger commented Oct 11, 2019

@liamdebeasi thanks for looking into this.

Attached is a timeline. There was noticeable interruption in the transition.

localhost-recording.json.zip

@liamdebeasi
Copy link
Contributor

@iherger Thanks for the timeline data! I’ve identified two points during the transition that likely cause your lag:

image

The call trees for “Click Event Dispatched” and “Load Event Dispatched” both take quite a long time. For context, for a 60fps animation, each frame should take ~16.7ms, and the entire operations for these are taking 86.11ms and 29.02ms respectively. As a result, these tasks are being spread out across multiple frames since they cannot be completed in 16.7ms. This is where your animation lag is going to come into play.

You can also visualize this in terms of frames:

image

Notice that there are 3 times that frames drop below 30fps. Presumably, these are the 3 events you see in the first image.

For context, the 2nd event “Full Garbage Collection” is just cleanup and appears to happen after the animation has completed, so I’m not too concerned about that (I expect there needs to be some cleanup once navigation has completed). I am also assuming that the network requests (the light blue parts on the timeline) are your GraphQL results.

If you right click the first event and click “Expand All” you will notice quite a bit of processing is happening on the Angular side. I won’t post the screenshot here since the call tree is quite large, but you should notice calls from Angular zone, Angular Core, Angular Router, utility functions, and others. I did not see any Ionic-specific tasks taking up more than ~1ms of processing time. Ionic Angular is typically the fesm5.js file you will see in there. I am seeing similar results with the second “Load Event Dispatched” event.

You can also see that after the first event (which is the biggest purple block on the timeline) there are several Paint, Composite, and Style recalculations going on (likely due to Angular change detection trying to rerender components).

It’s hard to make definitive conclusions without seeing the actual code and running the app, but as of now nothing from the Ionic side sticks out as contributing significantly to the lag you are experiencing. Hope this clears things up!

@chriswep
Copy link

chriswep commented Oct 14, 2019

@liamdebeasi thank you for all the background information. i'm also sure that Ionic does a great job at optimising everything for good performance. i was just suggesting the possibility that this is not related to performance or (missing) optimisation but that this is (simply) a bug. of course i might be wrong here. however since i feel i can't simply settle on this being the status quo now, i took the effort to create two blank Ionic projects, one with Ionic 4 and one with version 3. I added a second page "Page2" and a button on the home page that navigates there. "Page2" has this in the controller:

  ngOnInit() {
    setTimeout(() => {
      this.showDiv = true;
    },50)
  }

and this in the template:

<ion-content>
  <div *ngIf="showDiv">Delayed content</div>
</ion-content>

The Ionic 4 version clearly shows the issue (tested on Safari Reponsive Design mode on a fast dev machine, as well as iOS Sim), the Ionic 3 version works just fine.

@liamdebeasi
Copy link
Contributor

Oh yeah, there certainly could be a bug! Are you able to share those 2 projects with me?

@chriswep
Copy link

Oh yeah, there certainly could be a bug! Are you able to share those 2 projects with me?

IosPageTransitionBug_Ionic3vs4.zip

@liamdebeasi
Copy link
Contributor

Thanks for the follow up. I am able to reproduce the lag in the v4 version of the app in Safari.

The lag appears to come from NgZone running in the timeout. The v3 app uses Angular 5.x, but the v4 app uses Angular 8.x so it's hard to say what is different in Angular/NgZone that is causing this change.

To demonstrate that this lag is related to Angular/NgZone, you can try the following:

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

...

constructor(private zone: NgZone) {}

...

ngOnInit() {
  this.zone.runOutsideAngular(() => {
    setTimeout(() => {
      this.showDiv = true;
    }, 50);
  });
}

By running outside of NgZone the lag should disappear entirely. I'm still digging through all of the NgZone traces when running a Timeline recording on Safari, so I'll post any additional information here when I have it.

@chriswep
Copy link

By running outside of NgZone the lag should disappear entirely.

That is probably because then the *ngIf is not triggered which is what effectively causes the ionic animation to be interrupted. Also, if you leave the controller code as is and just replace *ngIf with [hidden], all is fine.

@liamdebeasi
Copy link
Contributor

Right, so ngIf will add/remove the div from the DOM whereas the hidden directive will just add "display: none" to the div. The DOM removal/addition is typically more expensive than just changing the display.

@liamdebeasi
Copy link
Contributor

The one thing that’s interesting is I can't reproduce this in Chrome (or any other browser) on the same machine. I wonder if we're hitting some edge case in Safari? Or maybe since Chrome supports css containment it's not as much of an issue?

@rossholdway
Copy link
Contributor

(Just to add to this, I have been able to repo this in Chrome in my own app by dropping the CPU performance to its slowest in devtools)

@liamdebeasi
Copy link
Contributor

Ah interesting. I have CPU set to 6x slowdown in dev tools and the performance still seems pretty good. I'll keep digging around.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 15, 2019

So I wanted to try one thing. Can you do the following and let me know if the performance issue goes away or persists in Safari?

  1. Install this dev build: npm i @ionic/angular@4.12.0-dev.201910150008.2f88237
  2. Add the following CSS to src/global.scss:
ion-content {
  position: absolute;
  top: 0;
  bottom: 0;
  left: 0;
  right: 0;
  .inner-scroll {
    padding-top: 44px;
    background: white;
    height: 100%;
  }
}
  1. Try the test again using the v4 app @chriswep provided.

You app content will look a little funky, but I'm mostly interested in seeing if the performance issue went away. If it does, I have a theory as to why.

@chriswep
Copy link

but I'm mostly interested in seeing if the performance issue went away

the transition runs without issues with this (at least on Safari)

@rossholdway
Copy link
Contributor

Agreed. Tested this in Safari on iOS with the example @chriswep provided. The transition runs without issue after installing the dev build (and adding the CSS). 🎉 What's your theory / fix @liamdebeasi

@liamdebeasi
Copy link
Contributor

My current guess is the performance issue you were seeing is actually related to this bug in WebKit: https://bugs.webkit.org/show_bug.cgi?id=201048

Animations in the Shadow DOM tend to freeze/be janky when the layout is invalidated. In this case Node.insertBefore causes the issue. Unfortunately, Angular can sometimes call that when evaluating an ngIf, so the issue is easier to dig up in Angular apps.

I am discussing with the team as to whether there may be an appropriate workaround we could go with for now.

@mvergarair
Copy link

Any update Liam? Thanks!

@pvroom
Copy link

pvroom commented Nov 4, 2019

Liam, as developers, we can't move forward with iOS builds until this issue gets resolved. There are a few bandaids available but the deterioration in the performance of the app makes it almost unusable for clients. This issue has been hanging out there for 9 months now, when can we give birth to a permanent and complete solution?

@liamdebeasi
Copy link
Contributor

Hi @pvroom,

Unfortunately this is not an issue with Ionic, but rather an issue in WebKit/Safari. We have reached out to some contacts on the WebKit team to see if they can take a look at the issue.

The best thing I can recommend is to hold off on updating the view until the animation ends. I realize this is not an ideal workaround, but there is not much we are able to do from the Ionic Framework-side as of now. I will update this thread when I have more info to share. Thanks!

@pvroom
Copy link

pvroom commented Nov 6, 2019

Hi Liam,
Thx for getting back on this. You say that this is not an Ionic issue but Ionic is the one responsible for changing their Ionic 4 platform to a PWA-based approach heavily reliant upon Shadow Dom / Webkit, etc. We don't have any of these issues with Ionic 3 builds. So I hope you can appreciate my frustration when you say "this is not an issue with Ionic." Over the last year, Ionic has largely ceased ongoing support for Ionic 2/3, which was working well for our mobile apps and Ionic is now concentrating on a PWA approach with Ionic 4. So we were pretty much forced to move to the use of Ionic 4, only to discover that Ionic 4 brings with it serious performance issues like this. It doesn't matter to us that it's a webkit issue, what matters to us is that Ionic has put us in this position by relying on external factors that are apparently beyond your control. After 9 months of waiting for a fix, we are getting pretty discouraged.

@liamdebeasi
Copy link
Contributor

We appreciate the feedback! We moved to the Shadow DOM because of the performance and encapsulation improvements it brings. This move also allowed us to dynamically theme your app at runtime without having multiple CSS bundles. Having multiple bundles slowed down build times, startup times, etc. You can learn more about the benefits of the Shadow DOM here: https://developers.google.com/web/fundamentals/web-components/shadowdom.

I understand that this is frustrating, and we are eager to find a good resolution to this issue. Additionally, I have provided a workaround above that should allow you to avoid this performance issue entirely.

I am going to lock this thread for now. I think the current state of the issue is pretty clear, and I don't want to add any additional unwanted emails to the inboxes of other users who have interacted with this thread. Thanks!

@ionic-team ionic-team locked and limited conversation to collaborators Nov 6, 2019
@liamdebeasi
Copy link
Contributor

Hi everyone,

I wanted to provide an update regarding this issue. I am closing this thread as the main issue has been resolved in WebKit (https://bugs.webkit.org/show_bug.cgi?id=201048).

The Ionic Team does not know exactly when this fix will be available as that is not under our control. For any other bugs, please open a new issue.

Thanks!

@liamdebeasi
Copy link
Contributor

Quick update: The WebKit issue I linked to has been fixed and released as part of the latest Safari Technology Preview (release 101 at the time of writing). As it turns out, this bug was not solely related to the Shadow DOM as the WebKit team was able to reproduce this issue outside of the Shadow DOM.

We do not have a timeline as to when the fix will become available for all users as that release schedule is controlled by Apple.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests