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

Menu title LI created outside UL #1107

Closed
cmaish opened this issue Mar 24, 2014 · 4 comments · Fixed by #1114
Closed

Menu title LI created outside UL #1107

cmaish opened this issue Mar 24, 2014 · 4 comments · Fixed by #1114

Comments

@cmaish
Copy link

cmaish commented Mar 24, 2014

When videojs adds the menus, it seems that the .vjs-menu-title LI item that's created is added to the .vjs-menu (which is a DIV) instead of the .vjs-menu-content.

This is present for me in 4.4.3. This is present on the main page of http://www.videojs.com/ . The output HTML in my case is:

<div class="vjs-chapters-button vjs-menu-button vjs-control " role="button" aria-live="polite" tabindex="0" aria-haspopup="true" aria-label="Chapters Menu" style="display: none;">
   <div class="vjs-control-content"><span class="vjs-control-text">Chapters</span></div>
   <div class="vjs-menu">
      <ul class="vjs-menu-content"></ul>
      <li class="vjs-menu-title">Chapters</li>
   </div>
</div>

It seems that the culprit is:

https://github.com/videojs/video.js/blob/master/src/js/tracks.js#L951

which uses menu.el_ rather than menu.contentEl (or creates an LI rather than a div, not sure what the 'right' fix is here, but either should work).

@cmaish
Copy link
Author

cmaish commented Mar 24, 2014

Also there's another area in the source (vjs.MenuButton.prototype.createMenu) which may be affected by the same issue - it uses menu.el() which IIRC just returns component's el_ (so the div), but I don't hit that codepath.

@jnwng
Copy link
Contributor

jnwng commented Mar 24, 2014

+1, had the same issue, solved it by making a change here.

i've changed menu.el_ to menu.contentEl(), which properly sets the title item inside of the <ul></ul> element. there was a little bit of strangeness with styling that may additionally have to be addressed

@mmcc
Copy link
Member

mmcc commented Mar 24, 2014

@jnwng Any chance you'd be interested in submitting a PR for your fix?

@jnwng
Copy link
Contributor

jnwng commented Mar 24, 2014

yep, i can get to that hopefully by the end of the day.

@heff heff closed this as completed in c7e35e5 Apr 3, 2014
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants