Stops top menu item being triggered when opening the menu using the keyboard #1447
Conversation
| // This function is not needed anymore. Instead, the keyboard functionality is handled by | ||
| // treating the button as triggering a submenu. When the button is pressed, the submenu | ||
| // appears. Pressing the button again makes the submenu disappear. | ||
| vjs.MenuButton.prototype.onFocus = function(){}; |
There was a problem hiding this comment.
Was this removed to re-enable the button focus listeners?
There was a problem hiding this comment.
Yep, we only want to listen when we have focus.
| @@ -200,6 +191,7 @@ vjs.MenuButton.prototype.onKeyPress = function(event){ | |||
| }; | |||
|
|
|||
| vjs.MenuButton.prototype.pressButton = function(){ | |||
| this.on('keyup', this.onKeyPress); | |||
There was a problem hiding this comment.
I'm having trouble following the logic here. It looks like the onKeyPress listener calls pressButton which then adds the onKeyPress listener. I think that might duplicate the listener.
There was a problem hiding this comment.
We have to readd the listener to get bubble key press events from the menu item so we can close the menu. Ideally we would have the menu trigger an event when it is activated and deactivated.
If you look further down the function the first menu item is focused on, this will remove the menubutton's keyup listener set in the button focus listeners. Looking at the code now, it might be better to move this keyup listener to inside that if statement, so it is only set if there are menu items.
|
I think this might be related to the deeper issue of event bubbling unexpectedly in #1452. So I think we should probably address that first, at least to make sure it doesn't affect this. |
|
I looked into #1452 it seems to work fine on FireFox and Chrome on windows 8. Could be an osx thing? |
|
I just merged #1452 into master. Do you want to try that out and see if it changes this issue for you at all? Or are you saying you already tried that? |
|
@mynameisstephen we're working on a release. Any new info on this one? |
mynameisstephen
force-pushed
the
master
branch
from
04e94de
to
055d81d
Compare
|
Still an issue with the #1452 fix. Also tried changing the MenuButton code in master to use keydown events but that still causes issues. |
|
Hmm this didn't merge properly. Maybe because I moved the fix to a branch - https://github.com/mynameisstephen/video.js/tree/menubutton-fix - should I submit another pull request? |
Fixes #1446
Key press handler is only active when either the MenuButton has focus or the Menu is open (allows closing the menu when a menu item is selected).