feat(MenuToggle): add aria-haspopup prop - #12549
Open
Axel-DaMage wants to merge 4 commits into
Open
Conversation
Adds the aria-haspopup prop to MenuToggle with a default value of 'menu'. Updates all Select implementations across examples and demos to explicitly pass aria-haspopup="listbox" to properly reflect their semantic role, addressing issue patternfly#11792 (PF-1704).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds configurable Changesaria-haspopup support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/components/MenuToggle/MenuToggle.tsx (1)
76-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove the JSDoc comment for the
aria-haspopupprop.The comment "Default value of aria-haspopup" doesn't describe what the prop does. A more descriptive comment would help consumers understand its purpose and valid values.
📝 Suggested comment improvement
- /** Default value of aria-haspopup */ - 'aria-haspopup'?: 'menu' | 'listbox' | 'dialog' | boolean; + /** Indicates the type of popup controlled by this toggle. Defaults to 'menu'. */ + 'aria-haspopup'?: 'menu' | 'listbox' | 'dialog' | boolean;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/MenuToggle/MenuToggle.tsx` around lines 76 - 77, The JSDoc for the aria-haspopup prop in MenuToggleProps is too generic and should describe the prop’s purpose and accepted values instead of only stating a default. Update the comment near the aria-haspopup declaration in MenuToggle.tsx to explain that it indicates what kind of popup the toggle controls, and mention the allowed values used by the MenuToggle component. Keep the wording concise and aligned with the prop’s accessibility intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react-core/src/components/MenuToggle/MenuToggle.tsx`:
- Around line 76-77: The JSDoc for the aria-haspopup prop in MenuToggleProps is
too generic and should describe the prop’s purpose and accepted values instead
of only stating a default. Update the comment near the aria-haspopup declaration
in MenuToggle.tsx to explain that it indicates what kind of popup the toggle
controls, and mention the allowed values used by the MenuToggle component. Keep
the wording concise and aligned with the prop’s accessibility intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee5ada4-c051-46db-8e2e-10e12fdfc4ed
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/MenuToggle/__tests__/__snapshots__/MenuToggle.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
packages/react-core/src/components/Form/examples/FormState.tsxpackages/react-core/src/components/LoginPage/examples/LoginPageLanguageSelect.tsxpackages/react-core/src/components/MenuToggle/MenuToggle.tsxpackages/react-core/src/components/Select/examples/SelectBasic.tsxpackages/react-core/src/components/Select/examples/SelectCheckbox.tsxpackages/react-core/src/components/Select/examples/SelectFooter.tsxpackages/react-core/src/components/Select/examples/SelectGrouped.tsxpackages/react-core/src/components/Select/examples/SelectMultiTypeahead.tsxpackages/react-core/src/components/Select/examples/SelectMultiTypeaheadCheckbox.tsxpackages/react-core/src/components/Select/examples/SelectMultiTypeaheadCreatable.tsxpackages/react-core/src/components/Select/examples/SelectOptionVariations.tsxpackages/react-core/src/components/Select/examples/SelectTypeahead.tsxpackages/react-core/src/components/Select/examples/SelectTypeaheadCreatable.tsxpackages/react-core/src/components/Select/examples/SelectValidated.tsxpackages/react-core/src/components/Select/examples/SelectViewMore.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarComponentManagedToggleGroups.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarConsumerManagedToggleGroups.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarCustomLabelGroupContent.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarGroups.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarStacked.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarWithFilters.tsxpackages/react-core/src/components/Tooltip/examples/TooltipOptions.tsx
Contributor
|
/deploy-preview |
Collaborator
|
Preview: https://pf-react-pr-12549.surge.sh A11y report: https://pf-react-pr-12549-a11y.surge.sh |
thatblindgeye
requested changes
Jul 15, 2026
thatblindgeye
left a comment
thatblindgeye
left a comment
Contributor
There was a problem hiding this comment.
In addition to the file comments below:
- There's a bunch of lint errors being flagged, pretty much just putting the new aria-haspopup additions on its own line (instead of
<MenuToggle aria-haspopup... - Some snapshot tests are failing, so those just need to be updated
- For the various MenuToggle examples, can we just pass
aria-haspopup={false}to those? Those examples are more static so they don't actually open anything, so we should avoid them announcing that they popup something
| /** Adds styling which affects the size of the menu toggle */ | ||
| size?: 'default' | 'sm'; | ||
| /** Default value of aria-haspopup */ | ||
| 'aria-haspopup'?: 'menu' | 'listbox' | 'dialog' | boolean; |
Contributor
There was a problem hiding this comment.
I think we can omit the "dialog" option here for now. I can't think off the top of my head when the MenuToggle would trigger a dialog to open. We can always other the other allowed values down the line.
| badge?: BadgeProps | React.ReactNode; | ||
| /** Adds styling which affects the size of the menu toggle */ | ||
| size?: 'default' | 'sm'; | ||
| /** Default value of aria-haspopup */ |
Contributor
There was a problem hiding this comment.
Suggested change
| /** Default value of aria-haspopup */ | |
| /** Indicates what type of popup will be triggered by the menu toggle. A value of true is the same as a value of "menu". */ |
- Remove 'dialog' from aria-haspopup type definition
- Update JSDoc comment as suggested
- Fix lint errors by placing aria-haspopup on its own line
- Set aria-haspopup={false} on static MenuToggle examples
- Update snapshots
Contributor
Author
|
All the feedback addressed, thanks for the review @thatblindgeye.
|
thatblindgeye
approved these changes
Jul 28, 2026
thatblindgeye
left a comment
thatblindgeye
left a comment
Contributor
There was a problem hiding this comment.
Just needs a rebase to resolve conflicts otherwise lgtm, thank you!
thatblindgeye
requested changes
Jul 28, 2026
thatblindgeye
left a comment
thatblindgeye
left a comment
Contributor
There was a problem hiding this comment.
Actually quick update: the MenuTogglePlainCircle example file needs aria-haspopup of false on a couple other MenuToggles
Contributor
Author
|
The latest feedback has been addressed @thatblindgeye:
All checks are now passing, Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11792
Jira Issue: PF-1704
Description
As a followup to patternfly/patternfly#6561, this PR introduces the
aria-haspopupprop to theMenuTogglecomponent to improve screen reader accessibility.Changes included:
aria-haspopupprop toMenuTogglePropswith a default value of"menu".aria-haspopupattribute to the underlying<button>elements inMenuToggleBase.Selectcomponent examples and demos to explicitly passaria-haspopup="listbox", asSelectinherently behaves as a listbox rather than a standard navigation menu.Toolbar,Form,LoginPage, andTooltipthat utilizeSelectas a filter/dropdown to also explicitly passaria-haspopup="listbox"."menu"behavior for components that naturally function as menus (e.g.,Dropdown,ActionList,DataListkebab actions, etc.).MenuToggleunit test snapshots to reflect the new default attribute.Visuals
N/A - This is purely an accessibility (A11y) structural change, no visual regressions or changes are introduced.
Summary by CodeRabbit
New Features
Bug Fixes
aria-haspopupacross variants, including cases where no popup is opened.