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

Stop using setAttribute for ARIA #3489

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

avelad
Copy link
Member

@avelad avelad commented Jun 25, 2021

Related to: #3378

@avelad
Copy link
Member Author

avelad commented Jun 25, 2021

I have reviewed the code and there are no more references. It is up to @joeyparrish to decide if the associated issue can be closed or not.

Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

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

I just looked it up and... unfortunately, according to MDN, Firefox does not yet implement the ARIAMixin interface mixin. See: https://developer.mozilla.org/en-US/docs/Web/API/Element
I tried using ariaSelected on Firefox to confirm, and it does appear to be the case that these don't work.

So I don't know if we can actually use this yet. I've done some searching, but I couldn't find any talk about when or if Mozilla plans to implement ARIAMixin.

I suppose we could make an ARIAMixin polyfill, which uses Object.defineProperty to add getters and setters for the various values? We've been considering trying to reduce our reliance on polyfills, though, so this might not be a good time to add a new one.

externs/aria.js Show resolved Hide resolved
@avelad
Copy link
Member Author

avelad commented Jul 1, 2021

I suppose we could make an ARIAMixin polyfill, which uses Object.defineProperty to add getters and setters for the various values? We've been considering trying to reduce our reliance on polyfills, though, so this might not be a good time to add a new one.

@theodab I'm bad at doing this kind of polyfills, could you help me with this?

@theodab
Copy link
Contributor

theodab commented Jul 1, 2021

I suppose we could make an ARIAMixin polyfill, which uses Object.defineProperty to add getters and setters for the various values? We've been considering trying to reduce our reliance on polyfills, though, so this might not be a good time to add a new one.

@theodab I'm bad at doing this kind of polyfills, could you help me with this?

Okay, I'll write a CL for that real fast. I'll update here when it has passed review.

@shaka-bot
Copy link
Collaborator

All tests passed!

@theodab
Copy link
Contributor

theodab commented Jul 7, 2021

Okay, the polyfill CL has passed review. I might as well merge now, as the lack of a polyfill was my last issue with this.
Thank you for your contributions!

@theodab theodab merged commit 3b2f7db into shaka-project:master Jul 7, 2021
@avelad avelad deleted the setAttribute-aria branch July 8, 2021 05:42
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants