Widen firstResponder searches to all windows
#1143
Conversation
zradke
force-pushed
the
zradke/first-responder-search
branch
from
d4c61b2 to
c0cebe1
Compare
Classes/KIFUITestActor.m
Outdated
| for (UIView *fallback in fallbackViews) { | ||
| if ([fallback isKindOfClass:[UITextField class]] || [fallback isKindOfClass:[UITextView class]] || [fallback isKindOfClass:[UISearchBar class]]) { | ||
| NSLog(@"KIF: Unable to find keyboard key for %@. Inserting manually.", characterString); | ||
| [(UITextField *)fallback setText:[[(UITextField *)fallback text] stringByAppendingString:characterString]]; |
There was a problem hiding this comment.
I wonder if we should print a warning if there is more than one match, to make the surface the somewhat arbitrary decision being made here.
| [self clearTextFromElement:(UIAccessibilityElement *)firstResponder inView:firstResponder]; | ||
| for (UIResponder *firstResponder in [[UIApplication sharedApplication] firstResponders]) { | ||
| if ([firstResponder isKindOfClass:[UIView class]]) { | ||
| [self clearTextFromElement:(UIAccessibilityElement *)firstResponder inView:(UIView *)firstResponder]; |
There was a problem hiding this comment.
We probably should only do this from one element and not multiple.
There was a problem hiding this comment.
Do you mean that I should revert this portion so it continues to only check UIApplication.keyWindow?
There was a problem hiding this comment.
I guess what I mean is to bail from the loop after clearing the text from some first responder. Probably would want to do the same as above and output a warning if there are multiple matching fields and we're only clearing the contents of one of them.
| for (UIResponder *firstResponder in [[UIApplication sharedApplication] firstResponders]) { | ||
| foundResponder = firstResponder; | ||
|
|
||
| if ([foundResponder isKindOfClass:NSClassFromString(@"UISearchBarTextField")]) { |
There was a problem hiding this comment.
:eyeroll: at this special case being here, but not for the other method w/ traits matching below... (no action for you, just noticing this while reviewing)
There was a problem hiding this comment.
Yeah I noticed a lot of inconsistencies around accessibilityLabel checking. Might warrant a deeper follow up to unify that logic.
There was a problem hiding this comment.
In general, KIFUITestActor is the old way of doing things and should eventually be deprecated. It probably isn't worth the time, as everything is already unified in KIFUIViewTestActor (new way).
Classes/KIFUITestActor.m
Outdated
| } | ||
| KIFTestWaitCondition([[firstResponder accessibilityLabel] isEqualToString:label], error, @"Expected accessibility label for first responder to be '%@', got '%@'", label, [firstResponder accessibilityLabel]); | ||
|
|
||
| KIFTestWaitCondition(didMatch, error, @"Expected accessibility label for first responder to be '%@', got '%@'", label, [foundResponder accessibilityLabel]); |
There was a problem hiding this comment.
Maybe the assertions should use dump the whole array of elements (or maybe at least the count of elements checked)? (same applies below)
| KIFTestWaitCondition([[firstResponderIdentification accessibilityIdentifier] isEqualToString:accessibilityIdentifier], | ||
| BOOL didMatch = NO; | ||
| NSString *foundIdentifier = nil; | ||
| for (UIResponder __strong *firstResponder in [[UIApplication sharedApplication] firstResponders]) { |
There was a problem hiding this comment.
Why. is the __strong needed here?
There was a problem hiding this comment.
It lets me reassign to firstResponder in the for loop without having to declare a temporary local variable.
There was a problem hiding this comment.
I'd probably still opt to use the local variable to make the logic more straightforward, but don't feel strongly on this.
| } | ||
| } | ||
|
|
||
| KIFTestWaitCondition(didMatch, |
There was a problem hiding this comment.
NIT: indentation seems off
There was a problem hiding this comment.
Hmm looks like there's a mix of spaces & tabs in this file, so it looks fine with my config but wrong for github's. I'll fix all in file to convert them all to spaces. I'll do it as a separate commit but it will still dirty up the pull request so I'd advise ignoring whitespace afterward.
There was a problem hiding this comment.
Thanks for taking that up!
zradke
force-pushed
the
zradke/first-responder-search
branch
from
c0cebe1 to
f6f5e1d
Compare
Classes/KIFUITestActor.m
Outdated
|
|
||
| for (id fallback in fallbackViews) { | ||
| if ([fallback isKindOfClass:[UITextField class]] || [fallback isKindOfClass:[UITextView class]] || [fallback isKindOfClass:[UISearchBar class]]) { | ||
| if (fallbackViews.count > 1) { |
There was a problem hiding this comment.
I was thinking of gathering the set of first responders that match the class type check and then output the log message only if there is more than one match. Just concerned that this might be too noisy if it is an expected common scenario.
|
LGTM. Take a look at my one latest comment and LMK what you think. I'll merge this as-is if you don't think it is worth addressing. |
Previously searches for `UIWindow.firstResponder` were limited to the `UIApplication.keyWindow`. In practice, it is very possible for users to add additional valid windows that are not the key window and have first responders. To accommodate those cases, this change broadens the search for a first responder of an application to an array of first responders found in `UIApplication.windowsWithKeyWindow`.
zradke
force-pushed
the
zradke/first-responder-search
branch
from
f6f5e1d to
ded9474
Compare
Previously searches for
UIWindow.firstResponderwere limited to theUIApplication.keyWindow. In practice, it is very possible for users toadd additional valid windows that are not the key window and have first
responders. To accommodate those cases, this change broadens the search
for a first responder of an application to an array of first responders
found in
UIApplication.windowsWithKeyWindow.