fix(Menu): only set role=menu on the content list once it has an item - #9228
Open
nityanand123gupta wants to merge 1 commit into
Open
fix(Menu): only set role=menu on the content list once it has an item#9228nityanand123gupta wants to merge 1 commit into
nityanand123gupta wants to merge 1 commit into
Conversation
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 |
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.
Problem
Menu.createEl()setsrole="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 ofcreateEl()and into a newupdateMenuRole_()helper, called from bothaddItemandremoveChild, 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 infocus(), sinceMenuButtonalways adds its title viaaddItemtoo — a title-only menu (the exact real-world scenario in this issue) must not getrole="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 -cbuilds clean with this change — no syntax/bundling errors.test/unit/menu.test.js,"should add or remove role menu for accessibility purpose", that asserts exactly this behavior (noroleattribute with zero items,role="menu"once an item exists). On the current unmodifiedmain, that test should fail, sincecreateEl()sets the attribute unconditionally regardless of item count — this fix is what makes it pass.vjsstandard --fixlinter (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