On chrome mobile after 2 animations with many leaves then react-flip-move gets stuck in state where many nodes hang about that should have been removed. #120
Comments
|
I downgraded the |
|
I've tried some other versions: The bug manifests in So I assume it was broken in |
|
Hi @ohjames, Right. Interrupts have always been a little tricky, especially when an item that was in the middle of exiting is re-entered. I can't promise I'll get to fixing it soon, I have a lot of other things vying for my attention. For now, I'd keep using |
|
Thanks for the info! I went ahead and wrote my own react component that is very similar to react-flip-move but uses the "Web Animations" APIs. I wouldn't have been able to do it without your great article and the links you provided on the technique. Thanks! It's easier to cancel animations and react to events with that API, which made working around this problem much easier. It also makes scheduling easier, the webanimations are designed to start on the next frame so no |
|
Awesome, glad to hear it :) FlipMove's initial version was written with Web Animations API, but yeah, performance was noticeably worse, at least in the tests I did. I also didn't want to bloat the library with polyfills. You're right, though; initially this library didn't have enter/leave animations, so the tradeoff wasn't so significant. At this point, it may be worth the tradeoff, since it would greatly simplify the code. Anyway, glad to hear you found a solution that works :) is the code for it open-source? Would be curious to see how you dealt with cancellations. |
|
I dumped the code I'm using here for now: https://github.com/ohjames/react-flip-webanimate |
|
Using 2.8.0 I replicated the issue on desktop chrome. Elements that should have been removed get stuck in the DOM and start animating again each time another item is removed. |
|
@ConneXNL cool, thanks for the heads-up. I'm pretty busy with work but I'll try and find some time to investigate. Is it specifically when adding/removing items quickly (eg. interrupting a leave animation)? |
|
@joshwcomeau I just cloned the repo and playing with it although i never looked at the internals of FlipMove. But what I found is that when quickly adding multiple children only the last "transitionend" (https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L340) callback is fired. The other "transitionend" listeners don't seem to be triggered (they do remain on the dom elements) because the transition/styles instantly update to their final position when a new item is added. As a result https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L343-L348 is only called once (for the last animation). this.remainingAnimations will never become 0 again and thus I don't know what part is unintentional, but I can imagine that when adding items quickly you want them all to be animated instead of having the non-last items to be instantly flip to their end style. |
|
I found something similar, the |
|
@ohjames I am not 100% sure if it fires unreliably on a browser level. In my last comment I explained why this is happening. The transition never ends when FlipMove re-renders with new items as the transition doesn't actually finish. |
|
@ConneXNL and @ohjames thanks for digging into it. @ConneXNL I'm not certain I follow; When adding items quickly, the A future version of this project might indeed use Web Animations. The browser support at the moment is awful though, and the polyfill looks pretty massive. In the meantime, a dirty fix could be to use a setTimeout to clear, instead of relying on |
|
Accepting that PR is probably the pragmatic thing to do for the short term. |
|
@joshwcomeau did I make clear that by adding multiple items I meant adding them 1 by 1 instead of multiple at once. So I trigger multiple renders of the flipmove component. |
|
I am using this story to test the problem: Just click the add button quickly a couple times. Now click the remove button. As you see the links get out of sync. The problem is: #120 (comment) A naughty fix to at least let the FlipMove continue normally is to add:
at https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L106 I haven't gone over most of the other code of FlipMove, were quick re-renders of taken into account/tested? |
|
@ConneXNL yeah, I understood. I'm still confused about the 'why', but I understand the 'what' :) Thanks for the reproduction. I'm super busy this week but I'll dedicate some time this weekend to debugging, and if I can't come up with a solution I'll use the setTimeout solution for now. |
|
I edited my last comment with a potential temporary fix. |
|
Hm, so the problem with that fix is that I don't think it would fire off any of the user-supplied callbacks. And no, in my own projects I've never needed to handle quick adding/removing. It's also a massive pain to write tests for this module, so the basic stuff is tested but none of the edge-cases. |
|
@joshwcomeau thanks for your replies. I fiddled some more into the problem and found the cause of the problems. My example from above: https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L106 shows how items stay in the DOM because of However, it appears that when using an the default transition (elevator) my example does not break! All the And this is where it gets troublesome. It seems to be related to whether the item is "moving on the screen" whether the Only when we are using Elevator (fade, accordionHorizontal, accordionVertical all break) the items that are being added 1 by 1 all continue their move but all jump to a opacity(1) / scale(1)-size-wise. The reason why we see the items move with the 'elevator' animation is because of the scale transition (atleast thats the case in my example). The scale animation apparently somewhat continues (although bugged) and at least fires the This means that the CSS Transitions (at least in my chrome version) are falsely animating the transition when the component is being re-rendered. On top of that, the To be 100% sure of my theory I tweaked the animations with a little hack and gave them all a The result? All the The question is where to go from here. I think my hack could atleast solve FlipMove from breaking completely when adding/removing items quickly but it's still sad that the transitions aren't fully visible when the component gets re-rendered. I wonder what exactly FlipMove/React is doing with the DOM whenever a render is called. To me it seems like the transitions shouldn't break if the DOM of the transition item isn't touched at all. |
|
Huh! That's super weird. Thanks for digging into this. I'd be surprised if this was a React issue, it sounds like a Chrome issue. I'm gonna check to see if others have reported this issue, see if I can create a minimal recreation without react in a JSBin. As a quick fix, I'll likely update the presets to do this weird hack, and throw a warning in the docs. |
|
Did some more browser testing. FireFox works completely, it even continues the actual transitions of the opacity/scaling etc without hacks. Edge has the same problem as Chrome, but it doesn't want to be tricked. The There might be other ways to trick Edge though... |
|
Ok, digged some more to see why we are even touching entering/leaving transitions because if we don't touch them they should just animate/call their Commenting the following lines seems to let FlipMove for me animate, and trigger its events in Chrome, Firefox and Edge without any problem without any other changes/hacks: https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L229-L236 What are those lines for exactly? |
|
Right, so the issue is for shuffling, unrelated to enter/leave. If I recall correctly, let's say we have an item moving from 0px to 50px, and halfway through we trigger another shuffle. Without that, the item being shuffled will "jump" to 50px and begin transitioning from there. If you remove that line and shuffle quickly, is it smooth? Possible those lines have been made redundant by other changes. |
|
Tested in storybook, the lines are not made redundant by other changes. When I spam "Shuffle Items" on Enter/Leave Animations - composite > default (elevator preset) the shuffling shows unintended behavior when I uncomment those lines. |
|
A problem might be that whenever FlipMove is rendered items that might actually still be "entering" (that don't have "entering" set to false yet) have their entering value removed on the next render. Whereas they are then picked up by https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L229-L236 . I find this all very hard to wrap my head around but I added the concept of 'entered' which is achieved when the animation hook is called and for the initial items. This so far doesn't seem to break the shuffling, and makes the transitions for adding items 1 by 1 work again. Sadly tabs screwed up the commit but you can take a look at this solution and whether it makes sense. |
|
I take back that we need 'entered' as a prop. I think we need to get rid of 'leaving' and 'entering' as currently it seems to be possible that element has both props set to true. Having multiple status props whereas an element can only have 1 active creates room for errors. I suggest we create a 'status' instead that can either have 'leaving', 'entering' or 'entered' as its value. |
|
Impossible to see what changed in that commit, you should get your editor/IDE's |
|
@ohjames I know, but you can ignore that commit for now as just adding entered doesn't cut it. I am currently in the progress in overhauling the "cild status" (aka leaving/entering) and seperating this in a seperate class that keeps track of the objects. So far it's looking good but I am still testing if I broke something else. |
|
Yeah, good call on simplifying Looking forward to seeing your solution @ConneXNL! |
|
@joshwcomeau Things were coming along nicely until I hit the following CSS Transition limitation: This basically happends when you are shuffling items and React calls appendChild() to move/readd an item. It basically cancels the ongoing transition (which looks bad) and the transitionend is never called as a result (even worse). And here is how Flexbox could potentially work around the issue http://jsfiddle.net/connexnl/h783k0gv/ when the items would keep the same order in the DOM but have their order css property updated. I know this is kinda hacky :P Another less hacky solutions would be too delay all shuffling until the very end (the moment the hook is called currently). All in all pretty hard to implement. |
|
@ConneXNL riiight. Yeah. I wanna avoid that flexbox solution, as yeah it's pretty hacky. Especially since the module currently works for most usecases without the hacks. The timeout solution might be the way to go, then. Rather than relying on The tricky thing is if the I'd also want to check what the cost of having several simultaneous |
|
@joshwcomeau I will give this thing a couple more hours. So far it still looks like transitionend is only not called when an item is re-added and the animation breaks anyway (so not that buggy as far as transitionend goes). I almost have something fully working (i hope) where I catch this on https://github.com/joshwcomeau/react-flip-move/blob/master/src/FlipMove.js#L229-L236 . The transition would break anyway so we might aswell remove the transitionend handler here and call it manually. I will get back to you on this with all the changes I made before the weekend. Edit: I am now experimenting with getComputedStyle() with some success in order to re-start an animation if items were moved/shuffled during entering. |
|
After 3 days non stop: jrmyio@5a208e0 I am not proud of it. There are so many edge cases to cover when you want FlipMove to work with high frequency changes. This has resulted in (too) many changes. The bottom line:
In any case, a single child can only have a single status. Here is a list of all the status types:
I am aware the code got a lot messier but as it turned out a lot has to change in order for this library to work under stress. It will up to the maintainers whether they want to choose and pick certain parts of my re-work or go with it all the way. |
|
@ConneXNL If you can handle all the cases my WebAnimation version of react-flip-move covers with transitionEnd then I'm super impressed. I spent about 20 hours trying to get it to work, constantly finding some new corner case it failed on. |
|
@ohjames You can clone my repo and see if your use-cases /tests can break it. I focused for the most part on letting FlipMove not break during high frequency changes. There are still animations that can show wierd behavior when an item is (re)entering, leaving and moving in rapid speed. |
|
Seems like my last commit wans't complete. I added the missing componentWillUpdate now otherwise the 'refreshing' of entering/leaving transitions wasn't working. jrmyio@92832eb for the commit. Edit: I am now able to replicate some instances where setTimeout is needed. I think these are such edge cases that for now we can rely on setTimeout to do a cleanup. However, it did contain a bug that prevented the cleanup from doing its work, this is now fixed in jrmyio@1820a69 |
|
was there ever a resolution here? i'm running into this problem and it's resulting in users clicking a filtered option they didn't intend to since the opacity : 0 element is overlaid directly on the intended option. |
|
@scamden nope I ended up writing my own version using WebAnimations. You can try that if you want. |
|
@ohjames would love to. the repo said it's not ready for general use tho.. are you saying it may be ready? |
|
@scamden I've been using it in a production site for several months now. It only supports vertical animation for now. And the browser needs WebAnimations or a polyfil. |
|
ok thanks i'll give it a shot |
|
Yeah, I haven't been able to give this issue the attention it deserves. Sorry about that! Totally open if anyone wants to take a crack at solving it :) |
|
Given the amount of time spend documented in these comments I'm honestly
scared to, seems like a very tough issue
…On Wed, May 17, 2017 at 4:16 AM Joshua Comeau ***@***.***> wrote:
Yeah, I haven't been able to give this issue the attention it deserves.
Sorry about that!
Totally open if anyone wants to take a crack at solving it :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#120 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABE5mMjKE5thDTVXlUWvzTq8FIKUcc36ks5r6tcjgaJpZM4LVz9c>
.
|
|
I say we port this library to WebAnimations or improve my version that already uses WebAnimations. IMO this just can't be done without them, at least not with so many hacks that the code becomes readable. I don't care which people do but that's some good advice there from someone who has spent more than 30 hours on this issue across 3 projects now. Aurelia, a fairly popular framework, still suffers from ridiculous bugs when using the CSS animation library and they have many more developers working on it. |
|
Yeah, so the future is 100% WebAnimations API. It still has essentially no browser support, though, and from what I remember the polyfill is both larger than FlipMove itself, and isn't as performant. I could be mistaken, but I remember when I tried it, it was noticeably choppier than the current implementation. For an animation library, that's a big deal, lol. I imagine once the browsers implement it, the performance should be fine; it's probably the polyfill itself that has the perf issues? In the meantime, I'd encourage anyone who has this issue to give @ohjames' solution a try. Once the browser support situation changes (Chrome/Firefox/Safari), I think we should revisit this! |
|
WebAnimations are only slightly slower in Chrome. After making the library only animate nodes that are scrolled into view it performs more than adequately in Firefox/Chrome. Safari though... Safari sucks at everything. grumbles |
|
Throwing another vote in the "please support rapid addition/deletion" pile. I tried to use @ohjames above module, but it did not integrate with the existing We would love to use But we're just one use case, and if rapid addition/deletion is not beneficial for the majority of use cases, then perhaps it is better left as is until there is wider browser support. |
|
The API of my library is different but much more flexible due to the programmatic nature of WebAnimations. |
|
There is a chance it could be solved if we would store styles in state and pass it to react children instead of applying directly. In that case the styles would be preserved during rerender. But this would mean a breaking change as it would require all children to accept |
|
@Hypnosphi we can also remap children and refill styles. EDIT: Example of children cloning/mapping import React, { Children, cloneElement } from 'react'
// somewhere inside a render method...
const elementStyle = { /* ... */ }
const children = Children.map(
this.props.children,
( element ) => {
if ( ! element ) {
return element
}
return cloneElement( element, { style: elementStyle })
}
)
//... |
|
@cusspvz interesting... we'd also have to merge with any existing styles, but that's an easy-to-solve nit. Would be receptive to giving this a shot :) my schedule's crazy right now, involved in a couple other projects and very busy at work, but I think this is a problem worth fixing and storing styles in state seems to make sense to me (I'd have to think much more about it to know if there would be unintended side effects). |
|
@cusspvz yeah, that's basically what I meant |
|
Bonjour, I just started using the meteor framework (I'm using version 1.6) |
Using a recent version of mobile chrome go to kchomp.co and touch the
worldnewslink at the top followed by thenewslink as quickly as possible.From here if you click the
worldnewslink again you should see that a bunch of the nodes related tonewsremain in the DOM. I verified with remote debugging that these nodes are no longer inprops.children. From here clicking further links in the header leads to more and more HTML elements being left around. It seems thatreact-flip-movehas gotten into a state where it can no longer remove needed elements.I have been unable to replicate the issue on desktop chrome, it seems specific to mobile chrome. Using the latest react-flip-move (
2.7.3).The text was updated successfully, but these errors were encountered: