feat(byRole): Exclude inaccessible elements #352

eps1lon · 2019-09-19T12:42:51Z

BREAKING
Certain¹ elements will no longer be included in byRole queries.
Examples:

<ul hidden /> will no longer by included in *byRole('list')
<main aria-hidden="true"><ul /><main> will no longer return an element in *byRole('list') or getByRole('main')

What:

Implement https://www.w3.org/TR/core-aam-1.1/#exclude_elements2 with the following deviation:

role="none" and role="presentation" is included (not following MUST criteria)
I feel like this is more of a concern for an actual a11y tree implementation. getByRole('presentation') not returning anything even if the element is visible is probably confusing but I could see us throwing an error here. We could argue that one should use getByTestId instead since this makes it obvious you're not querying the a11y tree.
descendants of "Children Presentational: True" are included (not following SHOULD criteria)

Why:

Closes #350

How:

Implementation is kept simple and mirrors the spec. Faster implementation is considered for a future PR.

Checklist:

Documentation added to the
docs site Will do once this is approved
~~[ ] Typescript definitions updated~~ Separate type PR is prepared
Tests
Ready to be merged

I'm open to removing the option and making it default. In any case it should be released in a breaking change since it is potentially breaking.

¹ excluded are elements that

have aria-hidden=true or any of its ancestors
have the hidden attribute or any of its ancestors
have display: none or any of its ancestors
have hidden as their computed visibility

kentcdodds

Looking good. A few things. Also it looks like we're missing some tests and we should add this to all of the queries.

kentcdodds · 2019-09-19T15:48:13Z

src/__tests__/role.js

