Updating Button handling of aria-pressed

[andrewhayward]

What?

This PR gives the Button component greater flexibility in how isPressed is handled, by looking for a role and setting the appropriate ARIA state.

Why?

When the isPressed prop is used on Button, aria-pressed is set accordingly. But when Button is used with a role that is not button (the default), aria-pressed is not the appropriate attribute to set.

How?

By checking for a role property, we can pick the appropriate attribute to set on the rendered button, rather than always setting aria-pressed. This also allows consumers to override the attribute value if they need to use isPressed and the corresponding attribute differently.

Testing Instructions

Unit tests have been included to cover the relevant roles.

[brookewp]

Since the Button component can be given an href, should there also be a case for role="link" with aria-current?

[andrewhayward]

Potentially, but aria-current can actually be used for (almost) any item "within a container or set of related elements", so special-casing it here might not make sense. Happy to discuss though.

[brookewp]

Are there scenarios where isPressed is true with aria-selected and aria-checked set to false for roles like option and checkbox? I'm curious!

This also made me wonder if we could check the value of aria-selected, aria-checked , etc. via userEvent to test closer to how it'll be used in the DOM. 🤔

[andrewhayward]

I've been thinking about this a bit since I put the PR up, and I'm wondering if overloading isPressed is actually the right direction to be going in. Should we in fact be checking the role and then looking for a corresponding is*?

type booleanish = boolean | 'true' | 'false';

type ButtonProps = {
  ...
} & (
  | {
      role?: 'button';
      isChecked?: never;
      isPressed?: booleanish | 'mixed';
      isSelected?: never;
  }
  | {
      role: 'checkbox' | 'menuitemcheckbox';
      isChecked?: booleanish | 'mixed';
      isPressed?: never;
      isSelected?: never;
    }
  | {
      role: 'menuitemradio' | 'radio' | 'switch';
      isChecked?: booleanish;
      isPressed?: never;
      isSelected?: never;
    }
  | {
      role: 'gridcell' | 'row' | 'tab';
      isChecked?: never;
      isPressed?: never;
      isSelected?: booleanish;
  }
  | {
      role: 'option';
      isChecked?: booleanish;
      isPressed?: never;
      isSelected?: booleanish;
  }
);

✅ <Button isPressed />
⛔️ <Button isSelected />
✅ <Button role="option" isSelected />
⛔️ <Button role="tab" isPressed />
✅ <Button role="checkbox" isChecked />
⛔️ <Button role="radio" isChecked="mixed" />

Reinforcing more appropriate semantics does lose a little bit of flexibility, but how often does a role actually change on the fly? It's pretty difficult to know how much impact this would have (so a potentially breaking change), but we don't seem to use role at all with Button internally.

Do you have any thoughts @ciampo @brookewp @chad1008?

[ciampo]

The approach highlighted above would definitely enforce better semantics, but I'm not sure that adding even more props to a component that has already many props would be the right approach. That would also imply that, as the HTML spec gets updated, we would need to keep the component up to date.

Another approach that came to mind, instead, could be to deprecate the isPressed prop, in favor of using the aria-pressed prop directly. Style-wise, we could also add pressed styles when the relevant aria-* attributes are specified, or keep adding the is-pressed classname for backwards compat reasons.

Even if out of scope for this PR specifically, we could then use TypeScript to indicate which aria-* attributes should be used based on the role` prop. The alternative there is to do nothing and rely on:

• accessibility testing
• ready-made, higher level component that already come with the correct role and aria-* attributes.

[andrewhayward]

The approach highlighted above would definitely enforce better semantics, but I'm not sure that adding even more props to a component that has already many props would be the right approach. That would also imply that, as the HTML spec gets updated, we would need to keep the component up to date.

Another approach that came to mind, instead, could be to deprecate the isPressed prop, in favor of using the aria-pressed prop directly.

I'd be fine with deprecating isPressed in favour of using the standard aria-* props, but I suppose that would still bring with it the need to keep the component up to date. I don't think doing so would be a huge burden though, given that we wouldn't actually have to do it until it became a problem. As a general rule, the spec aims to be backwards compatible, so it's more likely that we'd be in a position where something was unachievable rather than broken.

Style-wise, we could also add pressed styles when the relevant aria-* attributes are specified, or keep adding the is-pressed classname for backwards compat reasons.

I don't know if anything uses the is-pressed class as a hook, but we could do any combination of continuing to use it (with is-checked/is-selected, as appropriate), and adding [aria-pressed="true"], [aria-checked="true"], [aria-selected="true"] to the style definition.

Even if out of scope for this PR specifically, we could then use TypeScript to indicate which aria-* attributes should be used based on the role` prop.

How do you see that working?

The alternative there is to do nothing and rely on:

• accessibility testing
• ready-made, higher level component that already come with the correct role and aria-* attributes.

I'm not aware of any high-level library that does this specifically, but it might exist somewhere. Would need a bit of further research. TypeScript exposes interfaces for the HTML DOM API, and the various ARIA attributes, but I don't think either provide sufficient information for us to piggy back.

There's always the JSX A11y ESLint plugin as an option, but that feels somewhat counter to using TypeScript! Maybe we need to port that to TypeScript as a separate package!

[ciampo]

After a quick chat, the agreed next steps are:

• depreacte isPressed (while keeping its functionality). Users should use the aria-pressed attribute instead
• when the aria-pressed attribute is set to true, the component internally should apply the same visual changes as when the isPressed prop is used

[andrewhayward]

Closing in favour of #54740.