Edge case where animation async event fires after loading was hidden #1100
Conversation
| ionic.DomUtil.centerElementByMargin(self.element[0]); | ||
| //One frame after it's visible, position it | ||
| ionic.requestAnimationFrame(function() { | ||
| if (self.isShown === undefined || self.isShown) |
There was a problem hiding this comment.
Can we just do if (self.isShown) { here, and move this.isShown = true; above the requestAnimationFrame?
In reality, this.isShown will always be true inside this function, since requestAnimationFrame is called a few milliseconds later than everything else.
But in our unit tests, requestAnimationFrame is called instantly, so we can move isShown up for the sake of our tests.
|
Thanks again @graemefoster - left a couple of comments. |
|
No worries - my laptop has died and I'm off on holiday tomorrow so won't be able to fix this up (sorry bout the line ending style). Feel free to close this and make the change. (I will get one of my pr's up soon!!) There is a similar race in ionic backdrop. You can call release while the retain animation is running, and have the backdrop get stuck Cheers Graeme Sent from my iPhone On 10 Apr 2014, at 9:25 pm, "Andy Joslin" <[email protected]mailto:[email protected]> wrote: Thanks again @graemefosterhttps://github.com/graemefoster - left a couple of comments. Reply to this email directly or view it on GitHubhttps://github.com//pull/1100#issuecomment-40080587. |
|
There's probably the same race in popup - I'll fix them all. Thanks and have a fun holiday, and yes we will definitely merge one of your PRs soon :-) |
|
Nice one :) cheers. Grae Sent from my iPhone On 10 Apr 2014, at 9:30 pm, "Andy Joslin" <[email protected]mailto:[email protected]> wrote: There's probably the same race in popup - I'll fix them all. Thanks and h a fun holiday, and yes we will definitely merge one of your PRs soon :-) Reply to this email directly or view it on GitHubhttps://github.com//pull/1100#issuecomment-40081304. |
|
Hi @ajoslin, Does the fix that you've committed also fix the race condition for the backdrop that Graeme mentioned above? I've been working with Graeme so I have a reliable reproduction of the race condition to test this stuff on so I can test out any changes you make. This is currently blocking me from upgrading to the latest ionic (which I badly need to do to get the latest scrolling fixes) hence why I'm keen to follow this up. Thanks |
|
Hi @robdmoore, I don't think backdrop has a race condition - it doesn't wait to add and remove the classes. Although I guess ngAnimate does have its own wait which could cause race conditions ... there is way to fix this though. Could you post a reproduce case? |
|
OK, I see the problem from @graemefoster's earlier backdrop PR (which I missed). So my fix for popup and loading race conditions probably didn't fully work either. Creating a generic fix. |
Addresses #1100. Fixes race conditions where $animate.{removeClass,hideClass} are called simultaneously, in addition fixes any blinking that coulld be caused by showing an element just attached to the dom.
|
Ok @robdmoore, I just pushed an abstraction that gets rid of any possible race conditions, and also factors out blinking fixes. Lemme know how it works once the travis build is done. |
|
Awesome! You guys are great. I'll grab the latest and let you know how I go :) |
|
Hi @ajoslin, It's almost perfect now. The race conditions are definitely gone in both the loading and the backdrop which is great! The only thing now is, if the loading it asked to show and then hide before the delay time passed into the options then what happens is after that delay it shows up briefly (whereas it shouldn't show up at all since it had been asked to be hidden). |
|
The other thing I noticed is that very occasionally the text on the loading modal is off-centre on my iPhone 4S, but it doesn't happen on Chrome. |
|
raises pitchfork die, ngAnimate! |
|
Can you test out the latest changes? Switched to regular old class changes & setTimeouts. Too many issues to deal with for ngAnimate in this case. |
|
Awesome! Great work on this mate - it seems perfect now :D |
|
Also, hilarious pitchfork image :) |
|
Awesome :-) |
Stop the race condition where the loading is hidden, but then the animation frame asynchronously kicks in.