Skip to content

fix: suggest hidden:true with getByRole if element is inaccessible #745

Merged
kentcdodds merged 5 commits into
testing-library:masterfrom
marcosvega91:pr/add_hidden_prop
Aug 22, 2020
Merged

fix: suggest hidden:true with getByRole if element is inaccessible #745
kentcdodds merged 5 commits into
testing-library:masterfrom
marcosvega91:pr/add_hidden_prop

Conversation

@marcosvega91

@marcosvega91 marcosvega91 commented Aug 19, 2020

Copy link
Copy Markdown
Member

What: If an element is inaccessible getSuggestedQuery should suggest to use hidden:true with getByRole

Why: Answering to this issue #744, I wanted to share a link of testing-playground but the query was not suggested as I expected.

How: If the element is inaccessible and the query is getByRole I have added to queryArgs an hidden:true

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@marcosvega91 marcosvega91 changed the title fix: suggest hidden:true with getByRole if element is not accessible fix: suggest hidden:true with getByRole if element is inaccessible Aug 19, 2020
@codesandbox-ci

codesandbox-ci Bot commented Aug 19, 2020

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a5dfd75:

Sandbox Source
kentcdodds/react-testing-library-examples Configuration

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't just suggest it but rather say that the/a element is not part of the a11y tree.

The element being removed from the a11y tree has strong implications for users relying on screen readers. It's equivalent to the element being invisible for a sighted user.

I fear people will just randomly add aria-hidden and query for it if we explicitly recommend it.

@marcosvega91

Copy link
Copy Markdown
Member Author

Yes it was something to discuss. I think that we should add this to testing playground too. Maybe we should do a console.warn? While on testing playground show a text ?

@marcosvega91

Copy link
Copy Markdown
Member Author

what do you think of this ?
I have added a warning prop with a message, maybe this will be useful for other things. On testing-playground side we can show the warning message

p.s. I have added some types to suggestions

diff --git a/src/suggestions.js b/src/suggestions.js
index f584e7b..0d23434 100644
--- a/src/suggestions.js
+++ b/src/suggestions.js
@@ -39,6 +39,8 @@ function getRegExpMatcher(string) {
 }
 
 function makeSuggestion(queryName, element, content, {variant, name}) {
+  let warning = ''
+
   const queryArgs = [
     queryName === 'Role' || queryName === 'TestId'
       ? content
@@ -51,6 +53,10 @@ function makeSuggestion(queryName, element, content, {variant, name}) {
 
   if (queryName === 'Role' && isInaccessible(element)) {
     queryArgs.push({hidden: true})
+    warning = `Element is inaccessible. This means that the element and all his children are invisible to screen readers.
+    If you are using aria-hidden prop make sure this is the right choice for your case.
+    `
+    console.warn(warning)
   }
 
   const queryMethod = `${variant}By${queryName}`
@@ -60,6 +66,7 @@ function makeSuggestion(queryName, element, content, {variant, name}) {
     queryMethod,
     queryArgs,
     variant,
+    warning,
     toString() {
       let [text, options] = queryArgs
 
diff --git a/types/suggestions.d.ts b/types/suggestions.d.ts
index 17e48d6..8c1e68c 100644
--- a/types/suggestions.d.ts
+++ b/types/suggestions.d.ts
@@ -1,5 +1,13 @@
+export type QueryArg = {
+  [key: string]: RegExp | boolean
+}
+
 export interface Suggestion {
   queryName: string
+  queryMethod: string
+  queryArgs: QueryArg[]
+  variant: string
+  warning: string
   toString(): string
 }

@nullromo

nullromo commented Aug 19, 2020

Copy link
Copy Markdown
-    warning = `Element is inaccessible. This means that the element and all his children are invisible to screen readers.
-    If you are using aria-hidden prop make sure this is the right choice for your case.

+    warning = `Element is inaccessible. This means that the element and all its children are invisible to screen readers.
+    If you are using the aria-hidden prop, make sure this is the right choice for your case.

("his" -> "its", "using aria-hidden prop" -> "using the aria-hidden prop", "prop make" -> "prop, make")

@eps1lon

eps1lon commented Aug 20, 2020

Copy link
Copy Markdown
Member

what do you think of this ? [...]

-- #745 (comment)

👍 That is way clearer. There's always some risk involved but I hope this won't cause too much damage.

@marcosvega91

Copy link
Copy Markdown
Member Author

I have added the message as we discussed above 👍

@codecov

codecov Bot commented Aug 20, 2020

Copy link
Copy Markdown

Codecov Report

Merging #745 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #745   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          657       665    +8     
  Branches       173       175    +2     
=========================================
+ Hits           657       665    +8     
Impacted Files Coverage Δ
src/suggestions.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe04b50...a5dfd75. Read the comment docs.

@kentcdodds

Copy link
Copy Markdown
Member

Looks solid. Thanks!

@kentcdodds
kentcdodds merged commit 6d6641d into testing-library:master Aug 22, 2020
@marcosvega91
marcosvega91 deleted the pr/add_hidden_prop branch August 22, 2020 23:03
@kentcdodds

Copy link
Copy Markdown
Member

🎉 This PR is included in version 7.22.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants