create rule: github/a11y-no-visually-hidden-interactive-element #446

kendallgassner · 2023-07-05T22:04:39Z

What

relates to: https://github.com/github/accessibility/issues/3739

This was an interesting lint rule to write because we not only utilize the className='sr-only' to visually hide react content but we use the Primer component VisuallyHidden.

I wrote an eslint rule for both scenarios. This eslint rule determines if a JSX.Elements is visually hidden (i.e. has the sr-only className or of type VisuallyHidden). If it the element is visually hidden then it checks its self and children to see if it contains and interactive elements.

Coverage

What is an interactive element?

I hard coded a list of interactive elements to check against:

a
button
summary
select
option
textarea

I thought it was better to have a dis-allow list rather then an allow list (e.g. span, div etc.)

What is considered invalid?

nested interative elements:<VisuallyHidden><div><button>Submit</button></div></VisuallyHidden>
as props: <VisuallyHidden as='button'>Submit</VisuallyHidden>
multiple classNames: <button className='other sr-only'>Submit</button>

kendallgassner · 2023-07-05T22:07:52Z

lib/index.js

@@ -1,5 +1,6 @@
 module.exports = {
  rules: {
+    'a11y-no-sr-only-class-when-focusable': require('./rules/a11y-no-sr-only-class-when-focusable'),


Should we wait to include this until: https://github.com/github/accessibility/issues/3868

I think it's fine! We could always override disable is temporarily in our projects.

khiga8

Yay I'm excited to see this! I will review more in-depth tomorrow but leaving a comment per quick skim of the description!

but we use the Primer component VisuallyHidden

TIL about VisuallyHidden in Primer! 🤯

Since sr-only and VisuallyHidden is very GitHub/Primer-centric, and many consumers use this plugin outside of GitHub/Primer-consumers, could we allow this (the sr-only classname and the type) to be configured through the ESLint rule-level configs? I believe there are examples of this in the jsx-a11y library.

For example, if I am a consumer whose project uses the class visually-hidden instead of sr-only, or the type ScreenReaderOnly instead of VisuallyHidden, I'd still like to be able to benefit from this rule.

kendallgassner · 2023-07-06T02:12:22Z

@khiga8 Love that thought! Great idea... I'll add this in and re-tag you for an in-depth review when its updated.

kendallgassner · 2023-07-06T19:02:03Z

Interestingly, my passion week project is a use-case where we do want to visuallyHide an interactive element 🤔

khiga8 · 2023-07-06T19:14:22Z

@kendallgassner Could you bring up this example in the channel so we can discuss if it's valid?

kendallgassner · 2023-07-07T17:18:03Z

lib/rules/a11y-no-visually-hidden-interactive-element.js

+const {getElementType} = require('../utils/get-element-type')
+const {generateObjSchema} = require('eslint-plugin-jsx-a11y/lib/util/schemas')
+
+const defaultClassName = 'sr-only'


considering making the default visually-hidden for readability.

I prefer sr-only just because it's more widely used within different GitHub projects! As long as we document what the default is and how consumers can change it, I think sr-only would be okay.

kendallgassner · 2023-07-07T17:19:09Z

tests/a11y-no-visually-hidden-interactive-element.js

+    {code: "<span className='sr-only'>Text</span>;"},
+    {code: "<button className='other'>Submit</button>"},
+    {code: "<input className='sr-only' />"},
+    {code: "<a className='other show-on-focus'>skip to main content</a>"},


Check that show-on-focus class is valid! This way I don't have to remove the a tag from the list.

khiga8

Let's add this rule to react.js so it's on by default :)

I have a few suggestions. This is coming along really nicely <3

khiga8 · 2023-07-07T17:21:33Z

lib/rules/a11y-no-visually-hidden-interactive-element.js

+const {getElementType} = require('../utils/get-element-type')
+const {generateObjSchema} = require('eslint-plugin-jsx-a11y/lib/util/schemas')
+
+const defaultClassName = 'sr-only'


I prefer sr-only just because it's more widely used within different GitHub projects! As long as we document what the default is and how consumers can change it, I think sr-only would be okay.

khiga8 · 2023-07-07T17:24:27Z

docs/rules/a11y-no-visually-hidden-interactive-element.md

+
+## Rule Details
+
+This rule guards against visually hiding interactive elements. If a sighted keyboard user navigates to an interactive element that is visually hidden they might become confused and assume that keyboard focus has been lost.


Can we note the default class/type, as well as which interactive elements we're checking against?

I like the note you added in the code about why we're excluding input. Could we add it here too?

khiga8 · 2023-07-07T17:48:46Z

lib/rules/a11y-no-visually-hidden-interactive-element.js

+
+const checkIfInteractiveElement = (context, node) => {
+  const elementType = getElementType(context, node.openingElement)
+  const asProp = getPropValue(getProp(node.openingElement.attributes, 'as'))


I love this approach of just directly grabbing the tag from as, a lot more than this experimental approach I added some time ago! I've been wanting to revisit and remove this - Component mapping experimental!

Since as is Primer React specific, can we allow the as to be configured in the config as well? (e.g. maybe some design systems use tag or component).

I think this line can be moved into getElementType so this prop can be checked for all JSX rules, and we can get rid of the comopnent mapping stuff (lines 15-34) and the README!

Maybe we can pair on this in our sync :)

htmlPropName - name for 'as' config

theinterned

I will definitely defer to @khiga8 's review here, but no concerns form the web-systems standpoint

kendallgassner · 2023-07-07T19:51:32Z

@khiga8 I added the documentation updates and the htmlPropName. Can I clean up Component mapping experimental! and add this to getElementType in a follow up pr? -- I think it would be nice to test those changes and look at the logic in a clean state.

khiga8 · 2023-07-07T20:40:20Z

tests/a11y-no-visually-hidden-interactive-element.js

+const errorMessage =
+  'Avoid visually hidding interactive elements. Visually hiding interactive elements can be confusing to sighted keyboard users as it appears their focus has been lost when they navigate to the hidden element.'
+
+ruleTester.run('a11y-no-visually-hidden-interactive-element', rule, {


Can we add a test to make sure the options work as expected? Here's an example! :)

Amazing! I didn't realize that was a test option!

khiga8

I have one test suggestion, but please feel free to merge after that!

This looks great 🚀! Good call on doing the other updates in a separate PR, I agree!

create rule: a11y-no-sr-only-class-when-focusable

7ddd508

kendallgassner requested a review from a team as a code owner July 5, 2023 22:04

kendallgassner requested review from theinterned and khiga8 July 5, 2023 22:04

kendallgassner commented Jul 5, 2023

View reviewed changes

khiga8 reviewed Jul 5, 2023

View reviewed changes

pass params

6b767a5

kendallgassner commented Jul 7, 2023

View reviewed changes

kendallgassner changed the title ~~create rule: a11y-no-sr-only-class-when-focusable~~ create rule: github/a11y-no-visually-hidden-interactive-element` Jul 7, 2023

kendallgassner changed the title ~~create rule: github/a11y-no-visually-hidden-interactive-element`~~ create rule: github/a11y-no-visually-hidden-interactive-element Jul 7, 2023

kendallgassner commented Jul 7, 2023

View reviewed changes

kendallgassner requested a review from khiga8 July 7, 2023 17:19

khiga8 reviewed Jul 7, 2023

View reviewed changes

theinterned approved these changes Jul 7, 2023

View reviewed changes

add docs and htmlPropName

7f76868

khiga8 reviewed Jul 7, 2023

View reviewed changes

khiga8 approved these changes Jul 7, 2023

View reviewed changes

kendallgassner added 2 commits July 10, 2023 16:37

add test and add to react config

982b103

update docs

c916e88

kendallgassner merged commit 1e0d857 into main Jul 10, 2023
6 checks passed

kendallgassner deleted the kendallg/sr-only branch July 10, 2023 18:06

This was referenced Aug 30, 2023

Update devDependencies 24webcomponents/typing-test-element#13

Open

fix(deps): update npm tetra-fox/ml-gen#133

Open

renovate bot mentioned this pull request Dec 1, 2023

Update eslint Stompf/ms-teams-deploy-release-card#457

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create rule: github/a11y-no-visually-hidden-interactive-element #446

create rule: github/a11y-no-visually-hidden-interactive-element #446

kendallgassner commented Jul 5, 2023 •

edited

kendallgassner Jul 5, 2023

khiga8 Jul 5, 2023

khiga8 left a comment

kendallgassner commented Jul 6, 2023

kendallgassner commented Jul 6, 2023

khiga8 commented Jul 6, 2023

kendallgassner Jul 7, 2023

khiga8 Jul 7, 2023

kendallgassner Jul 7, 2023

khiga8 left a comment

khiga8 Jul 7, 2023

khiga8 Jul 7, 2023

khiga8 Jul 7, 2023 •

edited

khiga8 Jul 7, 2023

kendallgassner Jul 7, 2023

theinterned left a comment

kendallgassner commented Jul 7, 2023 •

edited

khiga8 Jul 7, 2023

kendallgassner Jul 10, 2023

khiga8 left a comment


		## Rule Details

		This rule guards against visually hiding interactive elements. If a sighted keyboard user navigates to an interactive element that is visually hidden they might become confused and assume that keyboard focus has been lost.

create rule: github/a11y-no-visually-hidden-interactive-element #446

create rule: github/a11y-no-visually-hidden-interactive-element #446

Conversation

kendallgassner commented Jul 5, 2023 • edited

What

Coverage

What is an interactive element?

What is considered invalid?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 left a comment

Choose a reason for hiding this comment

kendallgassner commented Jul 6, 2023

kendallgassner commented Jul 6, 2023

khiga8 commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theinterned left a comment

Choose a reason for hiding this comment

kendallgassner commented Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 left a comment

Choose a reason for hiding this comment

kendallgassner commented Jul 5, 2023 •

edited

khiga8 Jul 7, 2023 •

edited

kendallgassner commented Jul 7, 2023 •

edited