refactor: Restrict aria roles by element type #4607
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
3bb1113 to
4fb26f6
Compare
rschristian
changed the title
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
e52b2b0 to
29a30e0
Compare
rschristian
commented
Dec 17, 2024
| alt?: Signalish<string | undefined>; | ||
| coords?: Signalish<string | undefined>; | ||
| download?: Signalish<any>; | ||
| href?: Signalish<string | undefined>; |
Member
Author
There was a problem hiding this comment.
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> { |
Member
Author
There was a problem hiding this comment.
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; |
Member
Author
There was a problem hiding this comment.
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
changed the title
Member
|
@rschristian Let's target this one at |
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
force-pushed
the
refactor/aria-role-subset
branch
from
f66acff to
eef3c0d
Compare
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
baa98ab to
7d16078
Compare
JoviDeCroock
approved these changes
Feb 14, 2025
Member
JoviDeCroock
left a comment
JoviDeCroock
left a comment
There was a problem hiding this comment.
This should allow us to close out the #3853 issue I reckon
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. |
rschristian
force-pushed
the
refactor/aria-role-subset
branch
2 times, most recently
from
3a33142 to
8f273f2
Compare
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
8f273f2 to
9013f3b
Compare
JoviDeCroock
force-pushed
the
v11-2
branch
2 times, most recently
from
d8906e9 to
28038c2
Compare
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
9013f3b to
1b72ced
Compare
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
1b72ced to
e1c440f
Compare
rschristian
force-pushed
the
refactor/aria-role-subset
branch
from
fa2875f to
3c4e804
Compare
Member
Author
|
Going to merge, everything seems good! |
JoviDeCroock
pushed a commit
that referenced
this pull request
Apr 8, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
May 3, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
May 3, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
May 8, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
May 23, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
May 30, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
JoviDeCroock
pushed a commit
that referenced
this pull request
Jun 6, 2025
* refactor: Restrict aria roles by element type * test: Add simple test for button roles * refactor: Make roles signalish * test: Add tests for `never` roles * refactor: Remove half-implemented select roles * fix: Ensure attributes w/ `never` roles can be constructed * refactor: Restrict more complex aria roles * refactor: Remove unused input type interface, fix select multiple type * fix: Ensure partials extend EventTarget * revert: Bring back input type attribute * test: Fix test typo * refactor: Remove 'generic' aria role from allowed values * refactor: Revise select multiple/size roles
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ("Ifsizeattribute is greater than 1"), but this does get most elements/roles.With this I think I'm all typed out for the foreseeable future 😅