-
Notifications
You must be signed in to change notification settings - Fork 125
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. And 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 in this pull request, and in 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
For sure! I've renamed it and will make the corresponding HTML-side changes now. (I used capitalized ARIAMixin instead of Pascal-case AriaMixin per https://w3ctag.github.io/design-principles/#casing-rules.) |
|
||
<section id="accessibilityroleandproperties-correspondence" class="normative" data-dfn-for="ARIAMixin" data-link-for="ARIAMixin"> | ||
<h2>ARIA Attribute Correspondence</h2> | ||
<p>The following table provides a correspondence between IDL attribute names and content attribute names, for use by <code>ARIAMixin</code>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to talk about content attributes here given that they're not content attributes for all consumers? From the wording in the HTML PR it seems these are really "state or properties" (that happen to match the casing conventions for content attributes and are used as such when this mixin is included by elements).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mostly just reflecting preexisting text (where the previous version of this table said "Reflected ARIA Content Attributes"), but I agree with your sentiment so I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, nope. Upon going to make the edit, I realized that because "role" is included in this table, "content attribute" is now the best super-category. ARIA defines a number of content attributes, namely role, a bunch of states, and a bunch of properties. So I think this is reasonable as-is.
There is a bigger editorial meta-issue about how ARIA seems to be largely defined in terms of content attributes instead of having underlying models, but let's not get in to that here :).
ARIA editors, I think this is ready to merge. Tests are available for the new stuff that this enables (which is specced in whatwg/html#4658) at https://github.com/web-platform-tests/wpt/blob/master/custom-elements/form-associated/ElementInternals-accessibility.html. Also, I realized I didn't make this super clear before: from the ARIA spec's perspective this is a refactoring without observable consequences. But it does support infrastructure like whatwg/html#4658. |
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 ("ARIAMixin"), since this makes it easier to define their behavior uniformly, and consumers should generally not mix in one without the other.
fa7ce86
to
f9b2ab2
Compare
Ping to ARIA editors. I've rebased this on master, and the corresponding HTML side pull request in whatwg/html#4658 for adding accessibility semantics to custom elements is blocked on this. |
Thanks for the ping, @domenic - it's looking good!
Other than those, I think this is good to merge! |
Fixed!
Although that makes some sense to me, I'm not sure how it relates to this PR; this PR just consolidates what was previously two mixins (one for role and one for everything else) into one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Wondering if we need an "Editor's Note" about the missing element reflection attributes in the mixin and the correspondence table?
Although that makes some sense to me, I'm not sure how it relates to this PR; this PR just consolidates what was previously two mixins (one for role and one for everything else) into one.
Good point. I'll open a new issue.
Thanks for all your help! Please feel free to merge at your convenience :). |
Gentle ping! I'd love to get this and the corresponding HTML spec pull request merged. |
* Generalize AccessibilityRole/AriaAttributes IDL 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 ("ARIAMixin"), since this makes it easier to define their behavior uniformly, and consumers should generally not mix in one without the other.
* Generalize AccessibilityRole/AriaAttributes IDL 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 ("ARIAMixin"), since this makes it easier to define their behavior uniformly, and consumers should generally not mix in one without the other.
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 @alice @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.
Preview | Diff