RocketChat / RocketChat/Rocket.Chat
[Refactor] Deprecate 'button' prop in GenericMenu to enforce semantic HTML
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 46.1k
- Forks
- 13.9k
- Avg merge
- 3d 3h
- Merged PRs (30d)
- 130
Description
📝 Description
The GenericMenu component currently exposes a button prop that allows developers to pass a custom element to act as the menu's dropdown trigger. However, there is no internal validation or type restriction guaranteeing that the provided element is actually a semantic HTML <button> (or a component that returns one).
This design flaw is actively flagged in the source code via an undocumented FIXME comment:
// FIXME: deprecate prop 'button' as there's no way to ensure it is actually a button
When a non-button element (like a <div> or <span>) is passed as a trigger, it breaks semantic HTML conventions, removes screen-reader button context, and inadvertently damages keyboard accessibility (tabbing).
🌎 Environment
- Rocket.Chat App Version: (Insert your current version, e.g., 6.x.x)
- NodeJS Version: (Insert version if running locally)
- Operating System: Windows/macOS/Linux
- Browser: All browsers
- Deployment Type: Browser / Web App
🚶 Steps to Reproduce
- Inspect the source code of
packages/ui-client/src/components/GenericMenu/GenericMenu.tsx(Line 52). - Note the permissive
buttonprop and the associated developerFIXMEcomment. - Observe that any arbitrary ReactNode can currently be passed and bound to the menu opening logic, completely bypassing semantic
<button>enforcement.
🎯 Expected Behavior
The GenericMenu should ideally restrict the button prop (or deprecate it entirely in favor of an internal native <button> or a strictly typed slot mechanism) to guarantee that the trigger element natively supports onClick, onKeyDown, aria-haspopup, and aria-expanded attributes correctly.
🛑 Actual Behavior
The component accepts any node through the unstructured button prop, creating blind spots in accessibility compliance across the UI where GenericMenu is implemented with a non-button trigger.
🛠 Possible Fix
- Completely deprecate the
buttonprop. - Replace it with an explicit
iconorlabelprop, and render a built-in standard<button>element insideGenericMenuto handle the trigger events. - Refactor existing implementations across the codebase that rely on the old
buttonprop.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in packages/ui-client/src/components/GenericMenu/GenericMenu.tsx around line 52, then search the codebase for implementations that pass the button prop. Review the existing FIXME and usages before choosing between deprecating the prop or replacing it with a built-in trigger; done means the trigger approach is consistently applied and existing usages are refactored.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react, typescript
- Domain
- accessibility, frontend
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100