fix(scrollTo): raise if scroll view element is not hittable. #3115

asafkorem · 2021-11-29T16:36:07Z

Description

This pull request addresses the issue described here: #2834.

In this pull request, I have reproduced the bug from the issue, by adding more tests to the test-app. Apparently, only the scrollTo didn't work as expected because we don't assert if the called element is hidden by another view or window.

This fixes the bug as we did in all other UI interaction actions (taps, long-presses, date-picking), by raising an assertion when the view is not hittable.

Similar failed tests could be achieved by hiding the scroll view with another view, but I still decided to implement them with UIWindow ("overlay") since it is not tested anywhere in our test suite, and that's an iOS-specific behaviour (in Android we touch the visible screen, and not the window behind the element).

Also, in addition to this fix, I rewrote the mechanism of assertions before interacting with a view (for any UI gesture).

noomorph · 2021-11-29T18:26:27Z

detox/test/e2e/35.overlay.test.js

@@ -0,0 +1,46 @@
+const { expectToThrow } = require('./utils/custom-expects');
+
+describe(':ios: Overlay', () => {


I'll run tomorrow, thanks!

noomorph · 2021-11-29T18:27:48Z

@asafkorem maybe I'll mention the obvious, but this change should pass pre-release lifecycle on our Wix internal projects, I guess. Nightly RCs for all the projects.

asafkorem · 2021-11-29T20:03:11Z

detox/ios/Detox/Actions/UIScrollView+DetoxActions.m

+	[self dtx_assertVisibleWithPercent:@1];
+


I don't like this.
However, I don't like the current approach also (>=75% visible), what we use in other views actions (see for example), since the view can be half visible and still accessible..

Also, the normalizedStartingPoint is undefined in many cases and then we use some almost-arbitrary point (from one of the edges). Theoretically I could use this point to check whether it's hittable on that point, but then it might result in breakage of tests (where this points are hidden but the view is partially visible, like in 03.actions-scroll.test.js).

It might be the best we can offer for this fix, without breaking the current API much. Of course, I'd like to hear more thoughts..

I agree that adding the ≥75 assertion offhand would be reckless, and also will not make sense as long as the percentage cannot be specified by the user.

As for the idea you suggested - checking hittability, I wouldn't get it off the table altogether. But in any case, on Android we essentially do something quite similar to your change with respect to visibility -- asserting the view is non-empty (which means it may actually make sense).

Done ✔️ we will now check hittability instead of visibility.
There's a lot of changes in the new implementation, so this may be effective to review them face to face.

of course, we should also test it with the latest changes as broadly as possible using the internal wix projects. especially when it adds more assertions to touch actions.

asafkorem · 2021-11-30T12:19:41Z

@wix/detox-team I'll appreciate if one of you can review this change :)

detox/ios/Detox/Utilities/UIView+DetoxUtils.m

d4vidi · 2021-12-01T17:40:36Z

detox/test/e2e/35.overlay.test.js

+    it('should be intractable when overlay is not shown', async () => {
+      await verticalScrollView.scrollTo('bottom');
+      await expect(showOverlayButton).not.toBeVisible();
+    });


I think is not necessary because it is tested in the actions suite

Basically you are right but not really, since I'm testing the whole screen, in this test I made sure that we start from the desired initial state (where the button disappears after scrolling). This covers the case where the scroll view might be not-scrollable (for example, too few items on some screen).

Still kind of sounds like this:

Detox/detox/test/e2e/03.actions-scroll.test.js

Line 33 in 2c6478d

it('should scroll to edge', async () => {

I disagree, I'm not testing the functionality of a scroll view, but the expectation from the initial state of this screen. Let's discuss about this f2f?

detox/test/e2e/35.overlay.test.js

d4vidi · 2021-12-01T17:42:10Z

detox/test/ios/example/NativeModule.m

+        overlayWindow = [[UIWindow alloc] initWithFrame:screenBounds];
+
+        [overlayWindow setWindowLevel:UIWindowLevelStatusBar];
+        [overlayWindow setBackgroundColor:[UIColor.grayColor colorWithAlphaComponent:0.5]];


Would it make sense to challenge further and use a 0 alpha?

Holy s**t, that's an excellent question!
If the overlying view has a transparent background (or, clearColor), we consider the underlying view as visible. So in this case the views are considered as "visible" but they are not tappable / scrollable.

So it makes a lot of sense, but the tests will break, but we need to cover these cases also since they shouldn't (touch gestures should not pass).

We must have some better mechanism here than just asserting on visibility.

asafkorem · 2021-12-02T10:32:31Z

@d4vidi fixed and squashed. I rewrote the mechanism of is-hittable check on iOS.

The guiding logic is:

If not point was given, finds a visible point of the view (otherwise the view is not hittable).
Finds the topmost window, tries to tap on the visible point, on the topmost window using hitTest.
Checks whether the hitted view is the view itself / descendent of the tested view.

The problems that were in the previous check (of is-hittable, which also weren't even in use in some actions):

It checked the visibility of the point only, at the given point or on the central point of the visible bounds of the view.
It didn't checked whether it passes the hits to another view (for example, a tappable subview).
It didn't considered transparent views (with clear-color), as overlying views that can block the hit from getting to the target view.

Some tests are still failing: I'm investigating the reasons. Pushed the current changes for visibility, in case you want to start looking.

As a side note, XCUITest already provide API for isHittable, so when we'll use XCUIT we'll get a much more accurate check.

d4vidi · 2021-12-05T17:00:05Z

detox/ios/Detox/Utilities/UIResponder+First.m

+//  Copyright © 2021 Wix. All rights reserved.
+//
+
+#import "UIResponder+First.h"


Ref stackoverflow

d4vidi · 2021-12-05T17:09:46Z

Descriptive error for when default start point is not hittable (i.e. use offset arg + ref the docs)
Add missing assertions in scrolling test with offsets
Add expect-to-fail scroll test -- when overlay is shown, in 03.actions.test.js
UIScreen etc.

asafkorem · 2021-12-06T10:08:07Z

Changes pushed

Descriptive error for when default start point is not hittable (i.e. use offset arg + ref the docs)

Done ✔️

Add missing assertions in scrolling test with offsets

Done ✔️

Add expect-to-fail scroll test -- when overlay is shown, in 03.actions.test.js

Done ✔️

Replaces this error message: `View “<RCTView: 0x7fa3020598b0>” is not visible: View is clipped by one or more of its superviews' bounds and does not pass visibility percent threshold (7,500)`, With this error message: `View “<RCTView: 0x7fa3020598b0>” is not visible: View is clipped by one or more of its superviews' bounds and does not pass visibility percent threshold (75)`.

Failed tests: - `scroll view -> should not be scrollable when overlay is shown`. - `button -> should not be hittable when overlay is shown`.

Rewrite the logic: - Finds a visible point of the view (otherwise the view is not hittable). - Finds the topmost window, tries to tap on the visible point using `hitTest`. - Checks whether the hitted view is the view itself / descendent of the tested view.

asafkorem · 2021-12-06T11:26:56Z

detox/test/e2e/03.actions.test.js

+    if (device.getPlatform() === 'ios') {
+      // TODO: investigate why this assertion fails on Android
+      await expect(element(by.text('Text1'))).not.toBeVisible(1);
+    }


This was already TODOed here:

Detox/detox/test/e2e/03.actions.test.js

Lines 147 to 150 in d95b5e7

if (device.getPlatform() === 'ios') {

// TODO: investigate why this assertion fails on Android

await expect(element(by.text('Text1'))).not.toBeVisible();

}

@jonathanmos / @d4vidi FYI

Actually, it's related to an issue that was reported very recently 😄 , and Amit has reported on that long ago in a RN issue: #3105 (comment), facebook/react-native#23870

…3115)" This reverts commit b146425, reversing changes made to d95b5e7. # Conflicts: # detox/package.json # detox/test/package.json # examples/demo-native-android/package.json # examples/demo-native-ios/package.json # examples/demo-react-native-detox-instruments/package.json # examples/demo-react-native-jest/package.json # examples/demo-react-native/package.json # lerna.json # package.json (Resolved by using the current files)

This change reverts #3150, and adds the following changes (hit-test): - #3149 - #3145 - #3115 Those changes resolves this issue #2834.

asafkorem requested review from d4vidi and noomorph as code owners November 29, 2021 16:36

asafkorem linked an issue Nov 29, 2021 that may be closed by this pull request

Interactive touch outside React-Native-Navigation Overlay when detox run #2834

Closed

1 task

asafkorem mentioned this pull request Nov 29, 2021

Interactive touch outside React-Native-Navigation Overlay when detox run #2834

Closed

1 task

noomorph reviewed Nov 29, 2021

View reviewed changes

asafkorem force-pushed the feature/rnn-overlay branch from 3124c1a to 3ebd8cf Compare November 29, 2021 19:04

asafkorem changed the title ~~fix(scrollTo): raise if element is hidden.~~ fix(scrollTo): raise if scroll view element is not hittable. Nov 29, 2021

asafkorem force-pushed the feature/rnn-overlay branch from 3ebd8cf to 6a5fc38 Compare November 29, 2021 19:46

asafkorem changed the title ~~fix(scrollTo): raise if scroll view element is not hittable.~~ fix(scrollTo): raise if scroll view element is not visible. Nov 29, 2021

asafkorem commented Nov 29, 2021

View reviewed changes

d4vidi reviewed Dec 1, 2021

View reviewed changes

detox/ios/Detox/Utilities/UIView+DetoxUtils.m Show resolved Hide resolved

d4vidi reviewed Dec 1, 2021

View reviewed changes

detox/ios/Detox/Utilities/UIView+DetoxUtils.m Outdated Show resolved Hide resolved

d4vidi reviewed Dec 1, 2021

View reviewed changes

detox/test/e2e/35.overlay.test.js Show resolved Hide resolved

d4vidi reviewed Dec 1, 2021

View reviewed changes

asafkorem force-pushed the feature/rnn-overlay branch from 6a5fc38 to 0f2652e Compare December 2, 2021 10:29

asafkorem force-pushed the feature/rnn-overlay branch 2 times, most recently from 6af218c to 9ce9eae Compare December 4, 2021 10:44

asafkorem changed the title ~~fix(scrollTo): raise if scroll view element is not visible.~~ fix(scrollTo): raise if scroll view element is not hittable. Dec 4, 2021

d4vidi reviewed Dec 5, 2021

View reviewed changes

asafkorem force-pushed the feature/rnn-overlay branch 2 times, most recently from 8d4c744 to e6c4341 Compare December 6, 2021 10:15

asafkorem added 3 commits December 6, 2021 12:39

fix(34.visibility.test.js): await for element matching.

1b3e71c

iOS(Test App): add native method for presenting overlay window.

9b0c874

asafkorem added 13 commits December 6, 2021 12:39

iOS(Test App): write failed detox tests (to reproduce wix#2834).

b78b551

Failed tests: - `scroll view -> should not be scrollable when overlay is shown`. - `button -> should not be hittable when overlay is shown`.

fix(DetoxActions): use is-hittable assertion before UI gestures.

0c65205

fix(isHittable): check if view is hittable from first responder.

5e055c4

UIView+DetoxUtils: remove redundant EL.

1ad2a7a

fix(actions): use the correct coordinates systems.

2063212

fix(DetoxUtils): fix error handling.

41beed4

fix(DetoxUtils): add nullable flag to error's value.

ae83497

UIResponder+First: add reference to the source SO thread.

44bfaa5

UIScrollView+DetoxActions: make informative assertion.

24b9992

03.actions.test.js: add expectations.

adfe875

03.actions-scroll.test.js: add tests for covered scroll view.

ad36fe3

03.actions-scroll.test.js: fix android test fail (due to timeout).

2813ef1

asafkorem force-pushed the feature/rnn-overlay branch from e6c4341 to ac6ccdb Compare December 6, 2021 11:14

asafkorem and others added 2 commits December 6, 2021 13:19

03.actions.test.js: expect only on iOS.

7d12668

Publish 19.3.0-smoke.0 [ci skip]

1d67fe5

asafkorem force-pushed the feature/rnn-overlay branch from ac6ccdb to 1d67fe5 Compare December 6, 2021 11:19

asafkorem commented Dec 6, 2021

View reviewed changes

asafkorem added 2 commits December 6, 2021 13:34

03.actions.test.js: replace TODOs with reference to issue.

281cd54

fix(33.attributes.test.js): expect both subviews to be hittable.

43e7ee6

asafkorem merged commit b146425 into wix:master Dec 6, 2021

asafkorem deleted the feature/rnn-overlay branch December 6, 2021 16:12

asafkorem mentioned this pull request Dec 27, 2021

Revert: is-hittable changes. #3150

Merged

asafkorem mentioned this pull request Oct 2, 2022

fix(iOS): revert^2 hittability-test mechanism fixes. #3620

Merged

asafkorem added a commit that referenced this pull request Nov 7, 2022

fix(iOS): improve hittability-check mechanism. (#3620)

e1f66d1

This change reverts #3150, and adds the following changes (hit-test): - #3149 - #3145 - #3115 Those changes resolves this issue #2834.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(scrollTo): raise if scroll view element is not hittable. #3115

fix(scrollTo): raise if scroll view element is not hittable. #3115

asafkorem commented Nov 29, 2021 •

edited

Loading

noomorph Nov 29, 2021

noomorph commented Nov 29, 2021

asafkorem Nov 29, 2021

asafkorem Nov 29, 2021 •

edited

Loading

d4vidi Dec 1, 2021 •

edited

Loading

asafkorem Dec 4, 2021

asafkorem Dec 4, 2021

asafkorem commented Nov 30, 2021

d4vidi Dec 1, 2021

asafkorem Dec 1, 2021 •

edited

Loading

d4vidi Dec 2, 2021

asafkorem Dec 2, 2021

d4vidi Dec 1, 2021

asafkorem Dec 1, 2021

asafkorem commented Dec 2, 2021 •

edited

Loading

d4vidi Dec 5, 2021

asafkorem Dec 6, 2021

d4vidi commented Dec 5, 2021

asafkorem commented Dec 6, 2021

asafkorem Dec 6, 2021

asafkorem Dec 6, 2021 •

edited

Loading

		@@ -0,0 +1,46 @@
		const { expectToThrow } = require('./utils/custom-expects');

		describe(':ios: Overlay', () => {

	if (device.getPlatform() === 'ios') {
	// TODO: investigate why this assertion fails on Android
	await expect(element(by.text('Text1'))).not.toBeVisible();
	}

fix(scrollTo): raise if scroll view element is not hittable. #3115

fix(scrollTo): raise if scroll view element is not hittable. #3115

Conversation

asafkorem commented Nov 29, 2021 • edited Loading

Description

Choose a reason for hiding this comment

noomorph commented Nov 29, 2021

Choose a reason for hiding this comment

asafkorem Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

d4vidi Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asafkorem commented Nov 30, 2021

Choose a reason for hiding this comment

asafkorem Dec 1, 2021 • 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

Choose a reason for hiding this comment

asafkorem commented Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4vidi commented Dec 5, 2021

asafkorem commented Dec 6, 2021

Choose a reason for hiding this comment

asafkorem Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

asafkorem commented Nov 29, 2021 •

edited

Loading

asafkorem Nov 29, 2021 •

edited

Loading

d4vidi Dec 1, 2021 •

edited

Loading

asafkorem Dec 1, 2021 •

edited

Loading

asafkorem commented Dec 2, 2021 •

edited

Loading

asafkorem Dec 6, 2021 •

edited

Loading