feat: accessibility findAll #787

gabrielgrover · 2021-07-27T00:22:58Z

Summary

This PR adds a new option to the render function called respectAccessibility. When this property is true a new implementation of ReactTestInstance.findAll() is
used. This new findAll() is defined in render.js inside the function appendFindAllTrap. This new findAll ignores sub trees that have a root node with a prop
accessibilityElementsHidden: true. This solution is an attempt to mimic the first approach found in @MattAgn 's list.

Test plan

Tests added in src/__tests__/byTestId.test.js, src/__tests__/byPlaceholderText.test.js, and src/__tests__/byDisplayValue.test.js

run yarn test
- tests should pass

Scope

Implement findAll trap
Add tests
Add flow & typescript type definitions
Update documentation

thymikee · 2021-07-27T08:14:52Z

cc @satya164. Would you like to check this out? Should make testing React Navigation more life-like :)

mdjastrzebski

I love the idea of this PR. Great work!

Couple of things to improve:

findAll trap seems to work in simple cases but we should also check if it works in more complex cases by writing tests with:
- more nested components (in both host & composite components)
- <Text>{1}</Text>, {null}, etc (check detailed comments).
add tests that cover finding of non-hidden elements in siblings of accessibilityElementsHidden elements
other minor stylistic issues (improved, typing, etc)

Also new params seems not added typescript defintions.

mdjastrzebski · 2021-07-27T14:12:45Z

src/__tests__/byDisplayValue.test.js

+    queryByDisplayValue,
+  } = render(<Comp />, { respectAccessibility: true });
+
+  await expect(findAllByDisplayValue('test_01')).rejects.toBeTruthy();


consider using rejects.toEqual(...)

nit: pls sort queries by they typical sort order: gets, query, find. Having them randomly shuffled raises question why they are in this particular order.

mdjastrzebski · 2021-07-27T14:16:29Z

src/__tests__/byDisplayValue.test.js

+      <View accessibilityElementsHidden>
+        <TextInput value="test_01" />
+      </View>
+      <TextInput value="test_02" />


Pls add some more expects here that query test_02 so that we know that accessibilityElementsHidden did not affect sibling elements.

mdjastrzebski · 2021-07-27T14:18:19Z

src/__tests__/byText.test.js

