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.

[AntoineDoubovetzky]

I just opened a PR on react-native repository to update the mock so the Modal doesn't render its children when the visible prop is false. It should fix your issue :)

[xThuby]

This ever get fixed?

[AntoineDoubovetzky]

@OliverAThunes It should be fixed in RN 0.67.4

[therynamo]

I'm currently facing the issue described in this comment.

We're working on something similar to "Flows" outlined in this article - where we can test our app and its user journeys to add confidence/reliability to each PR before it is merged.

TL;DR -> Entire app testing with Navigation (mocking network requests using MSW). While Detox exists, it has limitations that prevent it from running on every PR - mainly the compile times per platform. We're willing to accept that trade off of "native env" for the ability to write comprehensive tests for the JS flows of our app.

---

While this is exciting:

@OliverAThunes It should be fixed in RN 0.67.4

I'm curious if there is any progress towards enhancing the API like in #787 .

The modal not showing children is nice - but the problem still exists where the entire stack is rendered behind the "current active screen". This results in querying returning multiple results when there should be only one (as described in the issue).

At the moment I'm trying to work around this somehow. The current train of thought is to figure out how to dynamically add a testID prop to the visible screen on the stack, and not have to modify every screen manually in our app to have some ref check and update the id based on currentScreen === <screenName>. This way we could target the "current screen" with within and "regular" queries for assertions (something like findByTestId('activeScreen')). Is there any way to target the "current active screen" with a given RNTL query?

Even that idea though has its limitations. Navigating using Back is difficult and error prone as there are n number of Back buttons on the stack so you have to query for all of them and assume the last one is the one you want so you can "go back".

I can't imagine the React Navigation implementation would change to support this limitation. So I'm wondering if the queries could ignore screens not visible to the user while still being able to navigate to screens behind the current one if necessary?

[mdjastrzebski]

Closing as superseded by #970, which should provide a solution to the more generalised issues that the hidden screens content, e.g. when using stack navigator from React Navigation, is queried by RNTL by default.

As a partial solution/workaround you can within to scope queries to given active screen. You can mark your screens e.g. with testID attribute and use getByTestId query in a following manner:

const detailsScreen = within(screen.getByTestId('screen-details'));

// Now you can issue queries limited to elements under 'detailsScreens'
expect(detailsScreen.getByText(...)).toBeTruthy();

[cfradella]

I'm still running into trouble with this. This is using @mdjastrzebski 's solution:

    const animation = within(screen.getByTestId("particleAnimation"));

    expect(animation).toBeTruthy();

[mdjastrzebski]

@cfradella If you need to match a hidden element (your modal considered hidden as it has importantForAccessiblity="no-hide-descendents" prop), you need to pass includeHiddenElements: true option to your query:

screen.getByTestId("particleAnimation", { includeHiddenElements: true })

Docs link: https://callstack.github.io/react-native-testing-library/docs/api-queries#includehiddenelements-option

Note: the solution you mentioned above is stale and referred to pre v12 versions of RNTL.

Additionally, we no longer recommend using toBeTruthy() to check for elements existance. The recommended version would be:

expect(screen.getByTestId("particleAnimation", { includeHiddenElements: true })).toBeOnTheScreen()

[cfradella]

@cfradella If you need to match a hidden element (your modal considered hidden as it has importantForAccessiblity="no-hide-descendents" prop), you need to pass includeHiddenElements: true option to your query:

screen.getByTestId("particleAnimation", { includeHiddenElements: true })

Docs link: https://callstack.github.io/react-native-testing-library/docs/api-queries#includehiddenelements-option

Note: the solution you mentioned above is stale and referred to pre v12 versions of RNTL.

Additionally, we no longer recommend using toBeTruthy() to check for elements existance. The recommended version would be:

expect(screen.getByTestId("particleAnimation", { includeHiddenElements: true })).toBeOnTheScreen()

That worked, thanks for saving my day. : 👍