Skip to content

Conversation

mateoguzmana
Copy link
Collaborator

@mateoguzmana mateoguzmana commented Dec 13, 2024

Summary:

Fixes #46180

This PR fixes the testID not being set as a resource-id in the HorizontalScrollView.

Currently the resource-id is being set correctly when we use a vertical scroll view (this is done here for reference) but we still miss setting this when we use a horizontal one as the managers for both components are different.

Changelog:

[ANDROID][FIXED] - Handling testID correctly for horizontal scroll view

Test Plan:

Render a simple ScrollView component with horizontal set as true and pass a testID property as shown:

function Playground() {
  return (
    <ScrollView testID="customScrollViewTestId" horizontal>
      <View
        style={{
          marginVertical: 400,
          backgroundColor: 'white',
          padding: 14,
          margin: 50,
          width: 200,
          height: 200,
        }}
      />
    </ScrollView>
  );
}

Open Maestro Studio and search for customScrollViewTestId in the search bar.

Before the fix: The `testID` is not found. (See screenshot)

image

See the same in Appium. (See screenshot) image

Apply this fix, and search again in Maestro Studio.

After the fix: The `testID` is now recognised and can be found in the search bar. (See screenshot)

image

See the same in Appium. (See screenshot) image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 13, 2024
@javache
Copy link
Member

javache commented Dec 13, 2024

ReactScrollViewManager derives from BaseViewManager which already overrides setTestId. Could you please debug why that method isn't getting invoked?

@mateoguzmana mateoguzmana marked this pull request as draft December 13, 2024 14:49
…iew and reverting the test id prop setter overwrite
@mateoguzmana mateoguzmana changed the title [Android] ScrollView: support testID property [Android] ScrollView: handling testID correctly for horizontal scroll view Dec 13, 2024
@mateoguzmana
Copy link
Collaborator Author

ReactScrollViewManager derives from BaseViewManager which already overrides setTestId. Could you please debug why that method isn't getting invoked?

@javache thanks to your observation, I went back and checked the full flow and actually the methods were called correctly. When doing my fix I did some clean up in the repro example and actually made me think it got fixed (my mistake during testing 😄)

With the above said, I narrowed down the issue and discovered it only happens when we use a horizontal scroll view. For the vertical one it was working already correctly.

I've updated the PR info as well to reflect this, used also Appium to double check it's not a problem of a single e2e tool.

Copy link
Member

@javache javache left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for digging in!

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 13, 2024
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in 81c74cd.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @mateoguzmana in 81c74cd

When will my fix make it into a release? | How to file a pick request?

facebook-github-bot pushed a commit that referenced this pull request Dec 19, 2024
Summary:
Follow up from #48271 and #48254, I noticed that the Modal component also doesn't map the `resource-id` from the `testID` on Android. This PR addresses that.

## Changelog:

[ANDROID] [FIXED] - Modal: Setting `resource-id` from `testID` prop

Pull Request resolved: #48313

Test Plan:
Alternatively do:
```
$ adb shell uiautomator dump
UI hierchary dumped to: /sdcard/window_dump.xml
$ adb pull /sdcard/window_dump.xml
/sdcard/window_dump.xml: 1 file pulled, 0 skipped. 1.1 MB/s (3505 bytes in 0.003s)
```
and check in XML: ` resource-id="playground-modal"  class="android.view.ViewGroup" `
-------
Using Appium, check that the `testID` prop passed from JS is mapped as `resource-id` in the rendered view group of the Modal.

<details>
<summary>Example of the code implementation in the RNTester Playground:</summary>

```tsx
function Playground() {
  const [modalVisible, setModalVisible] = React.useState(false);

  return (
    <>
      <Modal
        visible={modalVisible}
        testID="playground-modal">
        <Text testID="inner-text-test-id">Hello World!</Text>
      </Modal>

      <Button
        title="Open Modal"
        onPress={() => {
          setModalVisible(true);
        }}
      />
    </>
  );
}
```
</details>

<details>
<summary>Output in Appium Inspector:</summary>

<img width="913" alt="image" src="https://github.com/user-attachments/assets/514ae2b3-35a8-4a1a-8efc-1ca6bd73f189" />

</details>

Reviewed By: javache

Differential Revision: D67369350

Pulled By: alanleedev

fbshipit-source-id: a799ad5b974895a39d9287e3d76d1139a6ef6a83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Android] ScrollView does not properly handle testID property

4 participants