-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Chapters are broken in 4.x #676
Comments
Thanks for the writeup and the example. I was only able to get to captions and subtitles support for the 4.0 release, and chapters still need to be finished up. @sh1ps, does the new font have a chapters icon? The id error may be a closure compiler mangling issue, because I'm not seeing it with the unminified version. |
Hello |
+1 |
Any news on this? |
+1 |
Sorry, no news on this yet. No one's had a chance to address it. I'm happy to answer any questions about the code if anyone's interested in digging in. |
I also needed this functionality, and it was urgent enough to spend some time figuring out the issue. Although I'm not able to post a full working fix, I will try to give the idea of the problem. First of all, in ChaptersButton.createMenu there is a check if the track is loaded, and if not, the method simply hooks up a handler to the track's loaded event, and returns. (It even missed the call to track.load method I guess.) Here comes the crash, because it breaks the initialization logic of the MenuButton element. So I've added the ability to synchronously load the track file (with jQuery for the simplicity), for this I needed to modify "vjs.get" method by adding and handling an "isSync" parameter. Then I used this snychronous method for loading the track if it's not loaded. I've also removed the too complex ChaptersButton.createMenu override, and instead, I moved the synchronous loading and menu initialization logic to the createItems override, but I think this is not important in the fixing process. Finally, I added the missing CSS styles for "vjs-chapters-button" (with my custom icon due to not knowing which unicode sign to use from VideoJS icons). I hope this helps to fix this issue in the library code, or at least to someone stubborn like me. Zoltán Ps.: I'm not posting my code because it's not fully tested yet, it's quick and dirty, not to mention it uses jQuery for the synchornous "get". |
Cool! Even if the code is quick, dirty, and untested, I'd still like to take a look. Mind posting an example of where you're using it in action? |
Unfortunately I can't post the example page because it's not public, but I will gather the code snippets what I've changed for getting it working, and post them tomorrow here, if that's OK. |
Ah totally understandable. Whenever you get a chance that'd be really cool. I bet others would appreciate seeing a working example as well. |
Here is the modified js file, I have put a "ZZZ" comment to the places where I modified the code. Some of the modifications don't relate to this bug, but maybe worth a consideration. http://www.vertesz.hu/zoli/video.dev.js Please take into account, that the base file was the last official stable build (4.4.2), not the latest github project. If you have any questions, feel free to contact me on my mail (zolitamasi@gmail.com). Have a nice day, |
A bit offtopic, but just for letting anyone know who checked the link above: during testing I had issues with my nestable implemenation of the show/hide methods, so I reverted them back to the original logic, and updated the link above. |
After looking at the code there seems to be a quick and simple fix to get it all working. Is there an existing icon in the font svg or elsewhere (or perhaps I can grab a preferred one from icomoon to include) that I can use in order to get this fixed? |
Hi @heff. Just wondering if there was any update on this? No rush. |
@Chris-DL There's a speech bubble icon you can use right now, but we'd like to add a more representative icon soon. Have you taken a stab at implementing this yet? |
@mmcc Okay, great! I'll use that icon and get the functionality fixed shortly. |
Sweet! Feel free to submit a PR whenever you're ready. |
FYI, chapters have been fixed in the master branch. It will go out with v4.7.0. |
Is this fixed? On relevant markup
relevant js
demo.vtt
|
Chapters functionality is completely broken in 4.x. This occurs on at least Chrome 28, Firefox 22 and IE9.
Expected:
When adding a line like
<track kind="chapters" source="chapters.vtt" default>
to a
<video>
element, a chapters button should be shown, with named chapters linking to cue points within the video.Actual:
A js error (
Uncaught TypeError: Cannot read property 'id' of undefined
) occurs and the player crashes, not showing any controls.See http://jsbin.com/uceyam/1/
Further info:
In 3.x, the chapters menu was only created (or at least displayed) if the
<track>
element with attribute/valuekind="chapters"
also had thedefault
attribute declared. It seems this attribute is still of influence, since without it defined on the chapters track there is no crash (no chapters, either, though).If it is defined, like i've done in this example (it doesn't seem to allow cross-origin linking of track files, but the result is the same - the error seems to occur before it gets to requesting the file), Chrome's js console will show the following error:
Uncaught TypeError: Cannot read property 'id' of undefined
This error comes from the
vjs.Component.prototype.addChild
method, which in this instance is being called, presumably somewhere along the creation of the chapters track, button or menu object, without validchild
andoption
parameters (both having a value ofundefined
when the error occurs). In addition to failing on the chapter tracks, this error causes the player to crash, never showing the controls bar.People have reported this before on StackOverflow (see here and here).
The text was updated successfully, but these errors were encountered: