byRole queries return only accessibility elements #1244

MattAgn · 2022-11-28T07:01:20Z

Summary

Solves #1180. It makes byRole queries return only implicit or explicit accessibility elements. Explicit accessibility elements are those with accessibility prop set to true. Implicit accessibility elements are Text, TextInput, Pressable and Switch (tested with VoiceOver for Switch to check it was indeed accessible even without setting the accessible prop and I checked through a snapshot, behing the scene, Switch resolves to a host component RCTSwitch).

Test plan

Unit tests for isAccessibilityElement helper
One test to check that byRole only returns accessibility elements

codecov · 2022-11-28T07:04:49Z

Codecov Report

Base: 94.94% // Head: 95.13% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (937fa2d) compared to base (e38c627).
Patch coverage: 98.61% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1244      +/-   ##
==========================================
+ Coverage   94.94%   95.13%   +0.18%     
==========================================
  Files          45       46       +1     
  Lines        3088     3205     +117     
  Branches      457      483      +26     
==========================================
+ Hits         2932     3049     +117     
  Misses        156      156

Impacted Files	Coverage Δ
src/queries/text.ts	`97.67% <93.33%> (+0.37%)`	⬆️
src/config.ts	`100.00% <100.00%> (ø)`
src/fireEvent.ts	`98.65% <100.00%> (ø)`
src/helpers/accessiblity.ts	`100.00% <100.00%> (ø)`
src/helpers/host-component-names.tsx	`100.00% <100.00%> (ø)`
src/helpers/matchers/matchTextContent.ts	`100.00% <100.00%> (ø)`
src/queries/displayValue.ts	`100.00% <100.00%> (ø)`
src/queries/placeholderText.ts	`100.00% <100.00%> (ø)`
src/queries/role.ts	`100.00% <100.00%> (ø)`

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

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

mdjastrzebski

So far looks quite good. Good catch with Switch component! This PR should use useBreakingChanges config option so that we will be able to turn it on for vNext.

mdjastrzebski · 2023-01-14T09:56:11Z

src/helpers/__tests__/accessiblity.test.tsx

+      const element = render(
+        <View accessible={true} testID="view" />
+      ).getByTestId('view');


nit, style: could we separate render and getByTestId calls, as it's more readable and matches with what we suggest to our users in docs and examples.

Suggested change

const element = render(

<View accessible={true} testID="view" />

).getByTestId('view');

const views = render(

<View accessible={true} testID="view" />

)

const element = views.getByTestId('view');

mdjastrzebski · 2023-01-14T22:56:59Z

src/queries/__tests__/role.test.tsx

@@ -733,3 +733,12 @@ test('byRole queries support hidden option', () => {
    `"Unable to find an element with role: "button""`
  );
 });
