Skip to content

Conversation

@ahosgood
Copy link

@ahosgood ahosgood commented May 9, 2025

Description

Fixes HTML validation

Specific Changes proposed

Maintain the role of "menu" on all MenuButton components which removes the HTML validation issue of:

An element with role=menuitemradio must be contained in, or owned by, an element with role=menubar or role=menu or role=group

...as MenuItem components can have a role.

Instead, add aria-hidden="true" on the MenuButton components without items to hide them from assistive technologies.

Tested in https://validator.w3.org/ with the included sandbox example.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
    • Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
    • Has no changes to JSDoc which cause npm run docs:api to error
  • Reviewed by Two Core Contributors

Added a role of "group" to all MenuButton components removes the HTML validation issue of "An element with role=menuitemradio must be contained in, or owned by, an element with role=menubar or role=menu or role=group" as MenuItem components can have a role.
@welcome
Copy link

welcome bot commented May 9, 2025

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@ahosgood ahosgood changed the title fix: add role="group" to MenuButton for valid HTML fix: invalid HTML May 9, 2025
opa26033

This comment was marked as spam.

@mister-ben
Copy link
Contributor

Menus that are not disabled do have role="menu". This would recreate the problem fixed in #7699

@ahosgood
Copy link
Author

ahosgood commented Dec 8, 2025

Removing role="menu" when the menu is not used doesn't feel like a robust solution because it causes this invalid HTML.

I feel better options would be either to add aria-hidden="true" to any of the menus that are disabled or just to remove the menu element from the HTML entirely.

@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.32%. Comparing base (46779e8) to head (2a76f82).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9040   +/-   ##
=======================================
  Coverage   84.32%   84.32%           
=======================================
  Files         120      120           
  Lines        8153     8154    +1     
  Branches     1964     1964           
=======================================
+ Hits         6875     6876    +1     
  Misses       1278     1278           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants