[ASDisplayNode] Certain animations cause relayout before layout transitions can complete

[jellenbogen]

To whomever reviews this, please let me know if I'm missing something here. I've been using the framework for a bit now, but there is a ton to learn here, thanks in advance.

There are certain animations that can cause a layoutSubviews or in this case layoutSubnodes to occur up the animating nodes parent chain. The 2 cases I've observed are frame and bounds.

This is causing the the node that is transitioning to snap to the finalLayout before all animations are complete and before completeTransition(didComplete: Bool) is called.

The proposed fix is to gate relayout by whether or not there is a transition occurring. I've added a check for _transitionContext != nil (which is set to nil on the transitionDidComplete callback) in the layout method on ASDisplayNode.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[appleguy]

@jellenbogen very interesting fix. I am left wondering how this is working for the use cases we have in the Pinterest app... Anyway, it is a to do item for us to make a public the basic animation controller that automatically fades in new sub nodes and fades out removed ones. It might be something about how that is set up that avoids the issue.

@nguyenhuy @levi @maicki does this change seem safe? How could we test it better?

[jellenbogen]

@appleguy Yeah I'm curious how you're usages are working as well. Are you animating either the frame or bounds properties on nodes in your layout? Here is an example of the transition where I first noticed this issue.

Layout:

override func layoutSpecThatFits(constrainedSize: ASSizeRange) -> ASLayoutSpec {
    super.layoutSpecThatFits(constrainedSize)
    banner.alignSelf = .Stretch
    banner.flexBasis = tapped ? ASRelativeDimensionMakeWithPoints(300) : ASRelativeDimensionMakeWithPoints(100)

    let finalLayout = ASStackLayoutSpec(direction: .Vertical, spacing: 0,
        justifyContent: .Start, alignItems: .Center, children: [banner])
    return finalLayout
}

And the the animation and animation initiation:

override func animateLayoutTransition(context: ASContextTransitioning!) {
    let initialFrame = context.initialFrameForNode(banner)
    print("initialFrame: \(initialFrame)")
    let finalFrame = context.finalFrameForNode(banner)
    print("finalFrame: \(finalFrame)")

    let frameAnimation = POPSpringAnimation(propertyNamed: kPOPViewFrame)
    frameAnimation.fromValue = NSValue(CGRect: initialFrame)
    frameAnimation.toValue = NSValue(CGRect: finalFrame)
    frameAnimation.completionBlock = {(animataion: POPAnimation!, complete: Bool) in
        context.completeTransition(complete)
    }
    frameAnimation.delegate = self
    banner.pop_addAnimation(frameAnimation, forKey: "frame")
}

func wasTapped() {
    tapped = !tapped
    transitionLayoutWithAnimation(true, shouldMeasureAsync: true, measurementCompletion: {})
}

Animating other properties that did not change the frame were working, for instance if I was just animating the alpha of new nodes.

And here is a sample app to demonstrate the issue. Whats happening is the animated node will jump to its final position immediately after starting the animation.

LayoutTransitionTestApp.zip

[appleguy]

@nguyenhuy I did get a crash on a Commerce pin while running with this, but it might be a separate issue (as discussed, Commerce is triggering some things that wouldn't happen to most clients). Or could this be due to the change?

[levi]

We might want to look into a flag that ignores any layout pass while the transition is in flight. That will likely fix this issue.

[jellenbogen]

Thats essentially my change here, since there is only a transitionContext if a transition is in process right?

[levi]

remove space between : [context

[levi]

@jellenbogen can you rebase this against latest master? I would like to do some testing to get this landed soon.

[maicki]

@levi @appleguy Especially after #1622 landed curious if the initial bug happens and the crash on the commerce pin.

[appleguy]

@jellenbogen are you around to do the re-base? Otherwise, it would be tremendously helpful if you could test the original bug against the current master version, as there have been many changes which may fix your problem.

This is one of the areas that has seen the most changing the framework in the last month, because Pinterest uses the system in a pretty intensive multithreaded way