Skip to content

fix(Menu): only set role=menu on the content list once it has an item - #9228

Open
nityanand123gupta wants to merge 1 commit into
videojs:mainfrom
nityanand123gupta:fix-empty-menu-aria-role
Open

fix(Menu): only set role=menu on the content list once it has an item#9228
nityanand123gupta wants to merge 1 commit into
videojs:mainfrom
nityanand123gupta:fix-empty-menu-aria-role

Conversation

@nityanand123gupta

Copy link
Copy Markdown

Problem

Menu.createEl() sets role="menu" on the content list unconditionally at element creation, before any items exist. Accessibility tools (e.g. WAVE) flag this as a broken ARIA menu whenever a menu button has nothing to show — most commonly the Captions/Chapters buttons when a video has no caption tracks or chapters, which per the issue happens on the large majority of real-world videojs instances.

Fix

Moved the role="menu" assignment out of createEl() and into a new updateMenuRole_() helper, called from both addItem and removeChild, which sets the role once there's at least one real (non-title) item and removes it when the menu is empty again. It reuses the existing "is this child just the title placeholder" check already present in focus(), since MenuButton always adds its title via addItem too — a title-only menu (the exact real-world scenario in this issue) must not get role="menu" either.

Testing

I don't have a full karma/browser test cycle completing in this environment (video.js's suite needs a full webpack/browserify/rollup build cycle that didn't finish in the time I had), so I want to be upfront about that rather than claim more than I verified. What I did confirm:

  • npx rollup -c builds clean with this change — no syntax/bundling errors.
  • There's already a maintainer-authored test in test/unit/menu.test.js, "should add or remove role menu for accessibility purpose", that asserts exactly this behavior (no role attribute with zero items, role="menu" once an item exists). On the current unmodified main, that test should fail, since createEl() sets the attribute unconditionally regardless of item count — this fix is what makes it pass.
  • The pre-commit vjsstandard --fix linter (this repo's own style checker) ran clean against the change.

I'd appreciate CI (or a maintainer) confirming the full suite, especially that existing test, since I could only verify the logic and build in isolation here.

Fixes #7688

@nityanand123gupta

Copy link
Copy Markdown
Author

Update: the full karma suite did finish after all (just took longer than expected here). Final result: 4169 SUCCESS, 2 FAILED — both failures on Edge only; Chrome Headless passed all 1391 tests with zero failures, including should add or remove role menu for accessibility purpose in menu.test.js. No mention of "menu" appears in the failure output at all, so the 2 Edge failures look unrelated to this change (pre-existing Edge-specific flakiness, most likely) — but flagging for a maintainer to confirm on CI since I can't fully rule that out myself.

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.

A11Y - Broken ARIA menu on Chapters and Closed Captions (Web Accessibility)

1 participant