- Notifications
You must be signed in to change notification settings - Fork2.2k
MenuItem and QueryList ScreenReader Accessibility Improvements#7543
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Conversation
Generate changelog in |
svc-palantir-github commentedAug 13, 2025
Remove redundant optionalityBuild artifact links for this commit:documentation |landing |table |demoThis is an automated comment from the deploy-preview CircleCI job. |
svc-palantir-github commentedAug 13, 2025
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 allowingid to be passed on to MenuItem and for settingaria-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we're omittingkey fromMenuItemProps for the return type, we should also removeReact.Attributes since the intersection of that type just ends up adding it back:
interfaceAttributes{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*asReactfrom"react"; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are we defining this empty focus event handler?
| menuProps:{ | ||
| ...menuProps, | ||
| id:this.listId, | ||
| role:"listbox", |
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 defaultitemListRenderer implementation already includes this role. Do we need to include it again here?
| <Menurole="listbox"{...listProps.menuProps}ulRef={listProps.itemsParentRef}> |
| menuProps, | ||
| menuProps:{ | ||
| ...menuProps, | ||
| id:this.listId, |
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.
Won't setting anid here replace the id referenced by the outermost combobox that already definesaria-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 |
|---|---|
 |  |
| constinput=( | ||
| <InputGroup | ||
| aria-activedescendant={listProps.activeItemId} |
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.
Why are we settingaria-activedescendant on the filter input group? Shouldn't we be setting this on the outermost combobox element?
Here's the WAISelect-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