Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Restrict aria roles by element type #4607

Merged
merged 13 commits into from
Mar 23, 2025

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Dec 13, 2024

Starting to take a look at #3853, seems role restriction should be simple enough for most elements. Spec Doc

Edit: Unfortunately there's some rare situations in which the spec should be ignored which presents an issue here: adding stricter types might dissuade people against using the correct role on some elements because the spec (and therefore our types) don't allow it. I'd like to guess that this occurs far less often than a user simply reaching for the wrong roles, however, hence this PR.


There are some roles that we can't limit via types, mainly due to descendant restrictions ("If not a descendant of <article>, <aside>, <main>, ..., etc.) or specific attribute values ("If size attribute is greater than 1"), but this does get most elements/roles.

With this I think I'm all typed out for the foreseeable future 😅

@coveralls
Copy link

coveralls commented Dec 13, 2024

Coverage Status

coverage: 99.615%. remained the same
when pulling f66acff on refactor/aria-role-subset
into bb68456 on main.

@rschristian rschristian force-pushed the refactor/aria-role-subset branch from 3bb1113 to 4fb26f6 Compare December 15, 2024 21:23
@rschristian rschristian changed the title refactor: Restrict aria roles by element type refactor: Restrict aria roles by element type (simple cases only) Dec 16, 2024
@rschristian rschristian marked this pull request as ready for review December 16, 2024 01:56
@rschristian rschristian force-pushed the refactor/aria-role-subset branch from e52b2b0 to 29a30e0 Compare December 16, 2024 02:04
@rschristian rschristian marked this pull request as draft December 16, 2024 21:44
alt?: Signalish<string | undefined>;
coords?: Signalish<string | undefined>;
download?: Signalish<any>;
href?: Signalish<string | undefined>;
Copy link
Member Author

@rschristian rschristian Dec 17, 2024

Choose a reason for hiding this comment

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

When an incorrect role is applied, if we're restricting via more complex intersection types, our Signalish helper seems to cause error messages to blow up if the particular intersection key can be found on the base interface.

This isn't an issue in all cases, but to be somewhat consistent, I've stripped these from the base interfaces. Take the following example:

<input type="checkbox" role="slider" /> // I'm invalid!

With type?: Signalish<HTMLInputTypeAttribute | undefined> set, you get this error:

Type '{ type: "checkbox"; role: "slider"; }' is not assignable to type 'InputHTMLAttributes<HTMLInputElement>'.
  Types of property 'type' are incompatible.
    Type '"checkbox"' is not assignable to type '(Signalish<HTMLInputTypeAttribute | undefined> & Signalish<"button">) | (Signalish<HTMLInputTypeAttribute | undefined> & Signalish<...>) | (Signalish<...> & Signalish<...>) | (Signalish<...> & Signalish<...>) | (Signalish<...> & Signalish<...>)'. (tsserver 2322)

Without, you get this:

Type '{ type: "checkbox"; role: "slider"; }' is not assignable to type 'InputHTMLAttributes<HTMLInputElement>'.
  Types of property 'type' are incompatible.
    Type '"checkbox"' is not assignable to type 'Signalish<"button"> | Signalish<"image"> | Signalish<"range"> | Signalish<"reset"> | Signalish<"submit">'. (tsserver 2322)

Not an expert here, but the latter is a lot better.

src/jsx.d.ts Outdated
> &
AnchorAriaRoles;

interface PartialAreaHTMLAttributes<T> extends HTMLAttributes<T> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Partial... might be a poor name, very much open to suggestions.

src/jsx.d.ts Outdated
Comment on lines 2509 to 2394
// Spec states this branch is limited to "no `multiple` attribute AND no `size` attribute greater than 1".
// We can't really express that in TS though so we'll just ignore `size` for now.
//size?: never;
Copy link
Member Author

Choose a reason for hiding this comment

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

Worth calling out, we can't quite adhere to this spec but might be better than nothing. Can also just revert too, treat it like others we can't support via types and just allow any aria role.

@rschristian rschristian changed the title refactor: Restrict aria roles by element type (simple cases only) refactor: Restrict aria roles by element type Dec 17, 2024
@rschristian rschristian marked this pull request as ready for review December 17, 2024 05:10
@JoviDeCroock
Copy link
Member

@rschristian Let's target this one at v11-2 as well

@rschristian
Copy link
Member Author

Worried it might be breaking? I have no strong opinion either way, though with how verbose this is, it might make v11 look a bit messier.

@rschristian rschristian force-pushed the refactor/aria-role-subset branch from f66acff to eef3c0d Compare February 14, 2025 06:13
@rschristian rschristian changed the base branch from main to v11-2 February 14, 2025 06:13
@preactjs preactjs deleted a comment from github-actions bot Feb 14, 2025
@preactjs preactjs deleted a comment from github-actions bot Feb 14, 2025
@rschristian rschristian force-pushed the refactor/aria-role-subset branch from baa98ab to 7d16078 Compare February 14, 2025 07:20
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This should allow us to close out the #3853 issue I reckon

@rschristian
Copy link
Member Author

Hm, I don't think so.

While this does take a big first step, it's limited to aria roles; it does not attempt to provide (restricted) types for any of the myriad of other aria attributes that are available.

@JoviDeCroock JoviDeCroock force-pushed the v11-2 branch 4 times, most recently from 49c0171 to d1aa1d3 Compare February 19, 2025 19:51
@rschristian rschristian marked this pull request as draft February 26, 2025 23:16
@rschristian rschristian changed the base branch from v11-2 to main February 26, 2025 23:16
@rschristian rschristian changed the base branch from main to v11-2 February 26, 2025 23:17
@rschristian rschristian force-pushed the refactor/aria-role-subset branch 2 times, most recently from 3a33142 to 8f273f2 Compare February 26, 2025 23:29
@preactjs preactjs deleted a comment from github-actions bot Feb 26, 2025
@rschristian rschristian marked this pull request as ready for review February 26, 2025 23:31
@rschristian rschristian force-pushed the refactor/aria-role-subset branch from 8f273f2 to 9013f3b Compare February 27, 2025 08:00
@JoviDeCroock JoviDeCroock force-pushed the v11-2 branch 2 times, most recently from d8906e9 to 28038c2 Compare March 10, 2025 11:40
@rschristian rschristian changed the base branch from v11-2 to main March 13, 2025 21:43
@rschristian rschristian changed the base branch from main to v11-2 March 13, 2025 21:43
@rschristian rschristian force-pushed the refactor/aria-role-subset branch from 9013f3b to 1b72ced Compare March 13, 2025 21:44
@rschristian rschristian force-pushed the refactor/aria-role-subset branch from 1b72ced to e1c440f Compare March 18, 2025 21:04
@rschristian
Copy link
Member Author

Going to merge, everything seems good!

@rschristian rschristian merged commit 65e5f4b into v11-2 Mar 23, 2025
5 checks passed
@rschristian rschristian deleted the refactor/aria-role-subset branch March 23, 2025 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants