Add option to queries to respect accessibility

[MattAgn]

Summary

This PR aims to solve this issue. It adds a "respectAccessibility" option to all queries (false by default) that enables queries to ignore elements that would be hidden in an environment respecting accessibility props

TODO

• [x] Code and test a function isReactElementVisibleToAccessibility that does all the checks
• [x] rename tests titles properly in accessibility.test.ts
• [x] rename the option hidden to comply to RTL naming
• [ ] add the a11y check in all queries
• [ ] add a test for each query
• [ ] add documentation

Test plan

• The function isReactElementVisibleToAccessibility is 100% tested
• The queries all have one test to check the use of the option respectAccessibility

Next steps

• Make another implementation of this feature by implementing a new findAll that would do the a11y check top down instead of bottom up as we do today
• Make performance tests to choose the best implementation

[mdjastrzebski]

I wonder about naming our option. RTL's getByRole has hidden option that kind matches what we are doing here: https://testing-library.com/docs/queries/byrole#hidden

@MattAgn @thymikee @AugustinLF wdyt about proper name for this new option?

[MattAgn]

Wow so quick @mdjastrzebski 😮

[MattAgn]

@mdjastrzebski thanks a lot for the quick review! I'm far from done though, the work I still have to do:

• add the a11y check in all queries
• add a test for each query
• rename tests properly in accessibility.test.ts

[AugustinLF]

I like to have an API as close as possible from rtl web, so using hidden sounds like a great solution. Since their API lets user should the default value of hidden, we could release that in a minor with a default value of true, and turn it on by default in a major release (which is IMO better, and is also the default of RTL).

[mdjastrzebski]

re default value: that would be the plan to have a minor with default behaving as is, and then in the next major release swap the switch to more reasonable value of hidden: false

[MattAgn]

I like to have an API as close as possible from rtl web, so using hidden sounds like a great solution. Since their API lets user should the default value of hidden, we could release that in a minor with a default value of true, and turn it on by default in a major release (which is IMO better, and is also the default of RTL).

Yes sounds good! @mdjastrzebski @AugustinLF What do you think of the clarity of the naming hidden though? I don't find it very explicit about its purpose

[mdjastrzebski]

@MattAgn I'm on a fence here, on one hand it mimics RTL convention but on the other hand it's not very self explanatory. However I wonder if the other name (respectAccessiblity) is explanatory as well.

Seems like this is a concept that is less needed in RTL due to how web navigation works vs RN.

I think it will make more sense if/when we switch this feature on by default in a next major release, which btw will probably generate a lot of user commencts due to failing tests ;-)

[MattAgn]

@mdjastrzebski I agree that respectAccessibility was not so clear either, especially since we also check styles, maybe we should split it into 2 options respectA11yVisbility & respectStylingVisibility? A possible way to do it would be to open an issue on RTL web to discuss the issue with its maintainers?

I think it will make more sense if/when we switch this feature on by default in a next major release, which btw will probably generate a lot of user commencts due to failing tests ;-)

Totally, a migration guide will be required 😄 I did not understand what you mean by "it will make more sense when we switch", what will make more sense? The naming we use?

[mdjastrzebski]

@MattAgn I think that 'hidden' option name will make more sense when we will ignore hidden elements by default.

[MattAgn]

@mdjastrzebski oh alright I understand!

[MattAgn]

@mdjastrzebski I found 2 different ways to handle the a11y option and I can't choose between the 2:

1. Handle the option in each query independently, tried in this commit. Maybe I could shorten it a bit by extracting some logic a helper but we would still have a lot of duplication

   • Pros: handling the filtering of the results based on the options seems
   • Cons: lots of code duplication, a bit harder to maintain

2. Handle the option in makeQueries, in this commit

   • Pros: option handled in one place, no code duplication, easier to maintain. It also make sure all the queries have the a11y option automatically (I guess that's something but not 100% sure)
   • Cons: add a new responsibility to makeQueries, originally, its only responsibility was to create the queries, now it's also doing some logic. Might not be super intuitive for someone looking into the codebase for the first time

Imo, the first option seems the best, because the responsibility argument is the most important to me What do you think?

[mdjastrzebski]

Since hidden/respectAccessiblity would be an option for query not for render then each query should check for that option and use either regualar findAll or our accessiblity-respecting findAll. Sind makeQueries relies on feeding it with queryAll variant then it probably not the place we want to make the switch between findAll variants. That lead us to picking findAll variant separately in each of the base query functions, e.g. https://github.com/callstack/react-native-testing-library/blob/f5db461f806ef222ac6a21a91839556e415607a3/src/queries/text.ts#L87

[mdjastrzebski]

@MattAgn not sure but your first example looks a bit too complex. Seems like you could just declare some common findAll function:

function findAll(instance: ReactTestInstance, predicate: (instance: ReactTestInstance) => boolean, hidden: boolean) {
  // Use regular findAll fucntions
  if (hidden) return instance.findAll(predicate) 

  //else use new accessibility respecting algorithm,
}

Then inside each query, e.g. queryAllByText replace:

  function queryAllByTextFn(text, options) {
    const results = instance.findAll((node) =>
      getNodeByText(node, text, options)
    );

    return results;
  };

with

  // our new `findAll`
  function queryAllByTextFn(text, options) {
    const results = findAll(instance, (node) =>
      getNodeByText(node, text, options), options.hidden ?? true
    );

    return results;
  };

[mdjastrzebski]

@MattAgn I've added isInaccessible function from DTL to RNTL. In DTL it is used in implementation of hidden option. https://github.com/testing-library/dom-testing-library/blob/a9a8cf26992ff0f6b4257b7300939f461d04440d/src/queries/role.js#L193

Would you have time to resume working on this PR? Having the isInaccessible helper this should make the remaining work somewhat easier.

[MattAgn]

@mdjastrzebski I was away for a while, i'm ready to start working back on this 🚀 thanks for the isInaccessible function! Indeed should make it easier :)

[MattAgn]

@MattAgn not sure but your first example looks a bit too complex. Seems like you could just declare some common findAll function:

function findAll(instance: ReactTestInstance, predicate: (instance: ReactTestInstance) => boolean, hidden: boolean) {
  // Use regular findAll fucntions
  if (hidden) return instance.findAll(predicate) 

  //else use new accessibility respecting algorithm,
}

Then inside each query, e.g. queryAllByText replace:

  function queryAllByTextFn(text, options) {
    const results = instance.findAll((node) =>
      getNodeByText(node, text, options)
    );

    return results;
  };

with

  // our new `findAll`
  function queryAllByTextFn(text, options) {
    const results = findAll(instance, (node) =>
      getNodeByText(node, text, options), options.hidden ?? true
    );

    return results;
  };

Yes sounds good to me

[mdjastrzebski]

@MattAgn great to read that you're back!

re findAll: let's start with simplest implementation as in DTL:

  root.findAll(...).filter(element => {
      return hidden === false
        ? isInaccessible(element, {
            isSubtreeInaccessible: cachedIsSubtreeInaccessible,
          }) === false
        : true
    })

https://github.com/testing-library/dom-testing-library/blob/a9a8cf26992ff0f6b4257b7300939f461d04440d/src/queries/role.js#L193

It contains some optimization in form of cachedIsSubtreeInaccessible.

[MattAgn]

@MattAgn great to read that you're back!

re findAll: let's start with simplest implementation as in DTL:

  root.findAll(...).filter(element => {
      return hidden === false
        ? isInaccessible(element, {
            isSubtreeInaccessible: cachedIsSubtreeInaccessible,
          }) === false
        : true
    })

testing-library/dom-testing-library@a9a8cf2/src/queries/role.js#L193

It contains some optimization in form of cachedIsSubtreeInaccessible.

you mean I don't extract the findAll in another function yet but instead we add some caching to isInaccessible?

[mdjastrzebski]

@MattAgn We could replicate DTL behavior of using cachedIsSubtreeInaccessible for better coherence with DTL. While caching should also address performance issues.

[MattAgn]

cachedIsSubtreeInaccessible

sure, always nice to have a little boost in performance. I would still be in favor of extracting the findAll function though, wdyt? Otherwise it's gonna be quite a pain to maintain considering the number of queries we have

The findAll could look something like (I simply copy pasted the code from DTL for now) :

export function findAll<Options extends AccessibilityOption>(
  instance: ReactTestInstance,
  searchCallback: (node: ReactTestInstance) => boolean,
  options?: Options
) {
  const subtreeIsInaccessibleCache = new WeakMap();
  function cachedIsSubtreeInaccessible(element) {
    if (!subtreeIsInaccessibleCache.has(element)) {
      subtreeIsInaccessibleCache.set(element, isSubtreeInaccessible(element));
    }

    return subtreeIsInaccessibleCache.get(element);
  }

  const results = instance.findAll(searchCallback);

  if (options?.hidden === false) {
    return results;
  }

  return results.filter(
    (result) =>
      !isInaccessible(result, { isSubtreeInaccessible: cachedIsSubtreeInaccessible })
  );
}

[mdjastrzebski]

Looks pretty good, we could rename searchCallback to predicate as more descriptive name and then tweak the body a bit. Pls also add defaultHidden option to configure as in DTL

 export function findAll<Options extends AccessibilityOption>(
  instance: ReactTestInstance,
  predicate: (node: ReactTestInstance) => boolean,
  options?: Options
) {
  const matchingElement = instance.findAll(predicate);

  const hidden = options?.hidden ?? getConfig().defaultHidden; // We will want to add `defaultHidden: boolean` option to `configure`  
  if (hidden) {
    return matchingElement;
  }

  const subtreeIsInaccessibleCache = new WeakMap();

  function cachedIsSubtreeInaccessible(element) {
    if (!subtreeIsInaccessibleCache.has(element)) {
      subtreeIsInaccessibleCache.set(element, isSubtreeInaccessible(element));
    }

    return subtreeIsInaccessibleCache.get(element);
  }

  return matchingElement.filter(
    (element) =>
      !isInaccessible(element, { isSubtreeInaccessible: cachedIsSubtreeInaccessible })
  );
}

@MattAgn wdyt?

[MattAgn]

@mdjastrzebski sounds great! I'm almost done, I'm mainly missing the tests

[MattAgn]

@mdjastrzebski yes almost done! (Sorry I'm so slow on this PR, I've got a lot of time this week end, I'll finish it by then) I'm also missing tests on our new findAll, do you agree it should be tested? (I've started it but have an issue with the initial findAll, I'll push it this week end and you can tell me what you think)

Should I implement the new namings as well? Everybody seems to agree on this issue

[mdjastrzebski]

Pls rebase due to #1193 getting merged.

[codecov[bot]]

Codecov Report

Base: 94.18% // Head: 94.38% // Increases project coverage by +0.19% :tada:

Coverage data is based on head (dbccaf5) compared to base (b76f11a). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
+ Coverage   94.18%   94.38%   +0.19%     
==========================================
  Files          41       42       +1     
  Lines        2837     2936      +99     
  Branches      428      435       +7     
==========================================
+ Hits         2672     2771      +99     
  Misses        165      165              

Impacted Files	Coverage Δ
src/config.ts	100.00% <100.00%> (ø)
src/helpers/accessiblity.ts	94.80% <100.00%> (+1.36%)	:arrow_up:
src/helpers/findAll.ts	100.00% <100.00%> (ø)
src/queries/a11yState.ts	100.00% <100.00%> (ø)
src/queries/a11yValue.ts	100.00% <100.00%> (ø)
src/queries/displayValue.ts	100.00% <100.00%> (ø)
src/queries/hintText.ts	100.00% <100.00%> (ø)
src/queries/labelText.ts	100.00% <100.00%> (ø)
src/queries/placeholderText.ts	100.00% <100.00%> (ø)
src/queries/role.ts	100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[MattAgn]

@MattAgn I've added final ("Final 2 new" 😉) nit comments, otherwise I am really happy with the implementation you've built! ro🚀

BTW seems like we still need:

• [ ] Docs updates
• [ ] Flow type updates

Indeed completly forgot about flow types! @mdjastrzebski For the documentation, should I implement the new namings first? Then I can write the documentation with the new namings, otherwise we'll rewrite part of it

[mdjastrzebski]

@MattAgn let's write docs with the existing hidden naming and merge it as is. We will provide this naming anyway for RTL-compat, and the improved naming is just an alias and we can make that changes in a separate smaller PR. I would like to get this feature merged, as constantly rebasing is simply a waste.

[MattAgn]

Good point, i'm on it (and I agree, rebasing all the time is a pain 😅)

[MattAgn]

Done for the documentation, I took some inspiration from RTL and the TextMatch option doc in RNTL, wdyt?