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

Add typings for ARIA attributes #3910

Merged
merged 5 commits into from Feb 23, 2023
Merged

Add typings for ARIA attributes #3910

merged 5 commits into from Feb 23, 2023

Conversation

andrewiggins
Copy link
Member

Related: #3853

@coveralls
Copy link

coveralls commented Feb 22, 2023

Coverage Status

Coverage: 99.548%. Remained the same when pulling 8223d00 on aria-types into a96e644 on master.

src/jsx.d.ts Outdated
| 'tree'
| 'treegrid'
| 'treeitem'
| (string & {}); // This trick allows any string to be valid here but also provides TS auto-complete behavior for the explicitly listed values here
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for allowing any string to be valid? Are there use cases for using roles outside of this list? Genuinely curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh, I'm not entirely sure. I've copied this from React. I assume it's to accommodate new roles as the spec changes? I need to check the git history of their types to see the reasoning.

Choose a reason for hiding this comment

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

I couldn't think of a reason and went back through the aria spec and didn't find anything concrete there either.

The spec doesn't change very quickly (last version was published December 2017) so having that carve out doesn't seem too pressing IMO. (That said, ARIA 1.2 should be published soon but it's also been soon for like a year now... But that still a 6 year cadence if it gets published this year.) It's also kind of a slippery slope where you could argue any of the attributes should allow for anything because the spec could change.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, missed that. Doesn't look like a reason was given, unless I'm missing part of the conversation.

I'm admittedly not a TS maximalist, so I personally have no issue with a string fallback (working auto-complete is lovely enough), but I wonder if it's better to be strict and maybe catch some typos, even if that means we might need to make alterations in the future? I'm not super knowledgeable about aria stuff though, so maybe there is a use for other roles.

Copy link
Member Author

Choose a reason for hiding this comment

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

@myasonik Yea, that makes sense. No point to reduce the usefulness of these typings just cuz anything could change lol. And like you said, the churn is super infrequent.

Thanks for digging that link up @rschristian! I dug a little further and it looks like this was the original thinking (link):

  1. It should still allow unknown roles e.g. when new roles are added but types are outdated
  2. what about fallback roles e.g. role="none presentation"?

For 1, like @myasonik was saying, I'm not worried about it anymore. But the second reason is news to me. I've never heard of "fallback roles" before. Scanning the spec, I can only find one reference to fallbacks roles in a note.

Maybe to start off we can just manually add "none presentation" as a possible role value? And in the future deal with other fallback roles as changes to the spec land? At that time we can either manually add more fallback combinations or look at more advanced TS techniques if it becomes complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with adding 'none presentation' and removing (string & {}) for now as that's my vote, but would like to hear what others think as well before merging.

Copy link

@myasonik myasonik Feb 23, 2023

Choose a reason for hiding this comment

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

Ah, I forgot about that and missed it when going back through... It's pretty uncommon but, yeah, role takes a space separated list of roles that you can use as fallbacks (https://www.w3.org/TR/wai-aria-1.1/#host_general_role)

The none presentation example given in the spec was there because none was new at the time so presentation was an appropriate fallback. But you can theoretically do it with any combination at any arbitrary length.

Though I can't imagine this being used very often, I do think allowing everything the spec allows is probably important enough to allow (string & {}) even if we agree that 'future-proofing' isn't worthwhile.

src/jsx.d.ts Outdated Show resolved Hide resolved
src/jsx.d.ts Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock merged commit 833d7de into master Feb 23, 2023
@JoviDeCroock JoviDeCroock deleted the aria-types branch February 23, 2023 06:30
JoviDeCroock added a commit that referenced this pull request Jan 16, 2024
JoviDeCroock added a commit that referenced this pull request Jan 16, 2024
* backport #3911

* backport #3906

* backport #3837

* backport #3908

* backport #3904

* backport #3905

* backport #3898

* backport #3910

* backport #3948

* backport #3941

* backport #3945

* backport #3919

* backport #3922

* backport #3921

* backport #3903

* fix lint

* update more

* debug
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.

None yet

5 participants