Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed two bugs #154

Merged
merged 2 commits into from Mar 21, 2012

Conversation

Projects
None yet
2 participants
Contributor

sirudog commented Mar 18, 2012

Hi,

I was integrating videojs player to my drupal site, and found these small bugs which caused the following problems:

  • flash-fallback could not pick up sources from tags, since the code was not case-insensitive, so the src of the video could not be included into flashvars.
  • the code could not completely remove the tags after processing them, because the loop was based on a dynamically changing condition, making it end prematurely (before it could remove all the source tags from video tag)

Please review these changes.
Thanks

sirudog added some commits Mar 18, 2012

@sirudog sirudog Fix bug in loop for removing source/track nodes from video element.
"j.length" changed in each loop step, causing source/track nodes being left in DOM.
5c2a3cb
@sirudog sirudog Fixed bug of traversing video source/track child nodes by making node…
…Name comparison case-insensitive.

It caused the issue of flash-fallback not being able to discover the source of the video and embed it into flashvars.
4df3d84

heff commented on 4df3d84 Mar 19, 2012

Thanks for the commit. This wouldn't hurt to add either way, but I'm not understanding what the issue with Flash is. Flash would be getting the source from options.sources just like HTML5, and I'm pretty sure nodeName should always be capitalized. Do you have a live example of what you were running in to?

Owner

sirudog replied Mar 20, 2012

I do not have a live example, but here is the code I used. I build up the player dynamically, so my nodeNames are lower-cased. Maybe the fact that I manually create the player causes this issue. I don't see why nodeName is always capitalized.

SiteAnimation.CreateVideoTag = function (videoSources) {
var tag, sourceTag;

tag = document.createElement("video");
tag.id = SiteAnimation.VideoId;
tag.className = "video-js vjs-default-skin";

$j.each(videoSources, function (index, sourceUrl) {
    // here I used lower-case nodeName, as I was not aware of that the library expects capitalized nodename
    sourceTag = document.createElement("source");
    sourceTag.src = sourceUrl;
    sourceTag.type = SiteAnimation.ParseVideoSourceType(sourceUrl);
    tag.appendChild(sourceTag);
});

return tag;

}

Then I called SiteAnimation.VideoPlayer = V(SiteAnimation.VideoId, { "controls": true, "autoplay": false, "preload": "none" });
to build my player.
I tested it in IE8, where flash-fallback is used as the tech, and none of the source tags could be picked up. I started to debug it, and I saw that the player code compares the nodeName to "SOURCE". I am not sure if html5 tech has the same issue...

Hope this helps to clarify my scenario.

Owner

sirudog replied Mar 20, 2012

sorry, the underscores bordering V was wiped out from the last line of code above.

heff replied Mar 21, 2012

Aha, cool. Other browsers convert to uppercase automatically, but it looks like IE doesn't (including IE9, so it'd affect HTML5 too). Thanks for digging into the issue.

heff commented on 5c2a3cb Mar 19, 2012

Heh, that makes sense. Interesting I haven't seen this yet. Doesn't seem to be an issue in most browsers when using 3 sources. Thanks for this.

Owner

sirudog replied Mar 20, 2012

You're very welcome. Thanks for this wonderful player. I'am glad I could help.

Owner

heff commented Mar 19, 2012

Commented on the second.

@heff heff added a commit that referenced this pull request Mar 21, 2012

@heff heff Merge pull request #154 from sirudog/master
Fixed two bugs: Removing sources with changing source count. Checking element nodeName, IE doesn't force uppercase.
43fc61f

@heff heff merged commit 43fc61f into videojs:master Mar 21, 2012

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