-
Notifications
You must be signed in to change notification settings - Fork 118
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
Adds support for posinset/setsize in toolbars #2170
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for going through and doing all this on a weekend! I left some suggestions, LMK if they make sense.
index.html
Outdated
@@ -9367,6 +9406,7 @@ <h5>Presentational Role Inheritance</h5> | |||
<p>The toolbar is often a subset of functions found in a <rref>menubar</rref>, designed to reduce user effort in using these functions. Authors MUST supply a label on each toolbar when the application contains more than one toolbar.</p> | |||
<p>Authors MAY manage focus of descendants for all instances of this <a>role</a>, as described in <a href="#managingfocus">Managing Focus</a>.</p> | |||
<p>Elements with the role <code>toolbar</code> have an implicit <pref>aria-orientation</pref> value of <code>horizontal</code>.</p> | |||
<p>Authors MAY set <pref>aria-setsize</pref> and <a>aria-posinset</a> on focusable interactive <a>accessibility children</a> of the role <code>toolbar</code> that don't have a <a href="#scope">required accessibility parent role</a>. Authors MUST NOT set <pref>aria-setsize</pref> and <a>aria-posinset</a> values scoped to the <code>toolbar</code> on <a href="#mustContain">allowed accessibility child roles</a> of any <rref>composite</rref> <rref>widget</rref> that already provides indexing.</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.
Couple small thoughts:
- I think it'd be good to add another paragraph stating that, unlike other composite widgets supporting these attributes, browsers do not provide implicit values for
aria-setsize
andaria-posinset
within toolbars. - I'd actually suggest removing the "that don't have a required accessibility parent role" text from the first sentence. If you use the same "accessibility children or accessibility descendants with only groups intervening" language from elsewhere, then that already rules out controls that have their own other composite widget parent.
- I might also suggest softening the "Authors MUST NOT" sentence, since I can kinda see a case where (chthulu help us) someone puts an inline menu inside a toolbar, but wants to keep the indexing flat for all controls within the toolbar. Maybe not a wonderful idea, but I'm not sure it merits a must-level spec requirement. How about something like this? "If the toolbar contains controls that already support aria-posinset and aria-setsize (such as options in a listbox or menuitems in a menu), authors SHOULD treat any provided aria-posinset and aria-setsize values on those descendants as fully independent from the toolbar scope."
I'm consolidating those into a change suggestion for ease of reviewing, but don't feel any pressure to use my wording 😅
<p>Authors MAY set <pref>aria-setsize</pref> and <a>aria-posinset</a> on focusable interactive <a>accessibility children</a> of the role <code>toolbar</code> that don't have a <a href="#scope">required accessibility parent role</a>. Authors MUST NOT set <pref>aria-setsize</pref> and <a>aria-posinset</a> values scoped to the <code>toolbar</code> on <a href="#mustContain">allowed accessibility child roles</a> of any <rref>composite</rref> <rref>widget</rref> that already provides indexing.</p> | |
<p>Authors MAY set <pref>aria-setsize</pref> and <pref>aria-posinset</pref> on focusable interactive <a>accessibility children</a> of the role <code>toolbar</code> or <a>accessibility descendants</a> with only elements with role <rref>group</rref> intervening. If the <code>toolbar</code> contains controls that already support <pref>aria-posinset</pref> and <pref>aria-setsize</pref> (such as <rref>option</rref> in a <rref>listbox</rref> or <rref>menuitem</rref> in a <rref>menu</rref>), authors SHOULD treat any provided <pref>aria-posinset</pref> and <pref>aria-setsize</pref> values on those descendants as fully independent from the toolbar scope.</p> | |
<p>Unlike other roles that support <pref>aria-setsize</pref> and <pref>aria-posinset</pref> such as <rref>listitem</rref> or <rref>menuitem</rref>, browsers are not required to calculate implicit values scoped to a <code>toolbar</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.
@smhigley This is listed as outdated but still showing up instead of collapsed. Given my lack of GitHub skills, what does this mean and how can I resolve 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.
adding the children > descendant wording suggestion to all other instances of the same paragraph
Corrects "accessibility children" to "accessibility descendants" Co-authored-by: Sarah Higley <smhigley@users.noreply.github.com>
Closes #2158
Outlines which roles within
toolbar
can acceptaria-setsize
andaria-posinset
.PR tracking
Check these when the relevant issue or PR has been made, OR after you have confirmed the
related change is not necessary (add N/A). Leave unchecked if you are unsure. Read the
Process Document or
Test Overview for more information.
Test, Documentation and Implementation tracking
Once this PR and all related PRs have been approved by the working group, tests
should be written and issues should be opened on browsers. Add N/A and check when not
applicable.
Preview | Diff