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

Keyboard Accessible MenuItems and VolumeMenuButton #1519

Merged
merged 3 commits into from Sep 30, 2014

Conversation

Projects
None yet
3 participants
@rpless

rpless commented Sep 19, 2014

Makes the volume control and menu items accessible via tab navigation on the keyboard.
Fix for #1517

Ryan Plessner
Changed the MenuItem and the Volume Control bar to gain focus when us…
…ing tab navigation.

Previously, they could not be navigated to via the keyboard.
Fixs #1517.
Tested via manaul cross-browser tests using keyboard navigation (chrome, firefox, safari, and IE 11 ).
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 23, 2014

Member

Thanks for this! I added my thoughts to #1517.

Member

heff commented Sep 23, 2014

Thanks for this! I added my thoughts to #1517.

Ryan Plessner
reverted the submenu behavior. it now needs to activated before it me…
…nuitems become tab targets. The volume control bar now uses the vjs-lock-showing class instead of new class.
@heff

View changes

Show outdated Hide outdated src/css/video-js.less
@@ -717,7 +717,8 @@ easily in the skin designer. http://designer.videojs.com/
}
.vjs-default-skin .vjs-menu {
display: none;
z-index: -1;

This comment has been minimized.

@heff

heff Sep 24, 2014

Member

I think we could use the hide-visually mix-in here instead of z-index. There's potential that a menu button component could be used somewhere where the menu wouldn't be covered by the the player.

@heff

heff Sep 24, 2014

Member

I think we could use the hide-visually mix-in here instead of z-index. There's potential that a menu button component could be used somewhere where the menu wouldn't be covered by the the player.

This comment has been minimized.

@rpless

rpless Sep 24, 2014

The .vjs-default-skin .vjs-menu selector is going back to using display: none based on your previous comment. So I don't it should be using hide-visually.
I don't think it makes sense to use visually-hidden for the volume bar, because then we have to manually undo all of the styles it entails and repeat styles we've already created.
Thoughts?

@rpless

rpless Sep 24, 2014

The .vjs-default-skin .vjs-menu selector is going back to using display: none based on your previous comment. So I don't it should be using hide-visually.
I don't think it makes sense to use visually-hidden for the volume bar, because then we have to manually undo all of the styles it entails and repeat styles we've already created.
Thoughts?

This comment has been minimized.

@heff

heff Sep 24, 2014

Member

Yeah, it doesn't make sense to use it on .vjs-menu with the other change.

I was mainly looking for an alternative to z-index for hiding it. Would height:0 or width: 0 work? That seems to basically be what youtube is doing. Alternatively we could add a .visuallyhidden class that contains the hide-visually styles, and add/remove that class to/from the volume slider when needed.

@heff

heff Sep 24, 2014

Member

Yeah, it doesn't make sense to use it on .vjs-menu with the other change.

I was mainly looking for an alternative to z-index for hiding it. Would height:0 or width: 0 work? That seems to basically be what youtube is doing. Alternatively we could add a .visuallyhidden class that contains the hide-visually styles, and add/remove that class to/from the volume slider when needed.

This comment has been minimized.

@rpless

rpless Sep 25, 2014

I'm a little hesitant on the .visuallyhidden idea (although I experimented with it and does work alright). My first concern is that we also need to listen to hover in code rather than css. Secondly, I kinda like being able to use the lock/unlockShowing methods on menu, as opposed to manually adding/removing classes.

The width: 0 works a bit better. It winds up being more css because you have to explicitly style the vjs-menu-content as well as the vjs-menu. However, I think this might be a better solution. I'm gonna try to clean my experiments up and push them up.
Sound good?

@rpless

rpless Sep 25, 2014

I'm a little hesitant on the .visuallyhidden idea (although I experimented with it and does work alright). My first concern is that we also need to listen to hover in code rather than css. Secondly, I kinda like being able to use the lock/unlockShowing methods on menu, as opposed to manually adding/removing classes.

The width: 0 works a bit better. It winds up being more css because you have to explicitly style the vjs-menu-content as well as the vjs-menu. However, I think this might be a better solution. I'm gonna try to clean my experiments up and push them up.
Sound good?

@heff

View changes

Show outdated Hide outdated src/css/video-js.less
@@ -717,7 +717,8 @@ easily in the skin designer. http://designer.videojs.com/
}
.vjs-default-skin .vjs-menu {

This comment has been minimized.

@heff

heff Sep 24, 2014

Member

Instead of making changes to the CSS for all menus is there a way we could use the vjs-volume-menu-button class and make updates only to that?

@heff

heff Sep 24, 2014

Member

Instead of making changes to the CSS for all menus is there a way we could use the vjs-volume-menu-button class and make updates only to that?

This comment has been minimized.

@rpless

rpless Sep 24, 2014

Yes, that seems better than what's there now.

@rpless

rpless Sep 24, 2014

Yes, that seems better than what's there now.

@@ -30,6 +30,12 @@ vjs.VolumeMenuButton.prototype.createMenu = function(){
contentElType: 'div'
});
var vc = new vjs.VolumeBar(this.player_, vjs.obj.merge({'vertical': true}, this.options_.volumeBar));

