Skip to content
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

Ignore titles in menu, if they exist. #3165

Closed
wants to merge 3 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 9, 2016

Description

This is because of the change in #3163 where we added the title as an
item in the childrens array so that it is ordered correctly but this
breaks keyboard support because they expect the children to be lined up
correctly and also because they expect the children to be component
objects with an 'el_' property.
Checking to see if the first item is a 'vjs-menu-title' and adding one
to the 'item' allows us to ignore the title element and also select the
actual menu items.
Fixes #3164.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by One Core Contributors

This is because of the change in videojs#3163 where we added the title as an
item in the childrens array so that it is ordered correctly but this
breaks keyboard support because they expect the children to be lined up
correctly and also because they expect the children to be component
objects with an 'el_' property.
Checking to see if the first item is a 'vjs-menu-title' and adding one
to the 'item' allows us to ignore the title element and also select the
actual menu items.
@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Mar 9, 2016
@gkatsev
Copy link
Member Author

gkatsev commented Mar 9, 2016

I think relying on the lack of el_ to tell that it's a "title" isn't specific enough, since, maybe something else snuck in there somehow? We probably should also verify that el_ exists before trying to call a method on it.

@OwenEdwards
Copy link
Member

LGTM. 👍

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Mar 10, 2016
@gkatsev gkatsev closed this in 8cc633e Mar 10, 2016
@gkatsev gkatsev deleted the menu-with-title branch March 10, 2016 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard control of menus doesn't work for the Chapters menu
2 participants