Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix(Android): Fixed issues with toBeVisible() #4519

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Fix(Android): Fixed issues with toBeVisible() #4519

merged 7 commits into from
Jul 8, 2024

Conversation

gosha212
Copy link
Contributor

Description

In this pull request, I have created a workaround for View.getGlobalVisibleRect issue in RN

For features/enhancements:

  • I have added/updated the relevant references in the documentation files.

For API changes:

  • I have made the necessary changes in the types index file.

@gosha212 gosha212 linked an issue Jun 30, 2024 that may be closed by this pull request
[Android] Scrollview item wrongly considered visible #4500
Closed
2 tasks
@d4vidi d4vidi self-assigned this Jul 1, 2024
asafkorem
asafkorem requested changes Jul 7, 2024
View reviewed changes
Copy link
Contributor

@asafkorem asafkorem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so happy that we finally addressed this (even though it's RN's broken logic, at least Detox is able to handle it now).. 💪🏼

I have a few minor cosmetic suggestions, but overall, this fix is quite straightforward and great!

detox/android/detox/src/full/java/com/wix/detox/espresso/DetoxMatcher.java Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetox.kt Outdated Show resolved Hide resolved
detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/ViewMatchers.kt Outdated Show resolved Hide resolved
detox/test/e2e/03.actions-scroll.test.js Show resolved Hide resolved
detox/test/e2e/03.actions-scroll.test.js
Comment on lines -117 to -132
it(':android: should be able to scrollToIndex on vertical scrollviews', async () => {
// should ignore out of bounds children
await element(by.id('ScrollView161')).scrollToIndex(3000);
await element(by.id('ScrollView161')).scrollToIndex(-1);
await expect(element(by.text('Text1'))).toBeVisible();

await element(by.id('ScrollView161')).scrollToIndex(11);
await expect(element(by.text('Text12'))).toBeVisible();

await element(by.id('ScrollView161')).scrollToIndex(0);
await expect(element(by.text('Text1'))).toBeVisible();

await element(by.id('ScrollView161')).scrollToIndex(7);
await expect(element(by.text('Text8'))).toBeVisible();
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It never worked. I'm deprecating this api

Copy link
Contributor

@asafkorem asafkorem Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecating or removing?
WDYM never worked? We have the test right here 👆🏼

Is it broken now because of this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also if we're deprecating let's mark it with @deprecated

Copy link
Contributor

@asafkorem asafkorem Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed F2F. This API is really not working. The test was a false positive 🤦🏼‍♂️
Let's @deprecate and remove it on next major

@gosha212 gosha212 enabled auto-merge July 8, 2024 08:41
asafkorem
asafkorem approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@asafkorem asafkorem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff :)

@gosha212 gosha212 merged commit 497a8f6 into master Jul 8, 2024
3 checks passed
@asafkorem asafkorem deleted the 4500-android-scrollview-item-wrongly-considered-visible branch July 14, 2024 14:52
kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this pull request Jul 26, 2024
@kirillzyusko
e2e: decrease detox version, since it looks like wix/Detox#4519 intro…
d0c5de7 
…duced a regression
@kirillzyusko
Copy link
Contributor

Hey @gosha212

I believe it caused my tests in keyboard-controller to fail 🙈

I've tried to update detox to the latest version, but I think this PR caused one of my test to fail constantly. I'm getting:

 Waiting for element by id: autofill_contacts
  origin: at kit/helpers/awaitable/index.ts:11:13
16:55:36.704 detox[87902] i child-process:EXEC_CMD "/usr/local/lib/android/sdk/platform-tools/adb" -s emulator-5554 shell du /sdcard/165425470_3.mp4
16:55:36.782 detox[87902] i child-process:EXEC_CMD "/usr/local/lib/android/sdk/platform-tools/adb" -s emulator-5554 shell "rm  \"/sdcard/165425470_3.mp4\""
16:55:46.622 detox[87902] i ws-client:APP_STATUS The app seems to be idle
16:55:56.627 detox[87902] i ws-client:APP_STATUS The app seems to be idle
16:56:06.631 detox[87902] i ws-client:APP_STATUS The app seems to be idle
16:56:06.650 detox[87902] i child-process:EXEC_CMD "/usr/local/lib/android/sdk/platform-tools/adb" -s emulator-5554 shell "screencap /sdcard/165425470_6.png"
16:56:06.907 detox[87902] i child-process:EXEC_CMD "/usr/local/lib/android/sdk/platform-tools/adb" -s emulator-5554 shell "screencap /sdcard/165425470_7.png"
16:56:06.914 detox[87902] i child-process:SPAWN_END /usr/local/lib/android/sdk/platform-tools/adb -s emulator-5554 shell screenrecord /sdcard/165425470_5.mp4 terminated with SIGINT
16:56:07.105 detox[87902] i lifecycle `KeyboardToolbar` specification: should show bottom sheet when `AutoFill Contacts` is pressed [FAIL]
16:56:07.106 detox[87902] i lifecycle `KeyboardToolbar` specification: should set focus back when modal closed

Let me know which additional information you need to reproduce/fix this problem - I'll be happy to co-operate here!

Reproduction can be found at https://github.com/kirillzyusko/react-native-keyboard-controller - you just need to use latest version in e2e folder.

@gosha212
Copy link
Contributor Author

Hi @kirillzyusko
I've started the investigation. I will keep you updated. Thanks for the info

@kirillzyusko
Copy link
Contributor

Thank you for the update @gosha212!

@gosha212
Copy link
Contributor Author

Can you please point me to the failing test and how to reproduce it? Which command should I run?
I run test-example:android-31 and some of the tests are failing on the master on my machine

@gosha212
Copy link
Contributor Author

@kirillzyusko is it easy to reproduce the problem on a side project? It will be easy to work in isolated environment

@kirillzyusko
Copy link
Contributor

@gosha212 I found you on Discord - let's keep conversation there? I believe it'll be more productive 🤞

@pwltr pwltr mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asafkorem asafkorem asafkorem approved these changes

@d4vidi d4vidi Awaiting requested review from d4vidi d4vidi is a code owner

@noomorph noomorph Awaiting requested review from noomorph noomorph is a code owner

Assignees

@d4vidi d4vidi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] Scrollview item wrongly considered visible
4 participants
@gosha212 @kirillzyusko @asafkorem @d4vidi