-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix UIResponder handling with view backing ASDisplayNode #789
Conversation
🚫 CI failed with log |
Source/ASDisplayNode.mm
Outdated
} | ||
|
||
if (checkFlag(Synchronous) | ||
|| ASSubclassOverridesSelector([_ASDisplayView class], _viewClass, @selector(canBecomeFirstResponder))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Source/ASDisplayNode.mm
Outdated
// We explicitly create the view in here as it's supposed to become a first responder | ||
[self view]; | ||
|
||
if (checkFlag(Synchronous)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably extracted to a macro as the only change is the selector
Source/Details/_ASDisplayView.mm
Outdated
return [node canResignFirstResponder]; | ||
- (BOOL)becomeFirstResponder | ||
{ | ||
ASDisplayNode *node = _asyncdisplaykit_node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parts also be extracted to a macro.
Thank you so much for fixing this Michael! |
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. |
🚫 CI failed with log |
d129d45
to
87d0df2
Compare
@maicki thanks for the ping, I'll take a look soon! Thanks for the really awesome description, and analysis underlying it :). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks especially for the tests! That significantly helps our future-looking confidence to maintain correctness of these details.
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any concerns with touchesBegan:, touchesMoved:, touchesEnded:, or touchesCancelled: ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look into them or saw any issues related to them.
Source/ASDisplayNode.mm
Outdated
#pragma mark UIResponder | ||
|
||
#define HANDLE_RESPONDER_METHOD(__sel) \ | ||
if (checkFlag(Synchronous)) { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I added a main thread assertion for all responder methods.
Source/ASDisplayNode.mm
Outdated
|
||
- (BOOL)__becomeFirstResponder | ||
{ | ||
if (self.isLayerBacked || ![self canBecomeFirstResponder]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -828,6 +830,94 @@ - (void)nodeViewDidAddGestureRecognizer | |||
_flags.viewEverHadAGestureRecognizerAttached = YES; | |||
} | |||
|
|||
#pragma mark UIResponder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Source/Details/_ASDisplayView.mm
Outdated
|
||
- (BOOL)canBecomeFirstResponder | ||
{ | ||
HANDLE_RESPONDER_METHOD(canBecomeFirstResponder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all into one macros and called it IMPLEMENT_RESPONDER_METHOD
Source/Details/_ASDisplayView.mm
Outdated
#pragma mark UIResponder Handling | ||
|
||
#define HANDLE_RESPONDER_METHOD(__sel) \ | ||
ASDisplayNode *node = _asyncdisplaykit_node; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in back
Source/Details/_ASDisplayView.mm
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also moved this into the macro and to 2 spaces.
} | ||
|
||
// Note: this implicitly loads the view if it hasn't been loaded yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good comment to keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Tests/ASDisplayNodeTests.mm
Outdated
- (BOOL)canBecomeFirstResponder { return YES; } | ||
- (BOOL)becomeFirstResponder { | ||
return [super becomeFirstResponder]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra return
c79bcee
to
4f3e113
Compare
@appleguy Thank you for reviewing. Can you take another look over it please. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn impressive work Michael. I'm down to land.
Tests/ASTableViewTests.mm
Outdated
|
||
// 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
What is the problem?
If certain
UIResponder
methods are overwritten in aASDisplayNode
subclass they are not considered. The only two methods that are currently properly forwarded are:canBecomeFirstResponder
andcanResignFirstResponder
in _ASDisplayView.mm. Every other UIResponder method will never reach any_ASDisplayView
. This especially is problem forASEditableTextNode
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
andASTableViewTests.mm
Some more details
The way I actually stumbled upon the issue is that as soon as an
ASEditableTextNode
is a subnode of aASCellNode
as well as a first responder, callingreloadData
on anASTableNode
will not reload theUITableView
. UIKit will bail out within the process.Internally UIKit's
UITableView
is trying to resign the first responder from the current first responder on anreloadData
what is thetextView
of theASEditableTextNode
and afterwards expects no view in theUITableView
hierarchy is the first responder anymore. That will not be case ifUITableView
is callingresignFirstResponder
on theASEditableTextNode.textView
as we push only the first responder to theASEditableTextNode.view
.UITableView
has an ivar called_firstResponderView
where it determines if the first responder view is a view in theUITableView
hierarchy. If this is ivar != null it will bail out ifreloadData
is called, before actually reloading the data (I verified that within Hopper).And so if the
UITableView
is callingresignFirstResponder
on theASEditableTextNode.textView
the_firstResponderView
will just beASEditableTextNode.view
, but it should be nil. If we properly forward all responder methods_firstResponderView
will be nil as theASEditableTextNode.view
will not become the first responder if the table view callsresignFirstResponder
on theASEditableTextNode.textView
.Current solution