This comment has been minimized.

@heff

heff Sep 30, 2014

Member

@gkatsev I'm realizing that the volume menu button is broken by default because vertical is true by default here and nothing else is set up to handle that. If I set it to false it makes it work. Does the BC version set this to true somewhere or would that change break things?

@heff

heff Sep 30, 2014

Member

@gkatsev I'm realizing that the volume menu button is broken by default because vertical is true by default here and nothing else is set up to handle that. If I set it to false it makes it work. Does the BC version set this to true somewhere or would that change break things?

This comment has been minimized.

@gkatsev

gkatsev Sep 30, 2014

Member

why would it be broken here? Seems to be working fine for us. It used to be broken when vertical wasn't quoted.

@gkatsev

gkatsev Sep 30, 2014

Member

why would it be broken here? Seems to be working fine for us. It used to be broken when vertical wasn't quoted.

This comment has been minimized.

@heff

heff Sep 30, 2014

Member

When I test this PR and try to drag the volume handle with my mouse, the bar is horizontal but the mouse movement needs to be vertical. Are you seeing something different? To be clear, I think this issue existed before this PR.

@heff

heff Sep 30, 2014

Member

When I test this PR and try to drag the volume handle with my mouse, the bar is horizontal but the mouse movement needs to be vertical. Are you seeing something different? To be clear, I think this issue existed before this PR.

This comment has been minimized.

@heff

heff Sep 30, 2014

Member

And by test I mean open /sandbox/index.html

@heff

heff Sep 30, 2014

Member

And by test I mean open /sandbox/index.html

This comment has been minimized.

@gkatsev

gkatsev Sep 30, 2014

Member

Ah, I see. There needs to be extra CSS that rotates it.

@gkatsev

gkatsev Sep 30, 2014

Member

Ah, I see. There needs to be extra CSS that rotates it.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 30, 2014

Member

Made a few CSS changes but this should be good to go.
https://github.com/rpless/video.js/pull/1

Member

heff commented Sep 30, 2014

Made a few CSS changes but this should be good to go.
https://github.com/rpless/video.js/pull/1

@heff heff merged commit f53583e into videojs:master Sep 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

heff added a commit that referenced this pull request Sep 30, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 30, 2014

Member

I merged in my changes to get them out with the release. Let me know if you have any later thoughts and we'll get them in.

Member

heff commented Sep 30, 2014

I merged in my changes to get them out with the release. Let me know if you have any later thoughts and we'll get them in.

@rpless

This comment has been minimized.

Show comment
Hide comment
@rpless

rpless Oct 1, 2014

I think they're good. Thanks @heff.

rpless commented Oct 1, 2014

I think they're good. Thanks @heff.

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