Linter: Implement a11y-no-visually-hidden-interactive-elements rule

[joelhawksley]

Implements the a11y-no-visually-hidden-interactive-elements rule from erblint-github's NoVisuallyHiddenInteractiveElements.

This rule flags interactive elements (a, button, summary, select, option, textarea) that have the sr-only CSS class, which visually hides them. Sighted keyboard users navigating to a visually hidden interactive element may become confused, thinking keyboard focus has been lost.

Note: input elements are intentionally not flagged to avoid false positives (e.g. file inputs).

Closes #1225

[brunoprietog]

Wouldn't this rule incorrectly flag things like sr-only focus:not-sr-only?

[joelhawksley]

@brunoprietog that's a good point! The original rule from rubocop-github did not account for those patterns. I went ahead and added tests and a fix and put your name on the commit ❤️

[brunoprietog]

Great! ❤️

I'm not sure if it should be enabled by default, since we're assuming you'd be using Tailwind here, right?

Generally, when I define the class in my projects, I do it like this:

.visually-hidden:not(:focus):not(:active):not(:focus-within) {
  clip-path: inset(50%);
  height: 1px;
  overflow: clip;
  position: absolute;
  white-space: nowrap;
  width: 1px;
}

And that way, you always avoid the problem. You could call it sr-only, too.

But if you use Bootstrap, it could also apply to visually-hidden, because the other version is visually-hidden-focusable.

Or maybe make the class name configurable in some way?

[joelhawksley]

@brunoprietog forgive me if I'm not following, but I think we can the accommodation for Tailwind and not worry whether folks are using it or not. As for whether to allow for configuring other classes, I think that's a good idea but maybe @marcoroth can decide? I assume we'd just allow a list to be passed to the rule configuration.

[brunoprietog]

Oh I'm sorry, I think I didn't explain myself clearly. What I meant was that this actually depends a lot on whether you're using Tailwind or not, in the sense that you could create your own sr-only class, which would be the same as the class I showed above with the :not(:focus):not(:active):not(:focus-within) pseudo classes, and in that case, it wouldn't really be a problem to apply that class to interactive elements.

Similarly, if we added the visually-hidden Bootstrap class to the list along with sr-only, it would be somewhat similar.

[joelhawksley]

@brunoprietog I see what you mean now. Yes, this rule depends on what the class actually does in your application. I think it's fine to leave this as disabled by default, but maybe we should add some docs to clarify your concern. Would you be up for suggesting changes on this PR?

[brunoprietog]

I have been thinking about this more and I have some doubts I wanted to share.

Whether sr-only is a problem depends on the CSS, not the HTML. If the class is defined to reveal the element on focus (like the example I shared before, with :not(:focus):not(:active):not(:focus-within)), then using it on interactive elements is never a problem, no matter where it is used. So the real fix is to define the class correctly once in CSS, instead of checking every usage in the templates. If you control the CSS, this rule does not add much.

However, Tailwind is very widely used, and there sr-only keeps the element permanently hidden, and you have to reveal it per element (focus:not-sr-only). In that case the fix really is per usage, so the rule is useful.

But this also means it is a Tailwind-specific rule, not a general a11y one. It will not catch Bootstrap, plain CSS, or a custom sr-only that reveals on focus, while the name a11y-no-visually-hidden-interactive-elements looks general.

So I have two questions:

• Is it worth keeping, considering the CSS fix is better when you control the CSS? I think yes, mainly because Tailwind is so widely used.
• If we keep it, should it be scoped and named to make clear that it is really about Tailwind's sr-only, instead of looking like a general rule? There is already precedent for context-specific rules (actionview-*).

What do you think?

[brunoprietog]

Suggested change

	Note: `input` elements are not flagged at this time as some visually hidden inputs might cause false positives (e.g. file inputs).
	Note: `input` elements are not flagged at this time as some visually hidden inputs might cause false positives (e.g. file inputs).
	## When to enable
	This rule is specific to Tailwind in practice: it detects Tailwind's `sr-only` class and its `*:not-sr-only` focus-reveal variants. It is disabled by default because whether `sr-only` is a problem depends on how the class is defined in CSS.
	In Tailwind, `sr-only` keeps the element permanently hidden, so revealing it on focus has to be added per element (`focus:not-sr-only`), which is what this rule checks for. If instead you define your own `sr-only` that reveals on focus (e.g. `.sr-only:not(:focus):not(:active):not(:focus-within)`), then using it on interactive elements is not a problem and you can keep the rule disabled.
	Class names also differ across frameworks (e.g. Bootstrap's `visually-hidden` / `visually-hidden-focusable`), which this rule does not currently handle.

[brunoprietog]

Since the rule already works on Tailwind's conventions, it might be better to handle its focus variants more completely. Instead of an explicit list, matching the semantics (any focus-triggered not-sr-only) would be more robust:

const VISUALLY_SHOWN_ON_FOCUS = /(?:^|:)(?:group-)?focus(?:-visible|-within)?:not-sr-only$/
const isUndoClass = (cls: string) => cls === "not-sr-only" || VISUALLY_SHOWN_ON_FOCUS.test(cls)

And then !classes.some(isUndoClass) below. This covers focus, focus-visible, focus-within, group-focus-within, and responsive prefixes like md:focus:not-sr-only at once. It still excludes hover: and active:, because those do not reveal the element for keyboard users, so the offense should still fire.

[joelhawksley]

• If we keep it, should it be scoped and named to make clear that it is really about Tailwind's sr-only, instead of looking like a general rule? There is already precedent for context-specific rules (actionview-*).

What do you think?

Funny you should mention this re: Tailwind, when we don't use it at GitHub! It just so happens that our implementation is similar in this regard.

What do you think about defaulting to sr-only and allowing the class to be configured?

[brunoprietog]

Yes I suppose so, but is that possible?

Just out of curiosity, is there a reason why you haven't fixed that at the source in the CSS on GitHub instead of using the rule? Or maybe there's something I'm not taking into account

[joelhawksley]

Just out of curiosity, is there a reason why you haven't fixed that at the source in the CSS on GitHub instead of using the rule? Or maybe there's something I'm not taking into account

I'm on paternity leave at the moment, so I can't answer that question 😄

@marcoroth how do you think we should proceed here?