-
Notifications
You must be signed in to change notification settings - Fork 127
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
Generalize AccessibilityRole/AriaAttributes IDL #984
Conversation
@domenic thanks a lot for putting the time to get this ready! wonderful stuff that we are eager to try out. |
@domenic based on your comment
are we to assume this is not ready to consider merging? If so do you mind if I put a WIP label on it? In order to consider merging in the future we'll need to the RFC2119 format changed to caps. Once merged into our Editor's draft we'll need tests before we can add into a WD - the IDL section previously had tests supplied from outside the ARIA WG - would it be possible for this to happen with this change too? |
Sorry for the delay in response! Please do put a WIP label on it. I'll fix the caps RFC2119 keyword. As a status update, I did the HTML pull request in whatwg/html#4658, and this pull request seems pretty good. So at least technically I think it's ready to go, but it should still be marked as WIP until we have rough consensus on the HTML pull request. For tests, I think this PR doesn't actually introduce any new normative requirements that would need testing. Instead, it restructures the existing normative requirements in a way that allows their underlying infrastructure to be reused by HTML, in whatwg/html#4658 will definitely get tests before landing, as a required part of the WHATWG working mode; my team at Chrome is happy to commit to that. |
I have concerns about combining
My preference would be to change this back to two separate mixins: AccessibilityRole and AriaAttributes. |
@cookiecrook thanks for raising these concerns. I'd like to try to understand them better. Note that how these mixins are structured, and their name, is largely an editorial detail, and is not observable to web developers. The reason I merged these is because the specification infrastructure operates in the exact same way on both role and on the attributes. In particular, the "get/set the accessibility IDL attribute" infrastructure, and the steps for the getters/setters for each IDL attribute. See the spec text below the IDL block the corresponding HTML pull request. We could certainly re-separate them. However, this would just make all the specification text more awkward: we would need to have separate cases for To your points,
This is definitely true, but I'm not sure how it relates to the PR. Perhaps you are anticipating in the future mixing
This isn't the intention. As with any Web IDL mixin, the only actual constraints on augmenting the AOM could add lots of functionality, which could target If your concern is mostly about naming, I would be happy with any naming convention. Keeping in mind that the name is only observable to spec authors, we could call it |
Thanks. Based on that explanation, I think it'd be okay to keep them together and just rename Does that work for you? |
83a2438
to
fa7ce86
Compare
This generalizes the AccessibilityRole/AriaAttributes IDL mixins to allow them to have different behaviors per host interface. For Element, they have the reflection behavior specified, but e.g. for ElementInternals, HTML can use this framework to define different behavior.
In the process, this consolidates the two mixins into one ("AccessibilityMixin"), since this makes it easier to define their behavior uniformly, and consumers should generally not mix in one without the other.
/cc @tkent-google.
My next step is to work on a counterpart pull request on the HTML side, to prototype how we would use this more general framework for ElementInternals to allow custom elements to have native roles/states/properties. It's probably not worth reviewing this PR in too much depth before that is ready, in case I find out that something needs to be tweaked.
Diff