MenuItem and QueryList ScreenReader Accessibility Improvements #7543
Conversation
Generate changelog in
|
Remove redundant optionalityBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Merge branch 'develop' into cmcdougall/accessibility_changes_menu_itemBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
There was a problem hiding this comment.
I'm a bit confused as to what the specific accessibility improvements here are. Can you provide some examples of what you're trying to achieve?
Generally supportive of allowing id to be passed on to MenuItem and for setting aria-activedescendant (though I have questions regarding how this is currently implemented).
| { handleClick, handleFocus, modifiers, query, ref }: ItemRendererProps, | ||
| ): MenuItemProps & React.Attributes { | ||
| itemProps: ItemRendererProps, | ||
| ): Omit<MenuItemProps, "key"> & React.Attributes { |
There was a problem hiding this comment.
If we're omitting key from MenuItemProps for the return type, we should also remove React.Attributes since the intersection of that type just ends up adding it back:
interface Attributes {
key?: Key | null | undefined;
}| ): Omit<MenuItemProps, "key"> & React.Attributes { | |
| ): Omit<MenuItemProps, "key"> { |
I'm supportive of this fix for the docs examples, but could we split that part out into its own PR?
| * limitations under the License. | ||
| */ | ||
|
|
||
| import * as React from "react"; |
There was a problem hiding this comment.
This React import can be removed:
| import * as React from "react"; |
| super(props); | ||
|
|
||
| // Generate unique ID for accessibility | ||
| this.listId = props.listId ?? `bp5-query-list-${Utils.uniqueId("ql")}`; |
There was a problem hiding this comment.
Suggest removing the hardcoded version prefix and replacing with the following:
| this.listId = props.listId ?? `bp5-query-list-${Utils.uniqueId("ql")}`; | |
| this.listId = props.listId ?? Utils.uniqueId("bp-query-list"); |
| // When input gains focus, we don't immediately set visual focus state | ||
| // Visual focus state is only set when the user navigates with arrow keys | ||
| // This ensures screen readers only announce activedescendant when appropriate | ||
| }; |
There was a problem hiding this comment.
Why are we defining this empty focus event handler?
| menuProps: { | ||
| ...menuProps, | ||
| id: this.listId, | ||
| role: "listbox", |
There was a problem hiding this comment.
The default itemListRenderer implementation already includes this role. Do we need to include it again here?
| menuProps, | ||
| menuProps: { | ||
| ...menuProps, | ||
| id: this.listId, |
There was a problem hiding this comment.
Won't setting an id here replace the id referenced by the outermost combobox that already defines aria-controls? I'm a bit confused as to what we're trying to accomplish. Here's a before/after of these changes on a simple example to illustrate:
| Before | After |
|---|---|
 |
 |
|
|
||
| const input = ( | ||
| <InputGroup | ||
| aria-activedescendant={listProps.activeItemId} |
There was a problem hiding this comment.
Why are we setting aria-activedescendant on the filter input group? Shouldn't we be setting this on the outermost combobox element?
Here's the WAI Select-Only Combobox Example for reference:
Fixes #0000
Checklist
Changes proposed in this pull request:
ScreenReader accessibility improvements for the ItemRenderer, MenuItem, QueryList and Select components
Added proper ARIA relationships and controls:
Improved focus management in QueryList:
Updated films example:
Reviewers should focus on:
Ensuring this allows for backwards compatibility with other users of the component.
Screenshot