+
+test('takes accessible prop into account', () => {


Suggested change

test('takes accessible prop into account', () => {

test('ignores elements with accessible={false}', () => {

mdjastrzebski · 2023-01-14T22:57:55Z

src/queries/__tests__/role.test.tsx

@@ -733,3 +733,12 @@ test('byRole queries support hidden option', () => {
    `"Unable to find an element with role: "button""`
  );
 });
+
+test('takes accessible prop into account', () => {


Could we add another test that would pass a View with accessibilityRole but without accessible prop, and check that it does not get matched.

mdjastrzebski · 2023-01-14T23:04:18Z

src/helpers/__tests__/accessiblity.test.tsx

+  describe('when accessible prop is not defined', () => {
+    test('returns true for Text component', () => {
+      const element = render(<Text testID="text">Hey</Text>).getByTestId(
+        'text'
+      );
+      expect(isAccessibilityElement(element)).toEqual(true);
+    });


How about structuring the tests a bit different form, by grouping default/true/false tests for given component type together. These tests will be both exhaustive and take less LOCs.

Suggested change

describe('when accessible prop is not defined', () => {

test('returns true for Text component', () => {

const element = render(<Text testID="text">Hey</Text>).getByTestId(

'text'

);

expect(isAccessibilityElement(element)).toEqual(true);

});

test('TextInput matching', () => {

const { getByTestId } = render(<View>

<Text testID="default">Default</Text>

<Text testID="true">True</Text>

<Text testID="false">False</Text>

</View>);

expect(isAccessibilityElement(getByTestId("default"))).toBeTrue();

expect(isAccessibilityElement(getByTestId("true"))).toBeTrue();

expect(isAccessibilityElement(getByTestId("false"))).toBeFalse();

});

We could have such tests for key views that render to host views: View, Text, TextInput, Switch.

Maybe we can also add tests for composite components that render to View with accessible prop: Pressable, TouchableOpacity.

good idea! the only downside of this method is that tests title and tests in general are less explicit

Now :

Before :

what do you prefer? both are fine for me

mdjastrzebski · 2023-01-14T23:09:51Z

src/queries/role.ts

        typeof node.type === 'string' &&
+        isAccessibilityElement(node) &&


This would be a breaking change so we should apply it only when useBreakingChanges config is set.

MattAgn · 2023-01-17T15:27:18Z

@mdjastrzebski thanks for the review! I've fixed everything you mentioned

mdjastrzebski

All good. Let's merge this 🔥

mdjastrzebski changed the title ~~byRole queries return only accessibility elements~~ [WIP] byRole queries return only accessibility elements Dec 28, 2022

MattAgn changed the title ~~[WIP] byRole queries return only accessibility elements~~ byRole queries return only accessibility elements Jan 13, 2023

MattAgn marked this pull request as ready for review January 13, 2023 12:35

MattAgn force-pushed the fix/byrole-accessible branch from 0476265 to 6947473 Compare January 13, 2023 13:34

MattAgn requested review from mdjastrzebski and pierrezimmermannbam January 13, 2023 13:42

mdjastrzebski requested changes Jan 14, 2023

View reviewed changes

EBAM006 added 8 commits January 17, 2023 11:36

refactor: group tests in accessibility

2577013

feat: add isAccessibilityElement helper

05be4af

chore: fix typo

7a4b7fc

feat: handle accessible in byrole

f0cda1c

feat: only check accessibility when useBreakingChanges true

9f731cd

test: group a11y tests together

56347ce

test: add implicit accessible test

18de89f

test: review byrole test titles

8611599

MattAgn force-pushed the fix/byrole-accessible branch from 6947473 to 8611599 Compare January 17, 2023 15:23

test: add test case for byRole

3326de7

mdjastrzebski added 2 commits January 17, 2023 22:29

refactor: code review changes

670333a

fix: code cov warnings

937fa2d

mdjastrzebski approved these changes Jan 17, 2023

View reviewed changes

mdjastrzebski merged commit 5f94d52 into callstack:main Jan 17, 2023

MattAgn mentioned this pull request Jan 18, 2023

*ByRole: return only elements with implicit or explicit accessible={true} prop #1180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

byRole queries return only accessibility elements #1244

byRole queries return only accessibility elements #1244

MattAgn commented Nov 28, 2022 •

edited

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

mdjastrzebski left a comment

mdjastrzebski Jan 14, 2023

mdjastrzebski Jan 14, 2023

mdjastrzebski Jan 14, 2023

MattAgn Jan 17, 2023

mdjastrzebski Jan 14, 2023

mdjastrzebski Jan 14, 2023

MattAgn Jan 17, 2023 •

edited

Loading

MattAgn Jan 17, 2023 •

edited

Loading

MattAgn Jan 17, 2023

mdjastrzebski Jan 14, 2023

MattAgn commented Jan 17, 2023

mdjastrzebski left a comment

	test('takes accessible prop into account', () => {
	test('ignores elements with accessible={false}', () => {

-  describe('when accessible prop is not defined', () => {
-    test('returns true for Text component', () => {
-      const element = render(<Text testID="text">Hey</Text>).getByTestId(
-        'text'
-      );
-      expect(isAccessibilityElement(element)).toEqual(true);
-    });
+  test('TextInput matching', () => {
+    const { getByTestId } = render(<View>
+      <Text testID="default">Default</Text>
+      <Text testID="true">True</Text>
+      <Text testID="false">False</Text>
+    </View>);
+    expect(isAccessibilityElement(getByTestId("default"))).toBeTrue();
+    expect(isAccessibilityElement(getByTestId("true"))).toBeTrue();
+    expect(isAccessibilityElement(getByTestId("false"))).toBeFalse();
+  });

		typeof node.type === 'string' &&
		isAccessibilityElement(node) &&

byRole queries return only accessibility elements #1244

byRole queries return only accessibility elements #1244

Conversation

MattAgn commented Nov 28, 2022 • edited Loading

Summary

Test plan

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

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

MattAgn Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

MattAgn Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattAgn commented Jan 17, 2023

mdjastrzebski left a comment

Choose a reason for hiding this comment

MattAgn commented Nov 28, 2022 •

edited

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

MattAgn Jan 17, 2023 •

edited

Loading

MattAgn Jan 17, 2023 •

edited

Loading