#1330 ✓ staged
brianchirls

[YouTube] HTMLYouTubeVideoElement width/height properties crash if called before youtube api is ready

Reported by brianchirls | September 9th, 2012 @ 11:41 AM | in 1.4

The elem element isn't created until after both a src is set and YT API is ready. But scripts that assume HTMLYouTubeVideoElement works like a real element may try to access width/height or other properties immediately upon creation.

So either (or both):
1) container element should be created immediately, even if YT is not loaded and src is not set
2) width/height should return 0 if element is missing.

Thanks

Comments and changes to this ticket

  • David Humphrey

    David Humphrey September 10th, 2012 @ 09:55 PM

    • State changed from “new” to “assigned”

    We should do 2) for all the wrappers, not just YouTube.

  • Charlie Stigler

    Charlie Stigler September 17th, 2012 @ 09:36 PM

    • State changed from “assigned” to “peer-review-requested”
  • brianchirls

    brianchirls September 21st, 2012 @ 09:43 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “David Humphrey” to “Charlie Stigler”
    • Milestone order changed from “36” to “0”

    This is not going to return the right value if width or height are queried after the size is set but before player is done loading. I suggest something like this:

      return elem ? elem.width : impl.width || 0;
    

    This patch could also use a set of unit tests that queries the dimensions at a few different stages:

    • after creating the element
    • after setting src
    • after loadedmetadata
    • after video starts playing
  • Charlie Stigler

    Charlie Stigler October 21st, 2012 @ 03:48 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “brianchirls”
    • Milestone order changed from “4” to “0”

    Hm, I believe returning 0 at that point is how native DOM elements work, but I guess it makes sense to take advantage of our extra information, and I don't think it'll break anything. I changed it to check impl.width/height as suggested before defaulting to 0, see the updated pull requests.

    On the tests: I started to write a set, but it turned out that they failed no matter what because onloadedmetadata never fired. We also would need a separate set per-element (instead of placing in common) because YouTube has min width/height that will be returned instead of 0 even before it is initialized. I find it really frustrating to write tests when everything surrounding the test is broken, so if we do need tests on this then hopefully you/somebody else can write them.

  • brianchirls

    brianchirls October 21st, 2012 @ 11:17 AM

    I only see the one commit from a month ago.

    I need to be iterating the YouTube wrapper much faster for my projects than the community can handle, I've forked and will be making changes on my own. You guys are welcome to them:

    https://github.com/hapyak/popcorn-js/tree/feature/youtube

  • Charlie Stigler

    Charlie Stigler October 21st, 2012 @ 10:07 PM

    Yeah, it's in that one commit, I squashed them together to simplify history.

    And thanks for the link to your fork, I have the same issue and am also building off my own fork right now, it'll be good to look through yours and try to pull some stuff.

  • Scott Downe

    Scott Downe December 13th, 2012 @ 01:47 PM

    • Assigned user changed from “brianchirls” to “Scott Downe”
  • Rick

    Rick January 6th, 2013 @ 09:49 PM

    • Title changed from “HTMLYouTubeVideoElement width/height properties crash if called before youtube api is ready” to “[YouTube] HTMLYouTubeVideoElement width/height properties crash if called before youtube api is ready”
    • Tag cleared.
  • Scott Downe

    Scott Downe January 7th, 2013 @ 01:43 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Tag set to youtube
    • Assigned user changed from “Scott Downe” to “brianchirls”

    "I guess it makes sense to take advantage of our extra information"

    I don't agree with this. Defaulting to 0 until loaded is expected. Until loaded, we shouldn't have anything visually on the page, thus height and width is actually 0.

    Not sure who to assign this to. I am willing to make the changes and get this to landed.

    Going to make a pull request and see what happens with it.

  • Scott Downe

    Scott Downe January 7th, 2013 @ 02:06 PM

    So, not what I expected: http://jsfiddle.net/yDvHR/1/

    It returns the height/width as soon as you set it. Why not just return impl.width || 0. I don't think we need to ever touch the elem based on that fiddle. Also, I do not see how that setter is going to do anything as it is now.

  • Scott Downe

    Scott Downe January 7th, 2013 @ 02:37 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    I'm just going to try to change it so elem always exists, and is added/removed from dom on ready/destroy. On destroy, it creates a new elem. That way we just do whatever elem does.

    https://github.com/mozilla/popcorn-js/pull/266

  • Scott Downe

    Scott Downe January 14th, 2013 @ 01:10 PM

    • Assigned user changed from “brianchirls” to “Jon Buckley”
  • Jon Buckley

    Jon Buckley January 15th, 2013 @ 04:41 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Jon Buckley” to “cadecairos”

    PR+

  • cadecairos

    cadecairos January 18th, 2013 @ 02:21 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “Scott Downe”

    One small lint issue to deal with.

  • Scott Downe

    Scott Downe January 18th, 2013 @ 06:51 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Scott Downe” to “cadecairos”
  • cadecairos

    cadecairos January 21st, 2013 @ 01:37 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “cadecairos” to “Scott Downe”

    SR+

  • Scott Downe

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Tags

Pages