+  expect(() => getByRole('list')).toThrowErrorMatchingInlineSnapshot(`
+"Unable to find an accessible  element with the role "list"
+
+There are no accessible roles.


We should probably add something to this message that says: "But there are some inaccessible roles. If you wish to access them, then set the hidden option to true. Learn more about this here: linktodocs.com"

This would require a lot of refactoring if we want to actually determine if there are hidden roles.

Would it be sufficient to say "There might be inaccessible roles" instead?

Extended the message and added a link to the docs but didn't implement displaying accessible vs. inaccessible roles. I think this isn't required to get this merged but I would like to see this at some point as well.

kentcdodds · 2019-09-19T15:49:20Z

src/queries/role.js


  ${roles.replace(/\n/g, '\n  ').replace(/\n\s\s\n/g, '\n\n')}
 `.trim()
  }

  return `
-Unable to find an element with the role "${role}"
+Unable to find an ${
+    hidden === false ? 'accessible ' : ''


Remove the extra space.

Suggested change

hidden === false ? 'accessible ' : ''

hidden === false ? 'accessible' : ''

I think I need to keep this and remove the unconditional space later on. Otherwise we get

Unable to find an element with the role "${role} ^^ double space

for hidden: false

Extra space is still there (since I'm pretty sure it's required) but I fixed the double space issue.

kentcdodds

I'm good with this. Just one thing.

kentcdodds · 2019-09-23T18:17:37Z

src/role-helpers.js

+
+  let currentElement = element
+  while (currentElement !== null) {
+    if (currentElement.hasAttribute('hidden')) {


Is it permissible to have hidden="false"?

This would also be considered hidden. I should use the property here since it does not require knowledge of attributes vs. properties. You can also check this in your user agent's default stylesheet which should have

[hidden] { display: none; }

This is only a recommendation though so we rely on the hidden attribute definition:

The hidden attribute is a boolean attribute.

-- https://html.spec.whatwg.org/multipage/interaction.html#dom-hidden

The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

-- https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attribute

Would be interesting to see what react does with boolean attributes. It would be nice if it would add a warning since hidden="false" does not work as one might expect.

And it does:

Warning: Received the string false for the boolean attribute hidden. The browser will interpret it as a truthy value. Did you mean hidden={false}?

src/role-helpers.js

kentcdodds · 2019-09-23T18:47:15Z

I'm happy with this. Thank you!

kentcdodds · 2019-09-23T18:51:15Z

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

eps1lon · 2019-09-23T18:56:55Z

Unfamiliar with semantic-release: Should I have used another message format or was this intended to be a feature release?

kentcdodds · 2019-09-23T19:03:42Z

I set the commit message when I use "Rebase & Merge." I intended this to be a feature release because I consider it highly unlikely to break anyone's existing tests. I'll take the brunt of any anger or frustration if I'm wrong.

jsphstls · 2019-10-14T17:34:03Z

I do think a breaking change should have been a major version. I somehow picked this up with a new lockfile.

My breaking case is that I have a table with a scrolling body which essentially created two headers, one that is visible and one that is sr-only. The strategy is arguable. EIther way, I was used to finding both sets of columnheaders in the tests but now only one is found so the tests broke with seemingly unrelated changes. This change is ultimately good but the rollout strategy cost me some time.

eps1lon · 2019-10-14T17:39:57Z

sr-only

If this means "screen-reader only" then this is either a bug on our side or it detected an actual issue inside your markup. Could you share your markup + style that you would consider screen-reader only? This function should implement specification from the W3C which is either already implemented in assistive technology or will be.

jsphstls · 2019-10-14T17:47:19Z

Apologies, I meant "one that is visible and aria-hidden and one that is sr-only". The changes in this PR appear to be working fine.

My point is that I did have tests that broke because of this PR.

eps1lon · 2019-10-14T17:49:40Z

My point is that I did have tests that broke because of this PR.

Well as kent said

I'll take the brunt of any anger or frustration if I'm wrong.

😄

kentcdodds · 2019-10-15T15:46:44Z

Sorry! 😬

shaunak · 2023-02-07T17:53:33Z

Is there a reason this doesn't exist for *byText queries?

eps1lon added enhancement New feature or request BREAKING CHANGE This change will require a major version bump labels Sep 19, 2019

kentcdodds reviewed Sep 19, 2019

View reviewed changes

eps1lon force-pushed the feat/byRole/accessible branch from 950c779 to 3f5debb Compare September 21, 2019 15:42

eps1lon added 7 commits September 23, 2019 15:49

Add acceptance tests

7c7527b

Implement hidden: false

11eca12

Distinguish error message for hidden

2c4890e

Fix double space

99a0b3b

Add hint about possible inaccessible roles

2750a8c

Add tests for hidden: true

08c94ca

Fix codecoverage

7cdfcfa

eps1lon force-pushed the feat/byRole/accessible branch from b3fe775 to 7cdfcfa Compare September 23, 2019 13:49

kentcdodds reviewed Sep 23, 2019

View reviewed changes

eps1lon commented Sep 23, 2019

View reviewed changes

src/role-helpers.js Outdated Show resolved Hide resolved

Use hidden property instead of manually reflecting boolean attributes

2a354ef

kentcdodds merged commit dbbea6e into testing-library:master Sep 23, 2019

kentcdodds added the released label Sep 23, 2019

eps1lon deleted the feat/byRole/accessible branch September 23, 2019 18:54

This was referenced Sep 24, 2019

feat(queries): Add docs about byRole#hidden testing-library/testing-library-docs#266

Merged

feat(testing-library__dom): Add byRole#hidden DefinitelyTyped/DefinitelyTyped#38560

Merged

eps1lon mentioned this pull request Oct 28, 2019

breaking change in 6.8.1 #396

Closed

eps1lon mentioned this pull request Oct 29, 2019

feat: Make default of hidden in ByRole configurable #398

Merged

4 tasks

	hidden === false ? 'accessible ' : ''
	hidden === false ? 'accessible' : ''

feat(byRole): Exclude inaccessible elements #352

feat(byRole): Exclude inaccessible elements #352

Uh oh!

Conversation

eps1lon commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eps1lon Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kentcdodds commented Sep 23, 2019

Uh oh!

kentcdodds commented Sep 23, 2019

Uh oh!

eps1lon commented Sep 23, 2019

Uh oh!

kentcdodds commented Sep 23, 2019

Uh oh!

jsphstls commented Oct 14, 2019

Uh oh!

eps1lon commented Oct 14, 2019

Uh oh!

jsphstls commented Oct 14, 2019

Uh oh!

eps1lon commented Oct 14, 2019

Uh oh!

kentcdodds commented Oct 15, 2019

Uh oh!

shaunak commented Feb 7, 2023

Uh oh!

Uh oh!

eps1lon commented Sep 19, 2019 •

edited

Loading

eps1lon Sep 21, 2019 •

edited

Loading