Skip to content

ARIA Mixin Properties should be *specific* #1184

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

Closed
chrisdholt opened this issue Oct 21, 2021 · 3 comments
Closed

ARIA Mixin Properties should be *specific* #1184

chrisdholt opened this issue Oct 21, 2021 · 3 comments

Comments

@chrisdholt
Copy link
Member

Copying over from microsoft/TypeScript#46456 as I see that these are generated files.


Bug Report

In TypeScript v4.4.0 the ARIAMixin was introduced, which enumerates all properties of the ARIAMixin as string. The FAST team (and subsequently anyone leveraging our Foundation components) is currently blocked from upgrading beyond [email protected] due to these types. The feedback from #45047 calls out the issue that these should be nullable, but we're hitting a somewhat different issue as well. Prior to the introduction of these types, we enumerated these ourselves due to specific requirements around our custom elements which delegate focus. With the introduction of ARIAMixin and the broad definition of strings in 4.4.0 we're hitting type definitions.

While each of these properties are a DOMString, many are specific strings. I don't have a direct link to each in the spec on hand, but I'll link to MDN's enumeration of these (ariaAtomic, ariaAutocomplete, ariaLive, ariaHidden, ariaHasPopup as a few examples). While many of the properties on the Mixin are strings not all are - from a standards and accuracy perspective, I think the typings should be updated to represent specific strings where applicable.

I would be more than happy to contribute a fix for this if you like.

🔎 Search Terms

ARIAMixin - As noted above, #45047 addresses the nullable nature of these, but there are no relevant issues regarding the specific typed DOMString's.

🕗 Version & Regression Information

This changed and is present in all versions greater than 4.3.5 (beginning 4.4.0).

💻 Code

const myElement = document.createElement('div');\

myElement.ariaAtomic = "foo" // not allowed, currently allowed

myElement.ariaAtomic = "true" // ok

myElement.ariaAtomic = "false" // ok

myElement.ariaAtomic = undefined; // ok

🙁 Actual behavior

Any string is valid. Anyone enumerating typed DOMStrings for these will get type errors due to one being essentially a type which !== generic string.

🙂 Expected behavior

The types map to the specification, where properties with specific DOMString values allowed are enumerated and nullable.

@orta
Copy link
Contributor

orta commented Oct 21, 2021

While this is nuanced, I think one way to think about it is: Will the amount of possible strings change with time? If so, it's best to keep them as strings (so that we don't cause compatibility issues given that these are globals) if not, then it should be good to switch to a tighter set of literals.

E.g. your example of "true" | "false" | undefined makes sense to switch ( I doubt a "maybe" string is coming down the line) but ariaCurrent which has "page" | "step" | "location" | ...could easily get one more.

We only ship types for the latest version of the specs and by setting a hardcoded version it's ambiguous whether all of the literal are supported by your runtime.

@chrisdholt
Copy link
Member Author

Thanks for the response, I agree it's nuanced and I can definitely appreciate the constraints of wanting to be both backward and forward compatible here. Does anything come to mind as to how this we could coerce or cast these to keep our specific typings (we find them helpful for users to ensure accessible UI) without ending up in conflict with the ARIAMixin provided OOTB by TypeScript?

@chrisdholt
Copy link
Member Author

@orta I believe that any issues will resolved with the update of the ARIAMixin to be nullable. I'm going to go ahead and close this. Thanks!

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

No branches or pull requests

2 participants