Non-visible modal content shows up in queries

[kirbycool]

Describe the bug

Hi, thanks for taking a look at this!
I'm fairly new to react-native, but have a lot of experience with web testing-library variants and ran across some unexpected (to me) behavior. Query methods find nodes in non-visible modals.

I think this is what this user was getting at #508

Steps to Reproduce

Here's a simple example test I set up to test this out

// App.js
import React from "react";
import { Modal, Text, View } from "react-native";

const App = () => {
  return (
    <View>
      <Modal visible={false}>
        <View>
          <Text>Exciting content</Text>
        </View>
      </Modal>
    </View>
  );
};

export default App;

// App.test.js
import React from "react";
import { render, fireEvent } from "@testing-library/react-native";
import App from "./App";

it("shows the modal on button click", () => {
  const { queryByText } = render(<App />);

  expect(queryByText("Exciting content")).toBeNull(); // fails
});

Expected behavior

I'd expect this test to pass, since there's no modal content visible to the user while the modal is not visible.

If this is expected behavior, is there a recommended way to test appearance/disappearance modals?

Versions

@testing-library/react-native@7.1.0

[MattAgn]

I think it's due to the fact that the getBy and queryBy queries parse the DOM itself. The modal is always in the DOM, its appearance to the user is dictated by its visible prop, which the queries like getBy don't take into account.

Personally what I usually tend to do in those cases is check the visible prop on the modal.

I think a better solution might be to abstract this hack (checking the visible prop) in an extension of jest expect method, that could be put in the jest native repo for example.

[kirbycool]

Yeah that makes sense. Thanks for taking a look.

Testing the visible prop isn't ideal since it's an escape hatch from the "test from the user perspective" philosophy. Ideally we'd be able to query for "is this content on the screen", which is how the user sees it and makes the test more robust in case the content is refactored outside of a modal later on.

I suppose this is more of a feature request given the context (and would probably be a breaking change). Maybe this will bug me enough to try implementing it at some point 😄

[MattAgn]

Totally understand your point of view. Unfortunately I think it's deeply tied to the modal implementation with the visible prop. In my mind, changing the getByText query to take into account the modal would imply checking all the parents of the node until you find (or not) a modal right?
I wonder if it could have a bad performance hit especially on large components, what do you think?

[thymikee]

There's a possibility that Modal sets some accessibility props for visibility. Our queries should take those into account. It's quite irritating when testing React Navigation, which keeps previous screens mounted, but disabled for accessibility.

[MattAgn]

@thymikee I did not know that could happen while testing navigation, I can see how that could be a problem. Do you want to have a fix that would check both React native modals as well as previously mounted react navigation pages?
I can try to work on it if you want :)

[thymikee]

If this can be done solely with accessibility props available on these components, then I'm all for it!

[joe307bad]

I think I ran into this same issue and pointed it out here.

It seems react-navigation likes to keep components mounted, including a custom header component. Because of this, I can't figure out a way to test using the back button or the menu toggle button in my custom header component since when I query for the back button, it brings back multiple elements. So it's similar to this issue because @testing-library/react-native is bringing back non visible items in the queries.

[MattAgn]

@thymikee you were right for react navigation, it does use accessibility props for its screen. It uses accessibilityElementsHidden for ios and importantForAccessibility for android. I think we could use either of them in our use case but importantForAccessibility="no-hide-descendants" might be the best since it hides all children from accessibility. I wonder about one thing though, could it be possible that someone uses theses props to ignore accessibility tools like VoiceOver for whatever reasons but still wants to show his component? In which case he would want his tests to find his component right?

For the modal from react-native, there is no accessibility prop according to the props given in the doc. Its prop visible sounds like the only option unfortunately.

[thymikee]

I'd be in favor of such a behavior as a default one. If someone needs to access hidden element, they could do it using the within and getById helpers, couldn't they? It would be on us to document such a scenario

[satya164]

For the modal from react-native, there is no accessibility prop according to the props given in the doc. Its prop visible sounds like the only option unfortunately.

The other option is to send a PR to React Native adding accessibility props for Modal.

[joe307bad]

I just wanted to confirm if possible that the issue I mentioned here is related to this issue. It seems that it is not only the modal that gets picked up in queries, but any non-visible component. How do people get around this when using react-navigation? Seems like this makes these two libraries (react-navigation and react-native-testing-library) at odds with each other.

[MattAgn]

Yes it seems related to your issue @joe307bad. To fix it, we were discussing if we could base queries like getBytext on accessibility props so that it would ignore any non visible element.

@thymikee here are the 3 possible solutions I see for getByText for instance. GetByText uses ReactTestInstance.findAll to search through all the nodes so:

• we could implement a new findAll method that would by default not go deeper in the tree as soon as it sees a non visible component (based on accessibility props)
  • pro: the new findAll stays in the repo, easier to modify
  • con: need to reimplement findAll
• we could open an issue and a PR to add this behaviour to the original ReactTestInstance.findAll
  • pro: no need to rewrite findAll
  • con: longer to be implemented
• we could let findAll as is, and once we find a text node matching the query, we could check all his parents and check their accessibility props to make sure the component is indeed visible
  • con: not great for performance I guess, we'll be going down the tree and then up again to check the accessibility props on the parents

Do you see other options? Which one do you prefer? (I think the simplest one would be the first)

[joe307bad]

@MattAgn, I would love to try my hand at implementing the first solution. Could you point me in the direction of the findAll method that would need to be reimplemented?

[gabrielgrover]

any progress on this? Or at least a decision on which option to take from @MattAgn's list above?

I have been digging into the codebase a bit. Not sure how the first option in @MattAgn 's list would be implemented. I might be misunderstanding how the dependency injection is being accomplished but it seems the findAll method that was mentioned is this one in src/helpers/makeQueries.js

function findAllByQuery(instance: ReactTestInstance) {
    return function findAllFn(
      args: QueryArg,
      waitForOptions?: WaitForOptions = {}
    ) {
      return waitFor(() => getAllByQuery(instance)(args), waitForOptions);
    };
 }

The function getAllByQuery is a function that calls a function that is passed by implementations such as src/helpers/byPlaceholderText.js or src/helpers/byText.js . Wouldn't each of these implementations need to be modified in order to accomplish the first option since that is where the render tree traversal is being done? Let me know if I am completely missing the mark here.