Add a check to the tooltip interactive trigger rule to exclude elements that have disabled attribute (reference)

Slack ref (GitHub stuff only link)

A no-theme-access-out-of-context rule would keep you from doing this:

import {theme, Box} from '@primer/components'

<Box width={theme.sizes.large} />

And auto-fix it to:

<Box width="large" />

It'd be great to have two specific rules that we can add to over time:

• use-preferred-imports
• use-preferred-props

These rules would encapsulate our preferred way to import components and prop usage for a component. This could help with migrating components across entrypoints or prop renames.

Summary

We often have to deal with disabled controls in Primer which are typically inaccessible to most users. If we could create a rule that would help stem that usage, then it might reduce the amount of inaccessible patterns that are commonly used with the disabled attribute.

Idea

We cannot restrict usage of the disabled attribute outright, as it may be a good option in some cases. Since there are many different nuances for how and when this attribute is used, we should restrict this rule to only be applied in specific cases.

In forms

We provide guidance against disabling save/submit buttons in forms and instead provide a reason as to why the submission isn't successful. We could potentially create a rule that throws a violation against disabled submit buttons used within a <form>, or similar form-based components.

Example of bad usage:

<form>
  <label>Name
    <input type="text" name="name" required />
  </label>

  <label>
    Bio
    <textarea name="bio" rows="5" />
  </label>
  
  <!-- The control is disabled, but provides no explanation as to why -->
  <button type="submit" disabled>Save</button>
</form>

We could warn in instances where disabled is used for the submit control, much like in the example above.

As suggestions

ESLint allows you to provide suggestions to patterns without throwing an error. We could utilize this to steer developers away from disabled, by encouraging them to look into using a more accessible pattern that will fits their needs.

Some components allow a as prop to be set. This flexibility can lead to non-semantic component usage which is very detrimental to accessibility.

For example, a <Heading> component should only ever render with a heading tag. However, it can currently be rendered with any tag like <p>.

Until a system-level update is made to define an allowlist for each component that accepts prop, we should restrict as values with a lint rule.

If we can make a system-level change, that would be preferable, but I'm not sure what work is involved for that.

Related Slack convo GitHub staff-only

Currently, when testing rules the ESLint parser does not support TypeScript syntax. It'd be great if we could support a TypeScript parser in these tests so that we can author input and output examples in TypeScript.

The component to tag mappings in recommended.js have not been updated for a while, and is overdue for a review.

In addition, this library is behind on jsx-a11y and eslint-plugin-github dependency updates.

We have a mapping that is used in eslint-plugin-jsx-a11y and a mapping that is used in eslint-plugin-github. The latter should be deprecated since it was an experimental feature that is deprecated in the latest eslint-plugin-github.

• Are there any component to tag mappings that should be newly added? (Notably, jsx-a11y supports 1:1 component to tag mapping). However, we should be wary of adding some mappings that could raise a false positive. For example, if we map Avatar to img, but it already sets alt internally default, the ESLint rule may incorrectly flag that there's a missing alt.
• Are there any component to tag mappings that should be removed?

Acceptance criteria

• Bump eslint-plugin-github and eslint-plugin-jsx-a11y dependency to the latest
• Ensure that the configs are up-to-date.

Based on our team strategy of rolling out breaking accessibility remediations, we need a supporting eslint rule for Tooltip. The rule will suggest using the new accessible tooltip rather than the one in the main bundle.

From

import { Tooltip } from '@primer/react'

to

import { Tooltip } from '@primer/react/next'

There are scenarios where folks may use TriangleDownIcon inside of trailingVisual instead of trailingAction. This rule should lint against usage in trailingVisual and recommend/auto-fix the usage to be trailingAction. This is for the Button component.

I think it'd be helpful to have a linter that suggests a more appropriate element based on the as prop.

I've noticed instances of <Box as="h1"> (or some other heading level). I think this is a misuse of Box, and a linter could suggest using a <Heading>. If there are other common component misuse, a linter could catch and suggest a more appropriate component.

This isn't directly tied to accessibility, but components being used in a predictable manner is important.

We should never allow aria-label or aria-labelledby on roles that do not support it. I've noticed instances of <Text aria-label> and <Box aria-label> in the codebase, which should be flagged.

I think we could flag inappropriate aria-label misuse based on what the as is set as, the role, and the default tag for the component.

Examples of what should be flagged:

<Heading aria-label="Some text">
<Box as="p" aria-label="Some text">
<Text aria-labelledby="some-id">

eslint-plugin-jsx-a11y has limited support for linting (polymorphic) components so unfortunately we aren't getting the a11y linting we need. We could get around this with a custom rule in this plugin.

Example:

             <Header>
                <Header.Item>
                     // Rule does not detect the fontSize
                    <Header.Link href="#" fontSize={2}>
                        // Rule detects the mr
                        <StyledOcticon icon={MarkGithubIcon} size={32} mr={2} />
                        <span>GitHub</span>
                    </Header.Link>
                </Header.Item>
            </Header>

To increase the adoption of our accessible/improved components, we want to add a lint rule that warns consumers if they stay using the deprecated version of the components.

We want to move away from

import { Tooltip } from '@primer/react/deprecated'

to

import { Tooltip } from '@primer/react/experimental'

Currently <Heading> defaults to h3 so consumers don't need to explicitly set as.

Default example:

<Heading>Some heading</Heading>

Should heading as always be set to ensure consumers are intentional, rather than obscuring by default?

Here is an example with the same default heading but with level explicitly set:

<Heading as="h3">Some heading</Heading>

cc: @TylerJDev