fix: *ByRole queries to match their own text text content

[AugustinLF]

Summary

The following test case doesn't work with our new getByRole(role, {name}) API,

 test('returns an element when the direct child is text', () => {
    const { getByRole } = render(
      <Text accessibilityRole="header" testID="target-header">
        About
      </Text>
    );

    // assert on the testId to be sure that the returned element is the one with the accessibilityRole
    expect(getByRole('header', { name: 'About' }).props.testID).toBe(
      'target-header'
    );
  });

Would probably warrant a patch to 11.2.

Closes #1140

[mdjastrzebski]

If I understand correctly, the root cause of the issue is that when inside queryAllByRole we process only host nodes, because part of your filter is typeof node.type === 'string'. However getByText is looking for composite Text exported from RN, which is actually a parent of our host Text.

This problem is not a general *ByText problem, as in regular cases there always are both composite and host Text pair. But in our case we start with queries bound to the host Text.

As @thymikee matching by instance.type === 'Text' is fragile, as different versions of RN used different host component name. This seems fair as the exact name of host element is an implementation detail, and we want to avoid relying on these if possible.

Perhaps a more robust workaround, but still a workaround, would be to pass node.parent to queryAllByText in matchAccessibleNameIfNeeded. Like this:

const matchAccessibleNameIfNeeded = (
  node: ReactTestInstance,
  name?: TextMatch
) => {
  if (name == null) return true;

  // Note: due to queryAllByText matching composite `Text` element, we have to start at parent level
  const nodeParentQueries = getQueriesForElement(node.parent);
  const matchesText = queryAllByText(name).length > 0;

  const nodeQueries = getQueriesForElement(node);
  const matchesLabel = queryAllByLabelText(name).length > 0;
 
  return matchesText || matchesLabel;
};

This is also a bit fragile as it assumes that composite Text exported from RN will be direct parent of host Text.

@thymikee, @AugustinLF wdyt?

[AugustinLF]

The proposition to switch to our own internal model that has been discussed a few times already (see #967) would fix this host vs composite component, right? If that's the case, should we then consider a "hack" to solve the problem at hand, while we look into a more long term solution as part of the rewrite?

Tested your implementation @mdjastrzebski which works (with a slight fix to make it work with TS)

const matchAccessibleNameIfNeeded = (
  node: ReactTestInstance,
  name?: TextMatch
) => {
  if (name == null) return true;

  // Note: due to queryAllByText matching composite `Text` element, we have to start at parent level
  const nodeParentQueries = getQueriesForElement(node.parent ?? node);
  const matchesText = nodeParentQueries.queryAllByText(name).length > 0;

  const nodeQueries = getQueriesForElement(node);
  const matchesLabel = nodeQueries.queryAllByLabelText(name).length > 0;

  return matchesText || matchesLabel;
};

We could also consider climbing up the parent tree util we find a host component, which would make it less brittle for future changes.

As discussed this is not ideal because it might pose a problem with future RN versions, but given that we now have support for running our test suite against multiple versions, if this ends up changing, it should be fairly easy for us to make sure RNTL works with all the versions we support.

[mdjastrzebski]

@AugustinLF I think we could tweak the solution a bit, by climbing up, but only through composite components to the exported Text component, since we know that we are doing that for *ByText query. That would make our workaround a bit more robust as now Text could render some intermediate composite components between exported component and final host component.

I think the proper way to add this, is to make queryAllByText have a special check, that would allow it to work when bound to host component instead of adding this to queryAllByRole. So essentially we want following test to pass:

const view = render(<Text testID="subject">Hello</Text>)`
const hostTextQueries = getQueriesForElement(view.getByTestId("subject"));
expect(hostTextQueries.getByText("Hello")).toBeTruthy();

We could build some helper to check for being host Text:

function isHostTextElement(element: ReactTestInstance) {
  // Not a host element
  if (typeof element.type !== "string") return false;
  
  let current = element.parent;
  while(typeof current.type !== 'string') {
    if (current.type === RN.Text) { 
      return true;
    }
    current = current.parent;
  }
  
  return false;
}

or similar logic in form of a getter

function getExportedTextForHostElement(element: ReactTestInstance) {
  // Not a host element
  if (typeof element.type !== "string") return null;
  
  let current = element.parent;
  while(typeof current.type !== 'string') {
    if (current.type === RN.Text) { 
      return current;
    }
    current = current.parent;
  }
  
  return null;
}

We should be wary not to climb too high the component tree, and then make down queries, as otherwise we might actually catch Text in the siblings on component with role which would be an implementation error. We might want to add some test to cover for that case.

[mdjastrzebski]

Good work @AugustinLF That PR looks pretty solid. I've found some minor things to address but look forward to having fully functional getByRole!

[mdjastrzebski]

@AugustinLF could you refactor the code to use regular Text/TestInput import from React Native instead of current dynamic one. This should simplify the codebase.

[mdjastrzebski]

@AugustinLF Looks good! Recommended some final tweaks to the tests.

[mdjastrzebski]

Awesome work on these difficult subjects @AugustinLF !

I guess we have all learned a lot about accessibility and host/composite elements tree 😆