Introducing tryFindingViewInFrame() to avoid scroll #1296

SimoncelloCT · 2024-07-12T21:49:56Z

Description

This MR introduces the method tryFindingViewInFrame(withAccessibilityIdentifier: ) that allows to check the visibility of an element in the current frame as it is showed to the user, without any additional interaction.
The method usingCurrentFrame() has been added in the new KIFUIViewTestActor to maintain parity with KIFUITestActor.

Why?

In some UITests we want to ensure an element is visible in the current frame.

When the element we want to assert is visible is inside a scrollView,tableView or collectionView the method tryFindingView(withAccessibilityIdentifier: ) causes a scroll to the offset where the element is placed, making it visible even if it wasn't. This behaviour does not allow to have reliable visibility checks (eg. if an element exists but is not actually visible, the method returns true).

EDIT: this MR also fixes a bug: under some conditions a control was considered tappable when just enabled and having a TapGestureRecognizer, even when it was occluded by another element.

justinseanmartin

Thank you so much for the contribution! Would you be able to add an integration test that verifies this functionality is working as expected and ensures we don't regress it going forward?

justinseanmartin · 2024-07-19T07:10:27Z

Sources/KIF/IdentifierTests/KIFUITestActor-IdentifierTests.h

+ @discussion if the element described by the accessibility identifier is visible, the method returns true.
+ @param accessibilityIdentifier The accessibility identifier of the element to query for
+ */
+- (BOOL) tryFindingViewInFrameWithAccessibilityIdentifier:(NSString *) accessibilityIdentifier;


We've been trying to maintain parity between KIFUITestActor (the old API) and KIFUIViewTestActor (the new API). Would you be willing to add a method like usingCurrentFrame or usingAutomaticScrolling:NO to KIFUIViewTestActor. This would be similar to how usingTimeout: works on KIFTestActor to alter matching behavior, but not necessarily altering the predicates used to do the matching. It'd store that state in the actor (defaults to scrolling if not specified) and then pass the scrolling behavior property through here (and a few lines below). Thoughts?

Hey @justinseanmartin, yes it totally makes sense! I'm going to commit these changes today 👌

Take a look now 😄

Looks great, thanks for the iteration!

SimoncelloCT · 2024-07-24T10:52:44Z

Hey @justinseanmartin , I added all the changes we've discussed and also fixed somethings specific for scrollViews, I found my prev solution was working only with TableViews and CollectionViews.

I checked that all the tests are passing as expected:

Thank you for the guidance and hope to see this PR merged soon 😄 🙏

justinseanmartin · 2024-07-26T05:59:07Z

I've kicked off a CI run and will check back on it in the morning to see if it is good to merge. There have been a couple of flaky tests in the integration test suite, so I'll rekick the job once or twice if it looks like that's happening.

Sources/KIF/Additions/UIView-Debugging.m

SimoncelloCT · 2024-07-29T22:00:30Z

Sources/KIF/Additions/UIView-KIFAdditions.m

+    return ([self isTappableInRect:self.bounds] || ([self hasTapGestureRecognizerAndIsControlEnabled] &&
+            [self isTappableInRect:self.bounds]));


Hello @justinseanmartin , I've found a bug here: a view was considered tappable even when occluded by another element, because isTappable was returning true even when only hasTapGestureRecognizerAndIsControlEnabled was true.

Since this is a reliability issue, I thought to take advantage of the MR that has not been merged yet and fix it. I've added also integration tests to make sure it will be maintained over time 😀

All the tests are passing locally, but let's wait for the CI checks!

I edited also the MR description, let me know if something is not clear 🙏🏻

Was this breaking something for you, or just something you noticed that seemed wrong while you were in here?

I want to say that this was by design, because sometimes the tap event might trigger even if the element isn't the one directly that gets returned by a hit test. I'll verify by pointing our internal tests at this branch and see if anything breaks.

