RocketChat / RocketChat/Rocket.Chat

[Refactor] Deprecate 'button' prop in GenericMenu to enforce semantic HTML

Open
#39,811 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

type: feature
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
  1. Inspect the source code of packages/ui-client/src/components/GenericMenu/GenericMenu.tsx (Line 52).
  2. Note the permissive button prop and the associated developer FIXME comment.
  3. 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
  1. Completely deprecate the button prop.
  2. Replace it with an explicit icon or label prop, and render a built-in standard <button> element inside GenericMenu to handle the trigger events.
  3. Refactor existing implementations across the codebase that rely on the old button prop.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.