-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Buttons #2209
Buttons #2209
Conversation
refactored buttonText and got tests passing refactor buttonText to controlText
line-height: 1em; | ||
line-height: 1.5em; | ||
height: 1.5em; | ||
width: 3em; // Firefox bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a more descriptive comment about the bug, please
Made some comments but otherwise lgtm. |
removing unnecessary localization fixed the menu button css Simplified controlText() and removed unnecessary override in MenuButton
@OwenEdwards any thoughts on how "menu buttons" should be treated? e.g. the captions menu. Should they be switched to buttons too or another element? |
Let's start by renaming 'menu button' to 'menu widget'! Semantically, this thing is a control widget with much more functionality than a button (but see below). Then, see my comments on #2155. I want to clarify a point here though about the menus, which is about usability as much as accessibility: Currently, the 'menu button' code is designed to work as a toggle button, which displays or hides the menu when clicked or activated by the keyboard (Space, Enter and Escape are supported). However, in the CSS/skin, the menu is displayed on hover, so clicking on the menu button only toggles focus (highlight) onto / off the top item in the menu, which (to me at least) is confusing, because it's not actually doing anything to the video player, but there's a visual change. When using the keyboard to control the 'menu button' (Tab to the control), pressing Enter or Space not only displays the menu, but also activates the top item on the menu, so the keyboard access is broken. We need to make a decision on what we want clicking on the menu button to do for mouse/touch users, and whether we want that behavior to be changeable with a skin. Note that it's okay to have 'display menu on hover' and still maintain keyboard accessibility (see http://oaa-accessibility.org/examplep/menubar1/). Then, we need to fix keyboard access to menus, per #2155. |
The quick fix to making menu buttons accessible is making the menus show up on |
|
This PR switches to using buttons instead of divs, as discussed in #1499. Just a note, this PR does not switch menu buttons over since that would require a substantial amount of refactoring from how things currently work. We plan on making that switch in a future minor release.