+  await expect(queryByText('hello world')).toBeNull();
+});
+
+test('queryAllByText returns an empty array if respectAccessibilityProps is true', () => {


Seems like it covers the same cases the above test, pls merge these together.

mdjastrzebski · 2021-07-27T14:25:39Z

src/render.js

+
+function appendFindAllTrap(renderer: ReactTestRenderer) {
+  return new Proxy(renderer.root, {
+    get(target, prop) {


nit, readability: could we get some more type explicit type annotations here?

mdjastrzebski · 2021-07-27T14:28:11Z

src/render.js

+  return new Proxy(renderer.root, {
+    get(target, prop) {
+      const isFindAllProp = prop === 'findAll';
+      const newFindAll = (fn, node = target) => {


can newFindAll be moved to external variable and be type-annotated?

mdjastrzebski · 2021-07-27T14:28:34Z

src/render.js

+        }, initial);
+      };
+
+      return isFindAllProp ? newFindAll : Reflect.get(...arguments);


Would that work?

Suggested change

return isFindAllProp ? newFindAll : Reflect.get(...arguments);

return isFindAllProp ? newFindAll : target[prop];

mdjastrzebski · 2021-07-27T14:30:57Z

src/render.js

+            return result;
+          }
+
+          return result.concat(newFindAll(fn, child));


nit:

Suggested change

return result.concat(newFindAll(fn, child));

return [...result, ...newFindAll(fn, child)];

mdjastrzebski · 2021-07-27T14:37:36Z

src/render.js

+        const initial = fn(node) ? [node] : [];
+
+        return node.children.reduce((result, child) => {
+          if (typeof child === 'string') {


Seems like we need also to handle number type here as well (maybe also boolean). Please create tests with <Text>{1}</Text>, {false}, {null}, {undefined} and see if it crashes.

mdjastrzebski · 2021-07-27T14:46:20Z

@thymikee I'm really impressed by this PR. After ironing our all of the potential edge cases we should consider this to be default option for next major release, as most of regular multi-screen tests would benefit from it.

mdjastrzebski · 2021-07-27T14:49:32Z

As the intended use case if with React Navigation, perhaps we could find a way to actually test it with that library.

gabrielgrover · 2021-07-27T20:25:30Z

Thanks for the detailed review @mdjastrzebski ! I will look through and address your comments after work today

mdjastrzebski · 2021-07-28T15:14:30Z

@gabrielgrover after some more consideration of findAll proxy trap, I think there is a risk that our implementation will behave slightly differently than findAll from React Test Renderer. To overcome this I see two approaches:

base our implementation on code from React Test Renderer (i.e. copy-paste-modify) so that we match their behavior,
or rather call the original findAll but filter the results be doing recursive parent traversal and removing elements for which any parent has accessibilityElementsHidden set to true.

IMO I would lean to option 2, as it would allows as to remove proxy code and will allow us to rely on React Test Renderer findAll implementation, without copying their code.

CC: @thymikee

gabrielgrover · 2021-07-28T15:49:43Z

@gabrielgrover after some more consideration of findAll proxy trap, I think there is a risk that our implementation will behave slightly differently than findAll from React Test Renderer. To overcome this I see two approaches:

base our implementation on code from React Test Renderer (i.e. copy-paste-modify) so that we match their behavior,

or rather call the original findAll but filter the results be doing recursive parent traversal and removing elements for which any parent has accessibilityElementsHidden set to true.

IMO I would lean to option 2, as it would allows as to remove proxy code and will allow us to rely on React Test Renderer findAll implementation, without copying their code.

CC: @thymikee

@mdjastrzebski So I actually did option 1. It is actually where i got the following line

  if (typeof child === 'string') { ... }

ReactTestRenderer basically does a depth first search. The only difference is my use of .reduce(). As for option 2 the only issue I see with calling their findAll is that there doesn't seem to be a way for me to identify the parent or child of a given node. The call back for findAll is called by ReactTestRenderer with a node as the first argument. Maybe I am missing something, but there doesn't seem to be anything in that node object that can uniquely identify itself nor its parent / children. If I am wrong about this please let me know because I agree that it would be a better approach to lean on their implementation. However if I am right then I think we would need to stick to option 1

Also note if we go with option 2 we would have to keep a list of root nodes in state somewhere that have the accessibilityElementsHidden prop set to true so we can invalidate their children when they come up. Which means tests will traverse entire subtrees regardless if a root has accessibilityElementsHidden set to true because we do not control traversal logic since we defer to the ReactTestRenderer lib.

satya164 · 2021-07-29T09:29:20Z

Excited for this PR!

I was thinking, since accessibilityElementsHidden is iOS only, that we'd need to handle few more accessibility-related props as well:

Similar prop for Android: importantForAccessibility="no-hide-descendants"
For all platforms: display: 'none' in styles
On iOS: accessibilityViewIsModal - This should be equivalent to marking sibling elements as non-accessible

mdjastrzebski · 2021-07-29T10:43:28Z

Re findAll ancestor traversal: (@gabrielgrover) I'm not sure if i understood your comment correctly, but since original findAll is returning TestInstance array, we could use TestInstance parent prop to navigate to root ancestors and simply filter out these elements that have accessibilityElementsHidden=true.

Pseudo-code:

function isVisibleToAccessibility(element: TestInstance) {
  const isElementVisible = !instance.accessibilityElementsHidden;
  const isParentVisible = !instance.parent || isVisibleToAccessibility(instance.parent);
  return isParentVisible && isElementVisible;
}

// Then
const elements = findAll(...);
const visibleElements = elements.filter(isVisibleToAccessibility);

mdjastrzebski · 2021-07-29T10:59:06Z

Re accessibilityViewIsModal (@satya164): this prop details sound a little confusing. Could you clarify that I am understanding this correctly?

If there is only one host component with accessibilityViewIsModal we threat its sibling as they have accessibilityElementsHidden == true.

If there is more than one host components with accessibilityViewIsModal only one of these is actually filtering out its siblings. And the "effective" component with accessibilityViewIsModal is the one that is the deepest in the hierarchy?

mdjastrzebski · 2021-07-29T11:00:11Z

(Closed by mistake, reopened this PR)

satya164 · 2021-07-29T11:06:50Z

If there is only one host component with accessibilityViewIsModal we threat its sibling as they have accessibilityElementsHidden == true.

Yes, I think we can just treat <A accessibilityViewIsModal /><B /> as <A /><B accessibilityElementsHidden />.

If there is more than one host components with accessibilityViewIsModal only one of these is actually filtering out its siblings. And the "effective" component with accessibilityViewIsModal is the one that is the deepest in the hierarchy?

I don't know how it'll work with multiple siblings with accessibilityViewIsModal, normally there needs to be only one modal shown at a time. We can check on an iOS device to see how VoiceOver reacts to it. If there are views with accessibilityViewIsModal nested inside each other, then yea, as per my understanding, the innermost one is the active modal.

gabrielgrover · 2021-07-29T17:48:18Z

Re findAll ancestor traversal: (@gabrielgrover) I'm not sure if i understood your comment correctly, but since original findAll is returning TestInstance array, we could use TestInstance parent prop to navigate to root ancestors and simply filter out these elements that have accessibilityElementsHidden=true.

Pseudo-code:
function isVisibleToAccessibility(element: TestInstance) {
  const isElementVisible = !instance.accessibilityElementsHidden;
  const isParentVisible = !instance.parent || isVisibleToAccessibility(instance.parent);
  return isParentVisible && isElementVisible;
}

// Then
const elements = findAll(...);
const visibleElements = elements.filter(isVisibleToAccessibility);

I see. We started with different assumptions. I was going about this problem with the assumption that we wouldn't want to traverse the tree more than once for each query.

mdjastrzebski · 2021-07-30T11:07:03Z

I do not see any real problem (performance or other) with traversing the component tree bottom-up or top-down multiple times (as long as it's not an infinite loop ;-)). Performance impact will be limited to running the integration tests.

Digging a bit into details, traversing component tree bottom-up from given node should be cheaper than top-down, because going up you have only one parent, while going down you can have multiple children. Simplifying things a little bit, traversing up from given node should have O(log n) complexity while traversing down from root should have O(n) complexity, when using big O notation. Additionally we will do the traversal only for small number of components, those that are returned from findAll function, which should be in range from 0 to say 5, most of the time.

There for cost of these extra traversals bottom-up seems acceptable tradeoff compared to benefit of simpler and more robust code, which avoids patching renderer object and duplicating findAll algorithm.

@gabrielgrover: wdyt?

gabrielgrover · 2021-07-30T16:39:33Z

I do not see any real problem (performance or other) with traversing the component tree bottom-up or top-down multiple times (as long as it's not an infinite loop ;-)). Performance impact will be limited to running the integration tests.

Digging a bit into details, traversing component tree bottom-up from given node should be cheaper than top-down, because going up you have only one parent, while going down you can have multiple children. Simplifying things a little bit, traversing up from given node should have O(log n) complexity while traversing down from root should have O(n) complexity, when using big O notation. Additionally we will do the traversal only for small number of components, those that are returned from findAll function, which should be in range from 0 to say 5, most of the time.

There for cost of these extra traversals bottom-up seems acceptable tradeoff compared to benefit of simpler and more robust code, which avoids patching renderer object and duplicating findAll algorithm.

@gabrielgrover: wdyt?

Sounds good to me. Gonna work on it over the weekend.

…descendants"

gabrielgrover · 2021-08-10T21:28:26Z

@satya164 @mdjastrzebski Sorry just now have been able to dedicate some more time to this PR. Taking a look at the edge case for accessibilityViewIsModal. I assume I can check siblings by doing something like this

  const currentNodeIsHidden = instance.parent.children.some(c => c.props.accessibilityViewIsModal);

However, this would cause the node with the property accessibilityViewIsModal to also be "hidden". Is there someway I can identify a specific node so i can exclude it from the callback in the above code? Or can I make some assumptions on the number of elements with accessibilityViewIsModal?

satya164 · 2021-08-10T21:42:37Z

this would cause the node with the property accessibilityViewIsModal to also be "hidden".

I don't know any implementation details, but isn't something like this possible?

const currentNodeIsHidden = instance.parent.children.some(
  c => c !== instance && c.props.accessibilityViewIsModal
);

gabrielgrover · 2021-08-10T23:52:45Z

this would cause the node with the property accessibilityViewIsModal to also be "hidden".

I don't know any implementation details, but isn't something like this possible?
const currentNodeIsHidden = instance.parent.children.some(
  c => c !== instance && c.props.accessibilityViewIsModal
);

c and instance are both ReactTestInstances which are objects. There is no general way to compare objects. That's why we need to find some property of these objects that has a primitive value and is unique. Like an id of some sort. Or we can use JSON.stringify, but we will start hitting performance issues at that point.

gabrielgrover · 2021-08-11T04:42:33Z

this would cause the node with the property accessibilityViewIsModal to also be "hidden".

I don't know any implementation details, but isn't something like this possible?
const currentNodeIsHidden = instance.parent.children.some(
  c => c !== instance && c.props.accessibilityViewIsModal
);
c and instance are both ReactTestInstances which are objects. There is no general way to compare objects. That's why we need to find some property of these objects that has a primitive value and is unique. Like an id of some sort. Or we can use JSON.stringify, but we will start hitting performance issues at that point.

Actually it looks like using Object.is() will work.

const currentNodeIsHidden = instance.parent.children.some(
  c => !Object.is(c, instance) && c.props.accessibilityViewIsModal
);

gabrielgrover · 2021-08-11T04:55:50Z

@mdjastrzebski @satya164 pushed changes. Let me know what you think

mdjastrzebski · 2021-08-13T13:23:49Z

src/__tests__/byDisplayValue.test.js

+  );
+  const Comp = () => (
+    <View>
+      <OtherComp importantForAccessibility="no-hide-descendants" />


I believe that accessibility props (& style={{ display: 'none' }}) should be checked only on host components (typeof element.type === 'string') in order to reflect how are they rendered in RN. So here you either should use these props on e.g. View (recommended) or forward them inside OtherComp.

mdjastrzebski · 2021-08-13T13:31:01Z

src/render.js

@@ -107,3 +110,64 @@ function debug(
  debugImpl.shallow = (message) => debugShallow(instance, message);
  return debugImpl;
 }
+
+function appendFindAllTrap(renderer: ReactTestRenderer) {


I would suggest removing Proxy code, and using filter(isReactTestElementVisibleToAccessibility) in the queries. This way respectAccessibility would be moved from render option to query option, which would better reflect the fact that we are apply query modifier and not render modifier.

We should have some base QueryOptions type that would atm be { respectAccessibility: boolean } and would be passed as a second argument to all get/getAll/query/etc/ queries. Note, for findBy & findAllBy we would add the existing waitForOptions there.

mdjastrzebski

Thanks for building up this PR.

I've noticed two points that would like to discuss:

Atm respectAccessibility is a render option. However, it does not actual modify the render output (other than findAll method). IMO it would be better to make it queries option (get/getAll/query/etc). Each query would accept a 2nd optional argument of options and respectAccessibility would be the first available one.
In code checking accessibility props, you currently allow accessibility props to be added to composite components, this is especially visible in test code. However in order to reflect how RN renderer works, we should only take into account props set on host components (View, Text, etc). This is similar to styling composite components, you can have a style prop there, but the UI will not be styled unless you somehow apply that style to host component.

@gabrielgrover Pls let me know if you need any coding help in this PR. I'm eager to help :-)

mdjastrzebski · 2022-05-04T10:46:56Z

I'm closing this PR as stale, but since the idea is work pursuing I've create #970 issue which references this PR.

gabrielgrover added 2 commits July 26, 2021 15:06

feat: add Proxy trap for findAll method in ReactTestInstance

5c93709

test: add tests for new render option respectAccessibilityProps

dcf7c14

gabrielgrover changed the title ~~accessibility findAll~~ feat: accessibility findAll Jul 27, 2021

gabrielgrover added 4 commits July 26, 2021 17:24

feat: rename prop

521a81c

fix: remove comment

f166a87

test: update tests to account for option property rename

b627428

fix: lint errors

d33da47

thymikee requested a review from mdjastrzebski July 27, 2021 10:26

mdjastrzebski reviewed Jul 27, 2021

View reviewed changes

mdjastrzebski closed this Jul 29, 2021

mdjastrzebski reopened this Jul 29, 2021

gabrielgrover added 5 commits August 10, 2021 11:23

refactor: reimplement findAll trap to use parent traversal

d74b3e1

refactor: update tests

4647edb

feat: update options type

0013643

refactor: update newFindAll with new typings

cf8c6cb

feat: respect accessibility prop, importantForAccessibility="no-hide-…

1d8b401

…descendants"

refactor: update test

897a2b2

gabrielgrover added 3 commits August 10, 2021 21:43

feat: respect accessibilityViewIsModal

fd8ff4d

test: update test

c574ee9

refactor: change condition

b11c463

refactor: lint fix

5b5587f

gabrielgrover requested a review from mdjastrzebski August 11, 2021 05:03

mdjastrzebski reviewed Aug 13, 2021

View reviewed changes

therynamo mentioned this pull request Apr 13, 2022

Non-visible modal content shows up in queries #659

Closed

mdjastrzebski mentioned this pull request May 4, 2022

Provide query option to respect accessibility #970

Closed

mdjastrzebski closed this May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: accessibility findAll #787

feat: accessibility findAll #787

gabrielgrover commented Jul 27, 2021 •

edited by mdjastrzebski

Loading

thymikee commented Jul 27, 2021

mdjastrzebski left a comment

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski Jul 27, 2021

mdjastrzebski commented Jul 27, 2021

mdjastrzebski commented Jul 27, 2021

gabrielgrover commented Jul 27, 2021 •

edited

Loading

mdjastrzebski commented Jul 28, 2021

gabrielgrover commented Jul 28, 2021 •

edited

Loading

satya164 commented Jul 29, 2021

mdjastrzebski commented Jul 29, 2021

mdjastrzebski commented Jul 29, 2021 •

edited

Loading

mdjastrzebski commented Jul 29, 2021

satya164 commented Jul 29, 2021

gabrielgrover commented Jul 29, 2021 •

edited

Loading

mdjastrzebski commented Jul 30, 2021

gabrielgrover commented Jul 30, 2021

gabrielgrover commented Aug 10, 2021

satya164 commented Aug 10, 2021

gabrielgrover commented Aug 10, 2021

gabrielgrover commented Aug 11, 2021

gabrielgrover commented Aug 11, 2021

mdjastrzebski Aug 13, 2021

mdjastrzebski Aug 13, 2021

mdjastrzebski left a comment •

edited

Loading

mdjastrzebski commented May 4, 2022

	return isFindAllProp ? newFindAll : Reflect.get(...arguments);
	return isFindAllProp ? newFindAll : target[prop];

	return result.concat(newFindAll(fn, child));
	return [...result, ...newFindAll(fn, child)];

feat: accessibility findAll #787

feat: accessibility findAll #787

Conversation

gabrielgrover commented Jul 27, 2021 • edited by mdjastrzebski Loading

Summary

Test plan

Scope

thymikee commented Jul 27, 2021

mdjastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski commented Jul 27, 2021

mdjastrzebski commented Jul 27, 2021

gabrielgrover commented Jul 27, 2021 • edited Loading

mdjastrzebski commented Jul 28, 2021

gabrielgrover commented Jul 28, 2021 • edited Loading

satya164 commented Jul 29, 2021

mdjastrzebski commented Jul 29, 2021

mdjastrzebski commented Jul 29, 2021 • edited Loading

mdjastrzebski commented Jul 29, 2021

satya164 commented Jul 29, 2021

gabrielgrover commented Jul 29, 2021 • edited Loading

mdjastrzebski commented Jul 30, 2021

gabrielgrover commented Jul 30, 2021

gabrielgrover commented Aug 10, 2021

satya164 commented Aug 10, 2021

gabrielgrover commented Aug 10, 2021

gabrielgrover commented Aug 11, 2021

gabrielgrover commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski left a comment • edited Loading

Choose a reason for hiding this comment

mdjastrzebski commented May 4, 2022

gabrielgrover commented Jul 27, 2021 •

edited by mdjastrzebski

Loading

gabrielgrover commented Jul 27, 2021 •

edited

Loading

gabrielgrover commented Jul 28, 2021 •

edited

Loading

mdjastrzebski commented Jul 29, 2021 •

edited

Loading

gabrielgrover commented Jul 29, 2021 •

edited

Loading

mdjastrzebski left a comment •

edited

Loading