Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Commit

Permalink
Return nil CAAction when swapping implementation (#109)
Browse files Browse the repository at this point in the history
When swapping the implementation of `actionForKey:`, returning `NSNull`
would result in a crash because it could not respond to other messages
being passed (like `runAction:forKey:`). Instead, returning `nil` will
safely receive additional messages. This was easily reproducible by
running the unit test suite on iOS 8.1 or 8.3 simulators.

Returning `nil` matches the documented behavior of `actionForKey:` in
the CALayer headers:

> Returns the action object associated with the event named by the
> string 'event'. The default implementation searches for an action
> object in the following places:
>
> 1. if defined, call the delegate method -actionForLayer:forKey:
> 2. look in the layer's `actions' dictionary
> 3. look in any `actions' dictionaries in the `style' hierarchy
> 4. call +defaultActionForKey: on the layer's class
>
> If any of these steps results in a non-nil action object, the
> following steps are ignored. If the final result is an instance of
> NSNull, it is converted to `nil'.
>
> - (nullable id<CAAction>)actionForKey:(NSString *)event;

Also fixed a crash with CASpringAnimation on iOS 8.x because the desired
selectors aren't available.
  • Loading branch information
Robert Moore authored and ianegordon committed Jan 25, 2018
1 parent fd9710d commit 53255ab
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 30 deletions.
3 changes: 2 additions & 1 deletion src/private/CABasicAnimation+MotionAnimator.m
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ void MDMConfigureAnimation(CABasicAnimation *animation, MDMAnimationTraits * tra
// linking against the public API on iOS 9+.
#pragma clang diagnostic ignored "-Wpartial-availability"
BOOL isSpringAnimation = ([animation isKindOfClass:[CASpringAnimation class]]
&& [traits.timingCurve isKindOfClass:[MDMSpringTimingCurve class]]);
&& [traits.timingCurve isKindOfClass:[MDMSpringTimingCurve class]]
&& [animation respondsToSelector:@selector(setInitialVelocity:)]);
MDMSpringTimingCurve *springTimingCurve = (MDMSpringTimingCurve *)traits.timingCurve;
CASpringAnimation *springAnimation = (CASpringAnimation *)animation;
#pragma clang diagnostic pop
Expand Down
2 changes: 1 addition & 1 deletion src/private/MDMBlockAnimations.m
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ @interface MDMLayerDelegate: NSObject <CALayerDelegate>
// animations, we queue up the modified actions and then add them all at the end of our
// MDMAnimateImplicitly invocation.
[context addActionForLayer:layer keyPath:event];
return [NSNull null];
return nil;
}

NSArray<MDMImplicitAction *> *MDMAnimateImplicitly(void (^work)(void)) {
Expand Down
70 changes: 42 additions & 28 deletions tests/unit/InitialVelocityTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertEqual(animation.initialVelocity, 0.5,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.initialVelocity))) {
XCTAssertEqual(animation.initialVelocity, 0.5,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -62,10 +64,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertEqual(animation.initialVelocity, 0.5,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.initialVelocity))) {
XCTAssertEqual(animation.initialVelocity, 0.5,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -75,10 +79,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertGreaterThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.initialVelocity))) {
XCTAssertGreaterThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -88,10 +94,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertLessThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.initialVelocity))) {
XCTAssertLessThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -101,10 +109,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertGreaterThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.initialVelocity))) {
XCTAssertGreaterThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -114,10 +124,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertLessThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.settlingDuration))) {
XCTAssertLessThan(animation.initialVelocity, 0,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand All @@ -127,10 +139,12 @@ class InitialVelocityTests: XCTestCase {

XCTAssertEqual(addedAnimations.count, 3)
addedAnimations.flatMap { $0 as? CASpringAnimation }.forEach { animation in
XCTAssertEqual(animation.duration, animation.settlingDuration,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
if (animation.responds(to: #selector(getter: CASpringAnimation.settlingDuration))) {
XCTAssertEqual(animation.duration, animation.settlingDuration,
"from: \(animation.fromValue!), "
+ "to: \(animation.toValue!), "
+ "withVelocity: \(velocity)")
}
}
}

Expand Down

0 comments on commit 53255ab

Please sign in to comment.