Fix UIResponder handling with view backing ASDisplayNode by maicki · Pull Request #789 · TextureGroup/Texture

maicki · 2018-02-05T18:08:26Z

What is the problem?

If certain UIResponder methods are overwritten in a ASDisplayNode subclass they are not considered. The only two methods that are currently properly forwarded are: canBecomeFirstResponder and canResignFirstResponder in _ASDisplayView.mm. Every other UIResponder method will never reach any _ASDisplayView. This especially is problem for ASEditableTextNode as we overwrite these methods in there and expect to be considered.

Example repo: https://github.com/maicki/TextureResponderChain

Tests

I added a test that show's this invalid behavior. You can find them under ASDisplayNodeTests.mm and ASTableViewTests.mm

Some more details

The way I actually stumbled upon the issue is that as soon as an ASEditableTextNode is a subnode of a ASCellNode as well as a first responder, calling reloadData on an ASTableNode will not reload the UITableView. UIKit will bail out within the process.
Internally UIKit's UITableView is trying to resign the first responder from the current first responder on an reloadData what is the textView of the ASEditableTextNode and afterwards expects no view in the UITableView hierarchy is the first responder anymore. That will not be case if UITableView is calling resignFirstResponder on the ASEditableTextNode.textView as we push only the first responder to the ASEditableTextNode.view. UITableView has an ivar called _firstResponderView where it determines if the first responder view is a view in the UITableView hierarchy. If this is ivar != null it will bail out if reloadData is called, before actually reloading the data (I verified that within Hopper).
And so if the UITableView is calling resignFirstResponder on the ASEditableTextNode.textView the _firstResponderView will just be ASEditableTextNode.view, but it should be nil. If we properly forward all responder methods _firstResponderView will be nil as the ASEditableTextNode.view will not become the first responder if the table view calls resignFirstResponder on the ASEditableTextNode.textView.

Current solution

// Called canBecomeFirstResponder on _ASDisplayView
-> [_ASDisplayView canBecomeFirstResponder]
  -> if subclass of _ASDisplayView overwrites canBecomeFirstResponder
      // Prevent an infinite loop in here if [super canBecomeFirstResponder] was called
      -> if subclass of ASDisplayNode overwrites canBecomeFirstResponder
        -> [ASDisplayNode canBecomeFirstResponder];
      -> else
        // Call through to views superclass as we expect super was called from the
        // _ASDisplayView subclass and node does not implement canBecomeFirstResponder
        -> [_ASDisplayView __canBecomeFirstResponder]
          -> [super canBecomeFirstResponder]
  -> else
      // Call through to the display node to give a chance to dive in
    -> [ASDisplayNode __canBecomeFirstResponder]

// Called canBecomeFirstResponder on ASDisplayNode
-> [ASDisplayNode+UIViewBridge canBecomeFirstResponder]
  // Just call through to our internal method
  -> [ASDisplayNode (self) __canBecomeFirstResponder]

// Called form _ASDisplayView and ASDisplayNode from ASDisplayNode+UIViewBridge
-> [ASDisplayNode __canBecomeFirstResponder]
  -> if view it is a subclass of _ASDisplayView (synchronous == TRUE)
    // If it's synchronous just call through to the view as we expect it's
    // a non _ASDisplayView subclass that will respond
    -> [_view canBecomeFirstResponder]
  -> else
      -> if subclass of _ASDisplayView overwrites canBecomeFirstResponder
        // If the subclass overwrites canBecomeFirstResponder just call through
        // to it as we expect it will handle it
        [_view canBecomeFirstResponder];
      -> else
        // Call through to _ASDisplayView to get it handled
        -> [_ASDisplayView __canBecomeFirstResponder]
          -> [super canBecomeFirstResponder]

ghost · 2018-02-05T18:11:11Z

🚫 CI failed with log

maicki · 2018-02-05T19:39:12Z