IIUC - I think the boolean logic you have here is equivalent to return [self isTappableInRect:self.bounds];, disregarding whether hasTapGestureRecognizerAndIsControlEnabled is true or false. If that's the case, we should simplify this, but I do think there are cases where this was necessary.

Thanks for your answer! Indeed the boolean logic is the same, it can be simplified if we leave this change (sorry for that, just a bit tired 😅).
Yes it breaks tappable checks: calling tryFindingTappableView() I was getting true for occluded views, and the view is not tappable when occluded by another one.
You can easily check it running the integration test I've added testTryFindingOccludedTappableViewInFrameWithAccessibilityIdentifier without this change.

If you have a better idea on how to fix it, let me know and I can revert this change and follow your guidance 👍

Looking at the history, we tried to remove this check in #1244 and ran into problems that led us to reverting that change in #1250.

I think the way I'd probably address this for now would be in isTappable to grab the intersection of the self.bounds rect with the view's window (might require translating coordinate systems) that pass that into isTappableInRect:. We could probably add a small optimization in tappablePointInRect: to early return with NO if the supplied rect has 0 by 0 dimensions.

I confirmed that it does break a couple tests on a few screens in our app, but relatively minimal breakage. It might be something that we can work around on our end. In particular, I saw it trying to tap on a search field's placeholder text and saying it wasn't tappable. If we can avoid the breakage while still getting you the functionality that you need, that seems preferable.

Can we add a condition to check the isVisibleInWindowFrame of this view if it isn't tappable but has a gesture recognizer?

Hey Justin, thank you for your answers, sorry but I didn't have the time to check them before.

I've tried with a check like:

([self isTappableInRect:self.bounds] || ([self hasTapGestureRecognizerAndIsControlEnabled] && [self isVisibleInWindowFrame]));

but we have the same issue: An element that has a tap gesture recognizer and is visible in the current frame but occluded by another view is considered as tappable and this breaks some tests on our side (the test I've added testTryFindingOccludedTappableViewInFrameWithAccessibilityIdentifier fails too)..

So we still have the same issue, the element is actually not tappable (because totally occluded) but considered as it is 😢
I don't think there is a way to consider an element tappable when the check isTappableInRect:self.bounds fails, without affecting the reliability of this method tbh, but I might still miss something

What do you think?

Ok, I figured out an issues with one of our test suites caused by this change.

We have a search bar element that contains a search text field as a subview. The text field is being marked with userInteractionEnabled = false, because the tap handler for the search bar redirects to another screen instead of allowing to type into it directly the text field. However, the accessibilityIdentifier we were matching on is being set on the search text field (matching what is shown in the text field placeholder). This works today because the element matching the hitTest (the search bar) has a tap gesture recognizer on it.

In our case, I can change the predicate to match on the accessibilityIdentifier of the search bar instead and things seem to work in this case. That said, this would also potentially break tests at other companies as well. I also still have another failure to look into still that isn't related to this case.

On the surface, it seems like it should be reasonable to allow the tap action to be handled by a superview that can process touch events. From a users perspective, they don't know or care if the tap is being handled by the search text field or the search bar. That said, trying to make isTappableWithHitTestResultView: allow for this seems to cause different problems, where we'll match on occluded elements that don't actually respond to touch events (like a label within a button).

I'm still thinking about this, but wanted to give a bit of an update here.

Yeah, I got it, thank you for the update. We can keep each other updated here, I'll let you know if any idea comes to my mind when I'll have the time to work a bit on this

Hey @justinseanmartin, I've experimented a bit more too but I wasn't able to find a solution to allow both cases 😞.

On a more personal note, I think it's reasonable in UITests to refer always to the actual element that should handle the tap (this is what we usually do, but as you mentioned I don't know if referring to a subview, which cannot handle tap events, is something usual in in other companies tests. The change might be justified in that case if we confirm there is no better solution to make the check reliable).

Are you still trying to find a solution for this?

I'm sorry to push for an answer, the fact is that if we cannot merge the change for the reason above we will create a fork in our company GitHub account to avoid keeping reference to my personal account's fork
Thanks 🙏

