From 236cdd799c8a8d945a52be5947b3d4d1c30a28e6 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Fri, 23 Feb 2018 09:10:09 -0800 Subject: [PATCH] Fix UIResponder handling with view backing ASDisplayNode (#789) * Add failing tests * Fix responder chain handling in Texture * Add mores tests that horrible fail * Add Changelog.md entry * Some fixes * Update logic * Add tests that prevents infinite loops if a custom view is overwriting UIResponder methods * Add macro to forward methods in ASDisplayNode * Add macro for forwarding responder methods in _ASDisplayView * Remove junk * Address first comments * Update _ASDisplayView to cache responder forwarding methods * Use XCTAssertEqual --- CHANGELOG.md | 1 + Source/ASDisplayNode.mm | 93 ++++++++++++++ Source/Details/_ASDisplayView.h | 8 ++ Source/Details/_ASDisplayView.mm | 124 +++++++++++++++++-- Source/Private/ASDisplayNode+UIViewBridge.mm | 29 ++--- Source/Private/ASDisplayNodeInternal.h | 7 ++ Tests/ASDisplayNodeTests.mm | 82 +++++++++++- Tests/ASTableViewTests.mm | 50 +++++++- 8 files changed, 368 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5716f6857..655f186ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ - Make `ASCellNode` tint color apply to table view cell accessories. [Vladyslav Chapaev](https://github.com/ShogunPhyched) [#764](https://github.com/TextureGroup/Texture/pull/764) - Fix ASTextNode2 is accessing backgroundColor off main while sizing / layout is happening. [Michael Schneider](https://github.com/maicki) [#794](https://github.com/TextureGroup/Texture/pull/778/) - Pass scrollViewWillEndDragging delegation through in ASIGListAdapterDataSource for IGListKit integration. [#796](https://github.com/TextureGroup/Texture/pull/796) +- Fix UIResponder handling with view backing ASDisplayNode. [Michael Schneider](https://github.com/maicki) [#789] (https://github.com/TextureGroup/Texture/pull/789/) ## 2.6 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 5d6f305d3..1cdedaa44 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -545,6 +545,8 @@ - (UIView *)_locked_viewToLoad // Special handling of wrapping UIKit components if (checkFlag(Synchronous)) { + [self checkResponderCompatibility]; + // UIImageView layers. More details on the flags if ([_viewClass isSubclassOfClass:[UIImageView class]]) { _flags.canClearContentsOfLayer = NO; @@ -828,6 +830,97 @@ - (void)nodeViewDidAddGestureRecognizer _flags.viewEverHadAGestureRecognizerAttached = YES; } +#pragma mark UIResponder + +#define HANDLE_NODE_RESPONDER_METHOD(__sel) \ + /* All responder methods should be called on the main thread */ \ + ASDisplayNodeAssertMainThread(); \ + if (checkFlag(Synchronous)) { \ + /* If the view is not a _ASDisplayView subclass (Synchronous) just call through to the view as we + expect it's a non _ASDisplayView subclass that will respond */ \ + return [_view __sel]; \ + } else { \ + if (ASSubclassOverridesSelector([_ASDisplayView class], _viewClass, @selector(__sel))) { \ + /* If the subclass overwrites canBecomeFirstResponder just call through + to it as we expect it will handle it */ \ + return [_view __sel]; \ + } else { \ + /* Call through to _ASDisplayView's superclass to get it handled */ \ + return [(_ASDisplayView *)_view __##__sel]; \ + } \ + } \ + +- (void)checkResponderCompatibility +{ +#if ASDISPLAYNODE_ASSERTIONS_ENABLED + // There are certain cases we cannot handle and are not supported: + // 1. If the _view class is not a subclass of _ASDisplayView + if (checkFlag(Synchronous)) { + // 2. At least one UIResponder methods are overwritten in the node subclass + NSString *message = @"Overwritting %@ and having a backing view that is not an _ASDisplayView is not supported."; + ASDisplayNodeAssert(!ASDisplayNodeSubclassOverridesSelector(self.class, @selector(canBecomeFirstResponder)), ([NSString stringWithFormat:message, @"canBecomeFirstResponder"])); + ASDisplayNodeAssert(!ASDisplayNodeSubclassOverridesSelector(self.class, @selector(becomeFirstResponder)), ([NSString stringWithFormat:message, @"becomeFirstResponder"])); + ASDisplayNodeAssert(!ASDisplayNodeSubclassOverridesSelector(self.class, @selector(canResignFirstResponder)), ([NSString stringWithFormat:message, @"canResignFirstResponder"])); + ASDisplayNodeAssert(!ASDisplayNodeSubclassOverridesSelector(self.class, @selector(resignFirstResponder)), ([NSString stringWithFormat:message, @"resignFirstResponder"])); + ASDisplayNodeAssert(!ASDisplayNodeSubclassOverridesSelector(self.class, @selector(isFirstResponder)), ([NSString stringWithFormat:message, @"isFirstResponder"])); + } +#endif +} + +- (BOOL)__canBecomeFirstResponder +{ + if (_view == nil) { + // By default we return NO if not view is created yet + return NO; + } + + HANDLE_NODE_RESPONDER_METHOD(canBecomeFirstResponder); +} + +- (BOOL)__becomeFirstResponder +{ + if (![self canBecomeFirstResponder]) { + return NO; + } + + // Note: This implicitly loads the view if it hasn't been loaded yet. + [self view]; + + HANDLE_NODE_RESPONDER_METHOD(becomeFirstResponder); +} + +- (BOOL)__canResignFirstResponder +{ + if (_view == nil) { + // By default we return YES if no view is created yet + return YES; + } + + HANDLE_NODE_RESPONDER_METHOD(canResignFirstResponder); +} + +- (BOOL)__resignFirstResponder +{ + if (![self canResignFirstResponder]) { + return NO; + } + + // Note: This implicitly loads the view if it hasn't been loaded yet. + [self view]; + + HANDLE_NODE_RESPONDER_METHOD(resignFirstResponder); +} + +- (BOOL)__isFirstResponder +{ + if (_view == nil) { + // If no view is created yet we can just return NO as it's unlikely it's the first responder + return NO; + } + + HANDLE_NODE_RESPONDER_METHOD(isFirstResponder); +} + #pragma mark - (NSString *)debugName diff --git a/Source/Details/_ASDisplayView.h b/Source/Details/_ASDisplayView.h index 578dd8d58..36f3a00d6 100644 --- a/Source/Details/_ASDisplayView.h +++ b/Source/Details/_ASDisplayView.h @@ -39,6 +39,14 @@ NS_ASSUME_NONNULL_BEGIN - (void)__forwardTouchesEnded:(NSSet *)touches withEvent:(UIEvent *)event; - (void)__forwardTouchesCancelled:(NSSet *)touches withEvent:(UIEvent *)event; +// These methods expose a way for ASDisplayNode responder methods to let the view call super responder methods +// They are called from ASDisplayNode to pass through UIResponder methods to the view +- (BOOL)__canBecomeFirstResponder; +- (BOOL)__becomeFirstResponder; +- (BOOL)__canResignFirstResponder; +- (BOOL)__resignFirstResponder; +- (BOOL)__isFirstResponder; + @end NS_ASSUME_NONNULL_END diff --git a/Source/Details/_ASDisplayView.mm b/Source/Details/_ASDisplayView.mm index aa2c43eca..2b72d1fd6 100644 --- a/Source/Details/_ASDisplayView.mm +++ b/Source/Details/_ASDisplayView.mm @@ -23,9 +23,54 @@ #import #import #import +#import #import #import +#pragma mark - _ASDisplayViewMethodOverrides + +typedef NS_OPTIONS(NSUInteger, _ASDisplayViewMethodOverrides) +{ + _ASDisplayViewMethodOverrideNone = 0, + _ASDisplayViewMethodOverrideCanBecomeFirstResponder = 1 << 0, + _ASDisplayViewMethodOverrideBecomeFirstResponder = 1 << 1, + _ASDisplayViewMethodOverrideCanResignFirstResponder = 1 << 2, + _ASDisplayViewMethodOverrideResignFirstResponder = 1 << 3, + _ASDisplayViewMethodOverrideIsFirstResponder = 1 << 4, +}; + +/** + * Returns _ASDisplayViewMethodOverrides for the given class + * + * @param c the class, required. + * + * @return _ASDisplayViewMethodOverrides. + */ +static _ASDisplayViewMethodOverrides GetASDisplayViewMethodOverrides(Class c) +{ + ASDisplayNodeCAssertNotNil(c, @"class is required"); + + _ASDisplayViewMethodOverrides overrides = _ASDisplayViewMethodOverrideNone; + if (ASSubclassOverridesSelector([_ASDisplayView class], c, @selector(canBecomeFirstResponder))) { + overrides |= _ASDisplayViewMethodOverrideCanBecomeFirstResponder; + } + if (ASSubclassOverridesSelector([_ASDisplayView class], c, @selector(becomeFirstResponder))) { + overrides |= _ASDisplayViewMethodOverrideBecomeFirstResponder; + } + if (ASSubclassOverridesSelector([_ASDisplayView class], c, @selector(canResignFirstResponder))) { + overrides |= _ASDisplayViewMethodOverrideCanResignFirstResponder; + } + if (ASSubclassOverridesSelector([_ASDisplayView class], c, @selector(resignFirstResponder))) { + overrides |= _ASDisplayViewMethodOverrideResignFirstResponder; + } + if (ASSubclassOverridesSelector([_ASDisplayView class], c, @selector(isFirstResponder))) { + overrides |= _ASDisplayViewMethodOverrideIsFirstResponder; + } + return overrides; +} + +#pragma mark - _ASDisplayView + @interface _ASDisplayView () // Keep the node alive while its view is active. If you create a view, add its layer to a layer hierarchy, then release @@ -40,6 +85,21 @@ @implementation _ASDisplayView NSArray *_accessibleElements; CGRect _lastAccessibleElementsFrame; + + _ASDisplayViewMethodOverrides _methodOverrides; +} + +#pragma mark - Class + ++ (void)initialize +{ + __unused Class initializeSelf = self; + IMP staticInitialize = imp_implementationWithBlock(^(_ASDisplayView *view) { + ASDisplayNodeAssert(view.class == initializeSelf, @"View class %@ does not have a matching _staticInitialize method; check to ensure [super initialize] is called within any custom +initialize implementations! Overridden methods will not be called unless they are also implemented by superclass %@", view.class, initializeSelf); + view->_methodOverrides = GetASDisplayViewMethodOverrides(view.class); + }); + + class_replaceMethod(self, @selector(_staticInitialize), staticInitialize, "v:@"); } + (Class)layerClass @@ -49,6 +109,26 @@ + (Class)layerClass #pragma mark - NSObject Overrides +- (instancetype)init +{ + if (!(self = [super init])) + return nil; + + [self _initializeInstance]; + + return self; +} + +- (void)_initializeInstance +{ + [self _staticInitialize]; +} + +- (void)_staticInitialize +{ + ASDisplayNodeAssert(NO, @"_staticInitialize must be overridden"); +} + // e.g. ; frame = ...> - (NSString *)description { @@ -358,15 +438,41 @@ - (void)tintColorDidChange [node tintColorDidChange]; } -- (BOOL)canBecomeFirstResponder { - ASDisplayNode *node = _asyncdisplaykit_node; // Create strong reference to weak ivar. - return [node canBecomeFirstResponder]; -} - -- (BOOL)canResignFirstResponder { - ASDisplayNode *node = _asyncdisplaykit_node; // Create strong reference to weak ivar. - return [node canResignFirstResponder]; -} +#pragma mark UIResponder Handling + +#define IMPLEMENT_RESPONDER_METHOD(__sel, __methodOverride) \ +- (BOOL)__sel\ +{\ + ASDisplayNode *node = _asyncdisplaykit_node; /* Create strong reference to weak ivar. */ \ + SEL sel = @selector(__sel); \ + /* Prevent an infinite loop in here if [super canBecomeFirstResponder] was called on a + / _ASDisplayView subclass */ \ + if (self->_methodOverrides & __methodOverride) { \ + /* Check if we can call through to ASDisplayNode subclass directly */ \ + if (ASDisplayNodeSubclassOverridesSelector([node class], sel)) { \ + return [node __sel]; \ + } else { \ + /* Call through to views superclass as we expect super was called from the + _ASDisplayView subclass and a node subclass does not overwrite canBecomeFirstResponder */ \ + return [self __##__sel]; \ + } \ + } else { \ + /* Call through to internal node __canBecomeFirstResponder that will consider the view in responding */ \ + return [node __##__sel]; \ + } \ +}\ +/* All __ prefixed methods are called from ASDisplayNode to let the view decide in what UIResponder state they \ +are not overridden by a ASDisplayNode subclass */ \ +- (BOOL)__##__sel \ +{ \ + return [super __sel]; \ +} \ + +IMPLEMENT_RESPONDER_METHOD(canBecomeFirstResponder, _ASDisplayViewMethodOverrideCanBecomeFirstResponder); +IMPLEMENT_RESPONDER_METHOD(becomeFirstResponder, _ASDisplayViewMethodOverrideBecomeFirstResponder); +IMPLEMENT_RESPONDER_METHOD(canResignFirstResponder, _ASDisplayViewMethodOverrideCanResignFirstResponder); +IMPLEMENT_RESPONDER_METHOD(resignFirstResponder, _ASDisplayViewMethodOverrideResignFirstResponder); +IMPLEMENT_RESPONDER_METHOD(isFirstResponder, _ASDisplayViewMethodOverrideIsFirstResponder); - (BOOL)canPerformAction:(SEL)action withSender:(id)sender { diff --git a/Source/Private/ASDisplayNode+UIViewBridge.mm b/Source/Private/ASDisplayNode+UIViewBridge.mm index 578caa7f6..13e932ef5 100644 --- a/Source/Private/ASDisplayNode+UIViewBridge.mm +++ b/Source/Private/ASDisplayNode+UIViewBridge.mm @@ -96,16 +96,6 @@ ASDISPLAYNODE_INLINE BOOL ASDisplayNodeShouldApplyBridgedWriteToView(ASDisplayNo */ @implementation ASDisplayNode (UIViewBridge) -- (BOOL)canBecomeFirstResponder -{ - return NO; -} - -- (BOOL)canResignFirstResponder -{ - return YES; -} - #if TARGET_OS_TV // Focus Engine - (BOOL)canBecomeFocused @@ -146,23 +136,34 @@ - (UIView *)preferredFocusedView } #endif +- (BOOL)canBecomeFirstResponder +{ + ASDisplayNodeAssertMainThread(); + return [self __canBecomeFirstResponder]; +} + +- (BOOL)canResignFirstResponder +{ + ASDisplayNodeAssertMainThread(); + return [self __canResignFirstResponder]; +} + - (BOOL)isFirstResponder { ASDisplayNodeAssertMainThread(); - return _view != nil && [_view isFirstResponder]; + return [self __isFirstResponder]; } -// Note: this implicitly loads the view if it hasn't been loaded yet. - (BOOL)becomeFirstResponder { ASDisplayNodeAssertMainThread(); - return !self.layerBacked && [self canBecomeFirstResponder] && [self.view becomeFirstResponder]; + return [self __becomeFirstResponder]; } - (BOOL)resignFirstResponder { ASDisplayNodeAssertMainThread(); - return !self.layerBacked && [self canResignFirstResponder] && [_view resignFirstResponder]; + return [self __resignFirstResponder]; } - (BOOL)canPerformAction:(SEL)action withSender:(id)sender diff --git a/Source/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h index 545f1c5a2..ef2d460a3 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -275,6 +275,13 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo - (void)__incrementVisibilityNotificationsDisabled; - (void)__decrementVisibilityNotificationsDisabled; +// Helper methods for UIResponder forwarding +- (BOOL)__canBecomeFirstResponder; +- (BOOL)__becomeFirstResponder; +- (BOOL)__canResignFirstResponder; +- (BOOL)__resignFirstResponder; +- (BOOL)__isFirstResponder; + /// Helper method to summarize whether or not the node run through the display process - (BOOL)_implementsDisplay; diff --git a/Tests/ASDisplayNodeTests.mm b/Tests/ASDisplayNodeTests.mm index 17bf960e0..fcd277152 100644 --- a/Tests/ASDisplayNodeTests.mm +++ b/Tests/ASDisplayNodeTests.mm @@ -29,6 +29,7 @@ #import "ASDisplayNodeTestsHelper.h" #import #import +#import #import #import #import @@ -87,6 +88,10 @@ XCTAssertFalse(n.nodeLoaded, @"%@ should not be loaded", n.debugName);\ } +@interface UIWindow (Testing) +// UIWindow has this handy method that is not public but great for testing +- (UIResponder *)firstResponder; +@end @interface ASDisplayNode (HackForTests) - (id)initWithViewClass:(Class)viewClass; @@ -256,6 +261,20 @@ - (BOOL)resignFirstResponder { @end +@interface UIResponderNodeTestDisplayViewCallingSuper : _ASDisplayView +@end +@implementation UIResponderNodeTestDisplayViewCallingSuper +- (BOOL)canBecomeFirstResponder { return YES; } +- (BOOL)becomeFirstResponder { return [super becomeFirstResponder]; } +@end + +@interface UIResponderNodeTestViewCallingSuper : UIView +@end +@implementation UIResponderNodeTestViewCallingSuper +- (BOOL)canBecomeFirstResponder { return YES; } +- (BOOL)becomeFirstResponder { return [super becomeFirstResponder]; } +@end + @interface ASDisplayNodeTests : XCTestCase @end @@ -264,18 +283,77 @@ @implementation ASDisplayNodeTests dispatch_queue_t queue; } -- (void)testOverriddenFirstResponderBehavior { +- (void)testOverriddenNodeFirstResponderBehavior +{ ASTestDisplayNode *node = [[ASTestResponderNode alloc] init]; XCTAssertTrue([node canBecomeFirstResponder]); XCTAssertTrue([node becomeFirstResponder]); } -- (void)testDefaultFirstResponderBehavior { +- (void)testOverriddenDisplayViewFirstResponderBehavior +{ + UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; + ASDisplayNode *node = [[ASDisplayNode alloc] initWithViewClass:[UIResponderNodeTestDisplayViewCallingSuper class]]; + + // We have to add the node to a window otherwise the super responder methods call responses are undefined + // This will also create the backing view of the node + [window addSubnode:node]; + [window makeKeyAndVisible]; + + XCTAssertTrue([node canBecomeFirstResponder]); + XCTAssertTrue([node becomeFirstResponder]); +} + +- (void)testOverriddenViewFirstResponderBehavior +{ + UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; + ASDisplayNode *node = [[ASDisplayNode alloc] initWithViewClass:[UIResponderNodeTestViewCallingSuper class]]; + + // We have to add the node to a window otherwise the super responder methods call responses are undefined + // This will also create the backing view of the node + [window addSubnode:node]; + [window makeKeyAndVisible]; + + XCTAssertTrue([node canBecomeFirstResponder]); + XCTAssertTrue([node becomeFirstResponder]); +} + +- (void)testDefaultFirstResponderBehavior +{ ASTestDisplayNode *node = [[ASTestDisplayNode alloc] init]; XCTAssertFalse([node canBecomeFirstResponder]); XCTAssertFalse([node becomeFirstResponder]); } +- (void)testResponderMethodsBehavior +{ + UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; + ASEditableTextNode *textNode = [[ASEditableTextNode alloc] init]; + + // We have to add the text node to a window otherwise the responder methods responses are undefined + // This will also create the backing view of the node + [window addSubnode:textNode]; + [window makeKeyAndVisible]; + + XCTAssertTrue([textNode canBecomeFirstResponder]); + XCTAssertTrue([textNode becomeFirstResponder]); + XCTAssertTrue([window firstResponder] == textNode.textView); + XCTAssertTrue([textNode resignFirstResponder]); + + // If the textNode resigns it's first responder the view should not be the first responder + XCTAssertTrue([window firstResponder] == nil); + XCTAssertFalse([textNode.view isFirstResponder]); +} + +- (void)testUnsupportedResponderSetupWillThrow +{ + ASTestResponderNode *node = [[ASTestResponderNode alloc] init]; + [node setViewBlock:^UIView * _Nonnull{ + return [[UIView alloc] init]; + }]; + XCTAssertThrows([node view], @"Externally provided views should be synchronous"); +} + - (void)setUp { [super setUp]; diff --git a/Tests/ASTableViewTests.mm b/Tests/ASTableViewTests.mm index 939c910fb..fe54a4895 100644 --- a/Tests/ASTableViewTests.mm +++ b/Tests/ASTableViewTests.mm @@ -139,6 +139,7 @@ - (ASLayoutSpec *)layoutSpecThatFits:(ASSizeRange)constrainedSize @interface ASTableViewFilledDataSource : NSObject @property (nonatomic) BOOL usesSectionIndex; +@property (nonatomic) NSInteger numberOfSections; @property (nonatomic) NSInteger rowsPerSection; @property (nonatomic, nullable, copy) ASCellNodeBlock(^nodeBlockForItem)(NSIndexPath *); @end @@ -149,6 +150,7 @@ - (instancetype)init { self = [super init]; if (self != nil) { + _numberOfSections = NumberOfSections; _rowsPerSection = 20; } return self; @@ -165,7 +167,7 @@ - (BOOL)respondsToSelector:(SEL)aSelector - (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView { - return NumberOfSections; + return _numberOfSections; } - (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section @@ -833,6 +835,52 @@ - (void)testAutomaticallyAdjustingContentOffset XCTAssertEqual(node.contentOffset.y, 10); } +- (void)testTableViewReloadDoesReloadIfEditableTextNodeIsFirstResponder +{ + ASEditableTextNode *editableTextNode = [[ASEditableTextNode alloc] init]; + + UIWindow *window = [[UIWindow alloc] initWithFrame:CGRectMake(0, 0, 375, 667)]; + ASTableNode *node = [[ASTableNode alloc] initWithStyle:UITableViewStyleGrouped]; + node.frame = window.bounds; + [window addSubnode:node]; + + ASTableViewFilledDataSource *dataSource = [ASTableViewFilledDataSource new]; + dataSource.rowsPerSection = 1; + dataSource.numberOfSections = 1; + // Currently this test requires that the text in the cell node fills the + // visible width, so we use the long description for the index path. + dataSource.nodeBlockForItem = ^(NSIndexPath *indexPath) { + return (ASCellNodeBlock)^{ + ASCellNode *cellNode = [[ASCellNode alloc] init]; + cellNode.automaticallyManagesSubnodes = YES; + cellNode.layoutSpecBlock = ^ASLayoutSpec * _Nonnull(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize) { + return [ASInsetLayoutSpec insetLayoutSpecWithInsets:UIEdgeInsetsMake(10, 10, 10, 10) child:editableTextNode]; + }; + return cellNode; + }; + }; + node.delegate = dataSource; + node.dataSource = dataSource; + + // Reload the data for the initial load + [node reloadData]; + [node waitUntilAllUpdatesAreProcessed]; + [node setNeedsLayout]; + [node layoutIfNeeded]; + + // Set the textView as first responder + [editableTextNode.textView becomeFirstResponder]; + + // Change data source count and try to reload a second time + dataSource.rowsPerSection = 2; + [node reloadData]; + [node waitUntilAllUpdatesAreProcessed]; + + // Check that numberOfRows in section 0 is 2 + XCTAssertEqual([node numberOfRowsInSection:0], 2); + XCTAssertEqual([node.view numberOfRowsInSection:0], 2); +} + @end @implementation UITableView (Testing)