#1230 review-needs-work
Charlie Stigler

Subtitles positioning breaks when container has relative position

Reported by Charlie Stigler | July 22nd, 2012 @ 07:20 PM | in 1.4

If the div containing the video has position: relative, the Popcorn subtitles positioning will break. This is because the plugin is set up to position the subtitles container absolutely to the page, and with a position: relative ancestor it will instead be positioning it absolutely to that ancestor. So the subtitles appear in the wrong place.

I'm finishing up a patch that should fix this (by adding a position: relative wrapper) right now... while digging around I'm seeing some other subs cleanup that might be nice, but I'll leave that for later.

Comments and changes to this ticket

  • Charlie Stigler

    Charlie Stigler July 22nd, 2012 @ 08:00 PM

    OK, made a patch that adds a wrapper element (extContainer) that is relatively positioned, then puts the subtitle container inside that and positions it absolutely on the wrapper. This also means that we don't have to worry about repositioning on video movement, although I left the updatePosition timer with a longer period because I'm not sure about a better way to deal with video resizing. I also removed now-obsolete tests that tested how the subtitles moved when the video was repositioned, because now the browser should handle that itself.

    Note that I have only had a chance to test in a relatively narrow set of cases (including browsers etc.), so if other people could check and make sure this doesn't break in some browser or some use-case that would be good.

    Pull request: https://github.com/mozilla/popcorn-js/pull/150

  • Scott Downe

    Scott Downe July 23rd, 2012 @ 10:01 AM

    • Assigned user set to “Scott Downe”
    • Milestone changed from 1.3 to 1.4
    • Milestone order changed from “138” to “0”

    Going to take a look at this, but going to look at it in 1.4.

  • Charlie Stigler

    Charlie Stigler August 21st, 2012 @ 08:46 PM

    • State changed from “new” to “peer-review-requested”

    Bump for this since the 1.3 rush is now over. I don't know that my solution is the most elegant, it might fail in some CSS situations that I haven't considered (but then, the current version fails in any relatively positioned element, so it may still be an improvement).

  • Scott Downe

    Scott Downe August 28th, 2012 @ 05:19 PM

    • State changed from “peer-review-requested” to “feedback-requested”
    • Assigned user changed from “Scott Downe” to “Charlie Stigler”
    • Milestone order changed from “125” to “0”

    If the video is moved without the container being moved. So, the video element itself, the subtitle won't go with it.

    What if we just added our subtitle container to the body so absolute or relative, it'll mean the same thing. Then, in update position, we can get the video's bounding client rect, and check the left and top, which, I believe (correct me if I am wrong) are absolute to the body, no matter what. With this, we can update the subtitle's container.

  • Charlie Stigler

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

    • State changed from “feedback-requested” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “Scott Downe”
    • Milestone order changed from “2” to “0”

    Good point, that's a much better system. I redid this to work that way, appending the subtitle container to body. As an extra upside of this, the old tests are still relevant with this change (and they pass).

    Old pull request seems to have been closed for oldness, here's a new one:

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

  • Scott Downe

    Scott Downe November 27th, 2012 @ 10:38 AM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Scott Downe” to “mjschranz”
    • Milestone order changed from “173” to “0”

    Goood stuff. PR+

  • mjschranz

    mjschranz November 27th, 2012 @ 12:03 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “mjschranz” to “Charlie Stigler”
    • Milestone order changed from “24” to “0”

    Minor nit. SR+ with it.

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

Pages