justinseanmartin · 2024-07-30T17:57:58Z

FYI - you might want to merge in or rebase on top of #1297

justinseanmartin · 2024-08-05T18:40:22Z

I think if you rebase, we should be good to land this. We're going to work to drop Xcode 13 and getting things green on Xcode 15 in #1298, but I wouldn't block this PR on that.

I forgot that we still need to resolve the issue about tappability and gesture recognizers. Will put a reply in that thread.

- avoid scroll when looking for a view visibility

… automatic scroll if needed

- fix wrong parameter name in comments - add missing methods - add missing things that prevent us to build for tests

- with scrollDisabled = YES it now consider only views visible in the current frame, as it was done with TableView and CollectionViews - make explicit the scrollDisabled argument in recursive calls = NO when we are inside the scrolling section

…dentifier() and usingCurrentFrame()

This reverts commit 6827225.

SimoncelloCT · 2024-08-05T20:38:43Z

@justinseanmartin The branch has been rebased on top of master, let me know if we could still try something for the tappability check or it is good to go! 🙏🏻

Sources/KIF/Additions/UIAccessibilityElement-KIFAdditions.m

Co-authored-by: Justin Martin <jmartin@squareup.com>

… tests

SimoncelloCT · 2024-08-20T00:36:10Z

@justinseanmartin I see 2 tests were broken because of a duplicate label in the TextField I've added for the new tests, I've pushed a fix for that, now all tests should pass

justinseanmartin · 2024-09-17T23:23:47Z

@justinseanmartin I see 2 tests were broken because of a duplicate label in the TextField I've added for the new tests, I've pushed a fix for that, now all tests should pass

@SimoncelloCT - Hey, apologies for the delay here. I'm working on fixing the CI configuration in #1300 and getting it to run on Xcode 14 & 15 successfully. Once that's done, we should be able to land this. I also found a problem in the pullToRefresh tests that would could help to use your new usingCurrentFrame. I'm actively working on it this week, so hopefully should have this in a state to be landed soon.

justinseanmartin · 2024-09-19T20:57:07Z

I've got the CI fixes in #1300. I've confirmed that this works in that context, so I'm going to land this PR, even though it has red builds.

SimoncelloCT changed the title ~~introduce tryFindingViewInFrame~~ Introducing tryFindingViewInFrame() to avoid scroll Jul 12, 2024

justinseanmartin reviewed Jul 19, 2024

View reviewed changes

justinseanmartin approved these changes Jul 26, 2024

View reviewed changes

SimoncelloCT commented Jul 26, 2024

View reviewed changes

Sources/KIF/Additions/UIView-Debugging.m Outdated Show resolved Hide resolved

SimoncelloCT commented Jul 29, 2024

View reviewed changes

SimoncelloCT force-pushed the feature-tryFindingViewInFrame-noscroll branch from 2b979c3 to 6827225 Compare July 30, 2024 10:21

Simone Scionti added 12 commits August 5, 2024 22:25

introduce tryFindingViewInFrame

5d01a68

- avoid scroll when looking for a view visibility

remove wrong comment

1951085

improve comments

bf2ea3c

add missing disableScroll argument

19e52ff

introduce usingCurrentFrame() method on KIFUIViewTestActor to disable…

77f3597

… automatic scroll if needed

fixes:

801d7d9

- fix wrong parameter name in comments - add missing methods - add missing things that prevent us to build for tests

fix to scrollView behavior:

6b024e6

- with scrollDisabled = YES it now consider only views visible in the current frame, as it was done with TableView and CollectionViews - make explicit the scrollDisabled argument in recursive calls = NO when we are inside the scrolling section

add new integration tests for tryFindingViewInFrameWithAccessibilityI…

5ebdd68

…dentifier() and usingCurrentFrame()

Fix bug that was considering an occluded view as tappable

671b227

simplify boolean check and add integration test for broken case

2a7e011

Revert "simplify boolean check and add integration test for broken case"

23bcc0e

This reverts commit 6827225.