+  }
+
+  if (checkFlag(Synchronous)
+      || ASSubclassOverridesSelector([_ASDisplayView class], _viewClass, @selector(canBecomeFirstResponder))) {


We may should catch all of them in the initialization phase like ASDisplayNodeMethodOverrides. That said at that point we may not know the actual view class so we only could do it after the view is created.

maicki · 2018-02-06T17:11:26Z

+  // We explicitly create the view in here as it's supposed to become a first responder
+  [self view];
+
+  if (checkFlag(Synchronous)) {


This should probably extracted to a macro as the only change is the selector

maicki · 2018-02-06T17:11:50Z

-  return [node canResignFirstResponder];
+- (BOOL)becomeFirstResponder
+{
+  ASDisplayNode *node = _asyncdisplaykit_node;


This parts also be extracted to a macro.

timonus · 2018-02-06T17:15:24Z

Thank you so much for fixing this Michael!

appleguy · 2018-02-09T20:14:06Z

Amazing analysis and fix @maicki !

I never knew about this behavior, but it totally makes sense to avoid text fields being cleared by accidental app behaviors when a user is actively editing them.

ghost · 2018-02-09T20:43:42Z

🚫 CI failed with log

…g UIResponder methods

appleguy · 2018-02-20T02:57:41Z

@maicki thanks for the ping, I'll take a look soon! Thanks for the really awesome description, and analysis underlying it :).

appleguy

Thanks especially for the tests! That significantly helps our future-looking confidence to maintain correctness of these details.

appleguy · 2018-02-20T04:32:30Z

+  // 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


Any concerns with touchesBegan:, touchesMoved:, touchesEnded:, or touchesCancelled: ?

I didn't look into them or saw any issues related to them.

appleguy · 2018-02-20T04:34:56Z

+#pragma mark UIResponder
+
+#define HANDLE_RESPONDER_METHOD(__sel) \
+  if (checkFlag(Synchronous)) { \


This should probably assert main thread. At least -becomeFirstResponder might be reasonable for a developer to expect to be a UIViewBridge property and threadsafe to call, but this implementation triggers view creation.

Maybe worth even a special assertion in -becomeFirstResponder that tells developers to call that method on the main thread or in onDidLoad / -didLoad.

Agree, I added a main thread assertion for all responder methods.

appleguy · 2018-02-20T04:37:38Z

+
+- (BOOL)__becomeFirstResponder
+{
+  if (self.isLayerBacked || ![self canBecomeFirstResponder]) {


Check for isLayerBacked is probably redundant, and also grabs lock. Could check _view if a short circuit is needed, as this method is main thread only then lock isn't needed there.

I think we can get rid of the isLayerBacked call at all. We check for the _view in canBecomeFirstResponder anyway that should cover that case.

appleguy · 2018-02-20T04:40:24Z

  _flags.viewEverHadAGestureRecognizerAttached = YES;
 }

+#pragma mark UIResponder


IMO this should go in another file, since ASDisplayNode.mm is quite large - it seems very appropriate for UIViewBridge or perhaps we could introduce another one.

Not a blocking change (feel free to land this if you're just eager to get it in!)

We are currently have the event handling touchesBegan: etc. in ASDisplayNode.mm, eventually we should move both of them out too in a follow up diff.

appleguy · 2018-02-20T04:41:56Z

 - (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


This first sentence isn't quite clear -- "for ASDisplayNode responder methods to let the view call super responder methods".

Clarify if possible, otherwise not exactly sure how to improve it.

The sentence is kind of the same as the one for all of the forwarding touch events. In that case we should find a better way for both.

appleguy · 2018-02-20T04:45:24Z

+
+- (BOOL)canBecomeFirstResponder
+{
+  HANDLE_RESPONDER_METHOD(canBecomeFirstResponder);


I think you could use cmd for each of these. They could also be made a macro for IMPLEMENT_RESPONDER_METHOD instead of HANDLE, and define the method as well as its contents.

Moved all into one macros and called it IMPLEMENT_RESPONDER_METHOD

appleguy · 2018-02-20T04:46:58Z

+#pragma mark UIResponder Handling
+
+#define HANDLE_RESPONDER_METHOD(__sel) \
+  ASDisplayNode *node = _asyncdisplaykit_node; \


Can you keep the comment // Create a strong reference to weak ivar, from the original code? It would be easy to think this could be dropped and accessed directly on the ivar, but I assume the implication is that the ivar might change or become invalid.

Added in back

appleguy · 2018-02-20T04:47:41Z

+// This methods are called from ASDisplayNode to let the view decide in what UIResponder state they are not overwritten
+// by a ASDisplayNode subclass
+
+- (BOOL)__canBecomeFirstResponder


Technically these could be macro-ized as well, generated in a pair with the above ones :). Optional of course.

Nits: 2 spaces instead of 4 on the returns. Also, "overridden" instead of "overwritten" in the comment.

Also moved this into the macro and to 2 spaces.

appleguy · 2018-02-20T04:49:07Z

+  return [self __isFirstResponder];
 }

-// Note: this implicitly loads the view if it hasn't been loaded yet.


Seems like a good comment to keep.

We had this kind of comment in __becomeFirstResponder now:

// Note: this implicitly loads the view if it hasn't been loaded yet.

I changed it to the old comment in there though. Let me know if I should also add it to this place back again.

appleguy · 2018-02-20T04:50:07Z

+- (BOOL)canBecomeFirstResponder { return YES; }
+- (BOOL)becomeFirstResponder {
+  return [super becomeFirstResponder];
+


Extra return

maicki · 2018-02-22T00:55:33Z

@appleguy Thank you for reviewing. Can you take another look over it please. Thanks!

Adlai-Holler

Damn impressive work Michael. I'm down to land.

Adlai-Holler · 2018-02-22T18:02:18Z

+
+  // Check that numberOfRows in section 0 is 2
+  XCTAssertTrue([node numberOfRowsInSection:0] == 2, @"Number of rows in section 0 should be 2 after reload");
+  XCTAssertTrue([node.view numberOfRowsInSection:0] == 2, @"Number of rows in section 0 should be 2 after reload");


Nit: use XCTAssertEqual so that we can see the failing value if this fails on CI in the future.

…p#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

maicki commented Feb 5, 2018

View reviewed changes

maicki commented Feb 6, 2018

View reviewed changes

maicki added 7 commits February 17, 2018 10:01

Add failing tests

27a9013

Fix responder chain handling in Texture

e5dae0f

Add mores tests that horrible fail

661a297

Add Changelog.md entry

e27e300

Some fixes

b6dd993

Update logic

7b361e2

Add tests that prevents infinite loops if a custom view is overwritin…

87d0df2

…g UIResponder methods

maicki force-pushed the MSFixResponderMethods branch from d129d45 to 87d0df2 Compare February 17, 2018 18:02

maicki added 3 commits February 17, 2018 10:46

Add macro to forward methods in ASDisplayNode

77cbec0

Add macro for forwarding responder methods in _ASDisplayView

8336359

Remove junk

cc30688

maicki requested review from Adlai-Holler, appleguy and nguyenhuy February 17, 2018 19:17

appleguy approved these changes Feb 20, 2018

View reviewed changes

maicki added 2 commits February 21, 2018 14:47

Address first comments

f405577

Update _ASDisplayView to cache responder forwarding methods

4f3e113

maicki force-pushed the MSFixResponderMethods branch from c79bcee to 4f3e113 Compare February 22, 2018 00:54

Adlai-Holler approved these changes Feb 22, 2018

View reviewed changes

Use XCTAssertEqual

c71b3f5

maicki merged commit 236cdd7 into master Feb 23, 2018

wsdwsd0829 mentioned this pull request Mar 10, 2018

access view first before checking canBecome/Resign responder in becomeResponder methods #829

Merged

Adlai-Holler deleted the MSFixResponderMethods branch March 11, 2018 23:36

Uh oh!

Conversation

maicki commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the problem?

Tests

Some more details

Current solution

Uh oh!

ghost commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timonus commented Feb 6, 2018

Uh oh!

appleguy commented Feb 9, 2018

Uh oh!

ghost commented Feb 9, 2018

Uh oh!

appleguy commented Feb 20, 2018

Uh oh!

appleguy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maicki Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maicki Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maicki commented Feb 22, 2018

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

maicki commented Feb 5, 2018 •

edited

Loading

maicki Feb 21, 2018 •

edited

Loading

maicki Feb 21, 2018 •

edited

Loading