-
Notifications
You must be signed in to change notification settings - Fork 235
docs(tray): improve a11y of tray docs examples #5888
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
docs(tray): improve a11y of tray docs examples #5888
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
caseyisonit
left a comment
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.
Gorg
Rajdeepc
left a comment
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.
Good fix!! One thing was to test that updateAriaSelected() is also called whenever the selected property changes which I see it is happening on
if (changes.has('selected')) {
this.updateAriaSelected();
}
1st-gen/packages/menu/menu-item.md
Outdated
|
|
||
| #### Roles and ARIA attributes | ||
|
|
||
| When a menu item with `role="menuitem"` is selected, the `aria-current="true"` attribute is automatically applied to indicate the current item to screen reader users. This ensures assistive technology properly announces the selected state of menu items. |
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.
The appropriate way to indicate a selected state for an item in a menu is to use either role="menuitemcheckbox" for multi-selectable items, or role="menuitemradio" for mutually exclusive selection, with the aria-checked attribute to indicate the selected state. An element with role="menuitem" does not support the aria-checked or aria-selected property. See the supported states and properties for a menu item: https://w3c.github.io/aria/#menuitem.
An menuitem can support aria-current, but only in an appropriate context. See https://w3c.github.io/aria/#aria-current, which explains appropriate usage of aria-current:
aria-current state
Indicates the element that represents the current item within a container or set of related elements.The aria-current attribute is a token type. Any value not included in the list of allowed values SHOULD be treated by assistive technologies as if the value true had been provided. If the attribute is not present or its value is the empty string or undefined, the default value of false applies and the aria-current state MUST NOT be exposed by user agents or assistive technologies.
The aria-current attribute is used when an element within a set of related elements is visually styled to indicate it is the current item in the set. For example:
- A page token used to indicate a page within a set of pages, where the element is visually styled to represent the current page.
- A step token used to indicate a step within a step-based process, where the element is visually styled to represent the current step.
- A location token used to indicate the element that is visually styled as the current component, such as within a flow chart.
- A date token used to indicate the current date within a calendar or other date collection.
- A time token used to indicate the current time within a timetable or other time collection.
Authors SHOULD only mark one element in a set of elements as current with aria-current.Authors SHOULD NOT use the aria-current attribute as a substitute for aria-selected in widgets where aria-selected has the same meaning. For example, in a tablist, aria-selected is used on a tab to indicate the currently-displayed tabpanel.
Note how the WAI-ARIA spec explicitly states that "Authors SHOULD NOT use the aria-current attribute as a substitute for aria-selected in widgets where aria-selected has the same meaning."
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.
Let me rework this to come up with a solution that better fits how you're seeing the solution!
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.
Is this an issue with the menu component or the docs themselves?
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.
Menu already correctly assigns aria-selected (for role="option" menu items) and aria-checked (for role="menuitemcheckbox" or role="menuitemradio").
However, if the selects property is not defined, the default role for our menu items is menuitem, which doesn't support aria-checked/aria-selected. I think that because those menu examples on the tray docs didn't have the selects property (and therefore assigned menuitem roles to the sp-menu-items), we weren't announcing the selected state of the menu items correctly (because they don't hook into aria-checked).
I'm understanding Michael's comment below to be asking that question- if selects isn't on an sp-menu, how do we appropriately mark role="menuitem" elements as selectable (without using aria-current incorrectly).
This is what I'm imagining we would research in a separate ticket.
| if (role === 'menuitem') { | ||
| if (this.selected) { | ||
| this.setAttribute('aria-current', 'true'); | ||
| } else { | ||
| this.removeAttribute('aria-current'); | ||
| } | ||
| } |
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'd prefer to keep aria-current as a separate concern from selection.
role is determined by either the role on the containing sp-menu or the selects property on the parent sp-menu or sp-menu-group. If the selected state is indicated on a menuitem without selects="multiple", selects="single" or selects="inherit" on the parent sp-menu or sp-menu-group, we should maybe change the role for the sp-menu-item to menuitemcheckbox. This gets messy though, because then the question becomes, what then is the role for the unselected state, and how do we indicate that an unselected item is selectable?
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.
@majornista ah- thank you! I did see the note in the W3C docs about not substituting aria-current for aria-selected, but the ticket specifically called out aria-current as the solution, which is why I went this route.
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.
@majornista To immediately solve the issue, can I make sure that the menu as display in this tray has the selects property? Then we can hook into the existing aria-checked logic, I can remove the aria-current logic I added, and the screen reader announces the selected item properly (which I think was the underlying complaint in the ticket).
I could also add more documentation guidelines around ensuring the selects property is added to the parent sp-menu, and describe the accessibility errors/failures that happen regarding sp-menu-items when selects is missing.
I'm going to bring this up with our team this morning as well to chat about a solution!
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.
Update: these examples with a menu in a tray don't really make sense since they're only partial examples anyways. The team's preference is to remove the menu examples instead, resolving the ticket with documentation only 👍
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.
Should this be a separate issue for menu instead of the scope of a tray docs PR?
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 can make a new ticket! I have a local branch that should still have some of this previous work.
7b19176 to
ae793d0
Compare
| <sp-menu style="width: 100%"> | ||
| <sp-menu-item selected>Deselect</sp-menu-item> | ||
| <sp-menu-item>Select Inverse</sp-menu-item> | ||
| <sp-menu-item focused>Feather...</sp-menu-item> | ||
| <sp-menu-item>Select and Mask...</sp-menu-item> | ||
| <sp-menu-divider></sp-menu-divider> | ||
| <sp-menu-item>Save Selection</sp-menu-item> | ||
| <sp-menu-item disabled>Make Work Path</sp-menu-item> | ||
| </sp-menu> |
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.
In context, this menu doesn't really make- it's only a partial implementation and would normally be rendered within a popover instead.
After discussing with the team, we decided to leave out the tray examples with menus in favor of other content.
| </sp-button> | ||
| <sp-tray slot="click-content" has-keyboard-dismiss> | ||
| <p> | ||
| <p style="margin: 16px;"> |
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 added this solely to make it look a little nicer inside the tray.
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
ec8ab7b to
8538ccd
Compare
| <sp-tabs selected="dialog" auto label="Using tray's slot"> | ||
| <sp-tab value="dialog">Dialog</sp-tab> | ||
| <sp-tab-panel value="dialog"> | ||
| A tray has a single default `slot`. Expected content typically includes dialogs and their content, plain text, forms and/or form elements, and some native HTML elements. Always ensure that your tray's content is accessible according to WCAG standards. |
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.
Clean language, love it
| <div style="display: flex; flex-direction: column; margin: 16px;"> | ||
| <p style="margin-block-start: 0;"> | ||
| Custom content that doesn't have dismiss functionality, so the | ||
| tray detects it needs the visually-hidden dismiss buttons. |
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.
yessss kween to incorporating contextual documentation in the example!! we should flag this as a good practice to continue!
caseyisonit
left a comment
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.
GLORIOUS 🔥
8538ccd to
56a68e3
Compare
56a68e3 to
b26aa22
Compare
Description
This PR removes the menu examples found on the tray documentation page. The menu examples are partial and unrealistically used within the tray, so removing them resolves the accessibility concerns regarding
aria-current/aria-checked.Motivation and context
Screen readers were not announcing the selected state for menu items with
role="menuitem", causing confusion for users relying on assistive technology. But because our examples were not realistic examples, (menus are usually children of popovers instead), removing the menu examples from the tray docs is most accurate.Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Tray docs no longer have menu examples
i. Verify the accessibility of the
selectelement. It is implicitly associated to thelabelas a child of the label, and all options havevalueattributes.Screen reader announces selected state for menuitemGo to sp-tray documentationNavigate to the 'Anatomy' section, 'Dialog' tab, and select the 'Menu' tabActivate the "Toggle menu" button and navigate to a selected menu item using a screen reader (NVDA, JAWS, or VoiceOver)i. ~~In the bug ticket, they are specifically using NVDA on a Windows machine, so I was using AssitivLabs during development. ~~
Expect the screen reader to announce the selected/current state (e.g., "Deselect, current, 1 of 6")aria-current attribute is correctly appliedInspect a selectedsp-menu-itemwithrole="menuitem"in browser DevToolsVerify it hasaria-current="true"attributeInspect a non-selectedsp-menu-itemwithrole="menuitem"Verify it does NOT have anaria-currentattribute (not set to "false")Manually remove theselectedattribute from the first item.Verify the correspondingaria-currentattribute is removed.Other menu item roles are unaffectedTest menu items withrole="menuitemradio"androle="menuitemcheckbox"Verify they still usearia-checked(notaria-current)Test menu items withrole="option"in a listbox contextVerify they still usearia-selected(notaria-current)New unit tests pass in CI."sets aria-current to true if the item has selected property""removes aria-current if the item is deselected"New documentation for menu item is technically and grammatically accurate and follows Spectrum content standards.Device review