simplify boolean logic of the tappable check

89327a2

SimoncelloCT force-pushed the feature-tryFindingViewInFrame-noscroll branch from d9cf92a to 89327a2 Compare August 5, 2024 20:28

justinseanmartin reviewed Aug 8, 2024

View reviewed changes

Sources/KIF/Additions/UIAccessibilityElement-KIFAdditions.m Outdated Show resolved Hide resolved

SimoncelloCT and others added 2 commits August 9, 2024 11:16

add missing fromRootView argument

260b36f

Co-authored-by: Justin Martin <jmartin@squareup.com>

Change duplicated accessibilityLabel that was causing flakiness for 2…

3b50c18

… tests

justinseanmartin merged commit 29ac3e1 into kif-framework:master Sep 19, 2024
2 of 6 checks passed

This was referenced Sep 20, 2024

Get KIF CI working reliably with Xcode 14.3.1 & 15.4 #1300

Merged

jmartin/xcode 16 justinseanmartin/KIF#3

Closed

Fix UICollectionView cell enumeration hitting asserts in Xcode 16 #1307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing tryFindingViewInFrame() to avoid scroll #1296

Introducing tryFindingViewInFrame() to avoid scroll #1296

SimoncelloCT commented Jul 12, 2024 •

edited

Loading

justinseanmartin left a comment

justinseanmartin Jul 19, 2024

SimoncelloCT Jul 23, 2024

SimoncelloCT Jul 24, 2024

justinseanmartin Jul 26, 2024

SimoncelloCT commented Jul 24, 2024 •

edited

Loading

justinseanmartin commented Jul 26, 2024

SimoncelloCT Jul 29, 2024 •

edited

Loading

justinseanmartin Jul 29, 2024

SimoncelloCT Jul 29, 2024 •

edited

Loading

justinseanmartin Jul 29, 2024

justinseanmartin Jul 30, 2024

justinseanmartin Aug 5, 2024

SimoncelloCT Aug 5, 2024 •

edited

Loading

justinseanmartin Aug 7, 2024

SimoncelloCT Aug 7, 2024

SimoncelloCT Aug 20, 2024

justinseanmartin commented Jul 30, 2024

justinseanmartin commented Aug 5, 2024 •

edited

Loading

SimoncelloCT commented Aug 5, 2024

SimoncelloCT commented Aug 20, 2024 •

edited

Loading

justinseanmartin commented Sep 17, 2024 •

edited

Loading

justinseanmartin commented Sep 19, 2024

		return ([self isTappableInRect:self.bounds] \|\| ([self hasTapGestureRecognizerAndIsControlEnabled] &&
		[self isTappableInRect:self.bounds]));

Introducing tryFindingViewInFrame() to avoid scroll #1296

Introducing tryFindingViewInFrame() to avoid scroll #1296

Conversation

SimoncelloCT commented Jul 12, 2024 • edited Loading

Description

Why?

justinseanmartin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimoncelloCT commented Jul 24, 2024 • edited Loading

justinseanmartin commented Jul 26, 2024

SimoncelloCT Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimoncelloCT Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimoncelloCT Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinseanmartin commented Jul 30, 2024

justinseanmartin commented Aug 5, 2024 • edited Loading

SimoncelloCT commented Aug 5, 2024

SimoncelloCT commented Aug 20, 2024 • edited Loading

justinseanmartin commented Sep 17, 2024 • edited Loading

justinseanmartin commented Sep 19, 2024

SimoncelloCT commented Jul 12, 2024 •

edited

Loading

SimoncelloCT commented Jul 24, 2024 •

edited

Loading

SimoncelloCT Jul 29, 2024 •

edited

Loading

SimoncelloCT Jul 29, 2024 •

edited

Loading

SimoncelloCT Aug 5, 2024 •

edited

Loading

justinseanmartin commented Aug 5, 2024 •

edited

Loading

SimoncelloCT commented Aug 20, 2024 •

edited

Loading

justinseanmartin commented Sep 17, 2024 •

edited

Loading