Skip to content

Querying by role with name on nested touchables return multiple elements #1152

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.

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

Closed
AugustinLF opened this issue Sep 29, 2022 · 5 comments
Closed
Labels

Comments

@AugustinLF
Copy link
Collaborator

Describe the bug

We have a problem when querying nested touchables, see the example for more details:

describe('nested touchables', () => {
  it('conflicting elements', () => {
    const { getByRole } = render(
      <Pressable accessibilityRole="button">
        <Text>Some other text</Text>
        <Pressable accessibilityRole="button">
          <Text>Save</Text>
        </Pressable>
      </Pressable>
    );

    // Found multiple elements with accessibilityRole: button
    // Here I'm not sure what we should do. This is not an accessible pattern.
    // At least we should suggest a better error (might be just a warning/error on nested touchables)
    expect(getByRole('button', { name: 'Save' })).toBeTruthy();
  });

  it('with accessible={false}', () => {
    const { getByRole } = render(
      <Pressable accessibilityRole="button" accessible={false}>
        <Text>Some other text</Text>
        <Pressable accessibilityRole="button">
          <Text>Save</Text>
        </Pressable>
      </Pressable>
    );

    // Found multiple elements with accessibilityRole: button
    // Here it should work and only getting the lower Pressable.
    // `accessible={false}` means it won't be read to the screen-reader.
    // This is a common pattern I've seen where you make a whole area implicitly touchable for better UX
    // but add a nested explicit button for better discoverability (and accessibility)
    expect(getByRole('button', { name: 'Save' })).toBeTruthy();
  });
});

Versions

RNTL 11.2

@AugustinLF AugustinLF added the bug Something isn't working label Sep 29, 2022
@mdjastrzebski
Copy link
Member

I think that this might actually be a more general aspect of out tree walking code that uses findAll from RTR.

I imagine that it also should manifest itself for any scenario with matching nested elements, like: *ByLabel with following setup:

<View accessiblityLabel="hello">
  <View accessiblityLabel="hello">
    <Text>Hello</Text>
  </View>
</View>

or *ByA11yStates with following setup:

<View accessiblityStates={{ disabled: true }}>
  <View accessiblityStates={{ disabled: true }}>
    <Text>Hello</Text>
  </View>
</View>

Essentially for any case when component structure allows for nesting elements matching query predicate, it currently will result in matching both the inner and outer one. This might be more visible in certain scenarios like *ByRole queries, but probably affects all nesting scenarios.

It seems that RTL/DTL should behave the same as our code because their queries rely on using querySelectorAll which seems to have the same behavior of returning both outer and inner for nested elements.

I think we should first investigate how RTL would behave in a similar case, and then decide if this is a bug or a feature.
@AugustinLF wdyt?

@AugustinLF
Copy link
Collaborator Author

Overall I agree, that sounds fair and we should probably use the same API/behaviour. I do think the accessible={true} might be a special case, even though ideally the developer would use an onPress with a role on the parent element.

@AugustinLF AugustinLF removed the bug Something isn't working label Sep 29, 2022
@mdjastrzebski
Copy link
Member

Regarding the original tests mentioned in this issue, we should consider excluding elements with explicit accessible={false} prop from *ByRole query, since these are explicitly marked as not being accessibility elements, and afaiu should not have a role.

@AugustinLF wdyt?

@AugustinLF
Copy link
Collaborator Author

Yes, I think that'd be a great start!

@mdjastrzebski
Copy link
Member

Closing in favour of #1180 with more clear scope.

@mdjastrzebski mdjastrzebski closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants