#565 ✓ staged
Scott Downe

subtitles need a teardown after all

Reported by Scott Downe | June 10th, 2011 @ 11:37 AM | in 0.7

If you remove a subtitle while it is being displayed, you end up with a subtitle being displayed forever.

The subtitle will safely be removed if another subtitle is displayed, then removed.

This is because subtitles are just displayed in a div shared by all subtitles over the video, and we have no way of knowing, inside a teardown, if we should clear the div's contents or not.

Example 1: two subtitles, one after the other. paused on subtitle one, remove subtitle two, subtitle one is still displayed. Everything working as intended.
Example 2: two subtitles, one after the other. paused on subtitle two, remove subtitle two, subtitle two is still displayed. Won't be un displayed until another subtitle comes up.

If we clear the div's contents in teardown, we may actually break use example 1, while fixing example 2.

If we stored our subtitles as divs inside the div over the video, we could destroy just that subtitle by div reference, and leave all other subtitles in tact.

Comments and changes to this ticket

  • David Seifried

    David Seifried June 22nd, 2011 @ 12:36 PM

    • Assigned user changed from “Scott Downe” to “David Seifried”
    • Milestone order changed from “41” to “0”
  • David Seifried

    David Seifried June 22nd, 2011 @ 05:09 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Added teardown function and made each subtitle use its own child div.
    https://github.com/dseif/popcorn-js/commits/t556

  • cadecairos

    cadecairos June 22nd, 2011 @ 06:28 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    I see some places in this code for major performance fixes.

    line 9:

    if ( typeof google !== 'undefined' && google.load ) {
    // change to !google
    

    in the method createDefaultContainer we can decrease the amount of property lookups like so:

    createDefaultContainer = function( context ) {
      // clear this function from future calls; we are done
      createDefaultContainer = Popcorn.nop;
    
      var container,
        style ,
        media = context.media,
        position = context.position(),
       
        updatePosition = function() {
          // the video element must have height and width defined
          style.fontSize = "18px";
          style.width = media.offsetWidth + "px";
          style.top = position.top  + media.offsetHeight - container.offsetHeight - 40 + "px";
          style.left = position.left + "px";
    
          setTimeout( updatePosition, 10 );
        };
            
      container = context.container = document.createElement( 'div' );
      style = container.style;
      container.id = "subtitlediv";
      style.position = "absolute";
      style.color = "white";
      style.textShadow = "black 2px 2px 6px";
      style.fontWeight = "bold";
      style.textAlign = "center";
    
      updatePosition();
    
      document.body.appendChild( container );
    };
    

    line 11 needs to be brought into compliance with the style guide.

    this loop is better written as:

    for( var k = 0, children = options.container.children, len = children.length; k < len; k++ ) {
      if ( children[ k ].style.display === "inline" ) {   
        children[ k ].innerHTML = result.translation;    
      }  
    }
    

    line 112 has an extra space between the ) and }

    on line 164, move the declaration of "accessibility" to the top of _setup.

    this loop can also be sped up:

    for( var k = 0, children = options.container.children, len = children.length; k < len; k++ ) {
      if ( children[ k ].style.display === "inline" ) {   
        options.showSubtitle( options, children[ k ].innerHTML );   
      }  
    }
    

    line 248 has an extra space between the ) and }

    Also, the link to the branch above is incorrect, here's the correct one:

    t565

    Also, we should get some consistency with quotations. use double quotes where there are single quotes.

  • cadecairos

    cadecairos June 22nd, 2011 @ 06:29 PM

    I almost forgot. There is an error being thrown in the unit tests.

    document.getElementById("subtitle-0 ") is null
    (?)()popcor...unit.js (line 117)
    (?)(event=timeupdate )popcorn.js (line 240)
    [Break On This Error] ok ( document.getElementById( 'subtitle-0 ').style.display === "none" &&
    
  • David Seifried

    David Seifried June 23rd, 2011 @ 10:55 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Alright fixed everything, the code you posted has something wrong with it for the first main block, so I didnt end up using it exactly.

    t565

  • cadecairos

    cadecairos June 23rd, 2011 @ 12:12 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    just a few things this time:

    change the following to use "!google"

    if ( typeof google !== 'undefined' && google.load ) {
    

    because updatePosition is being called every 10 ms, calling position() once is a good idea:

    var updatePosition = function() {
      var position = context.position();
      // the video element must have height and width defined
      style.fontSize = "18px";
      style.width = context.media.offsetWidth + "px";
      style.top = position.top  + context.media.offsetHeight - context.container.offsetHeight - 40 + "px";
      style.left = position.left + "px";
    
      setTimeout( updatePosition, 10 );
    };
    

    Bring the inline comment on line 165 down one line.

    Use double quotes on line 40: 'div' >> "div"

  • David Seifried

    David Seifried June 23rd, 2011 @ 01:26 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Made the requested changes except for using !google as it does not work correctly
    https://github.com/dseif/popcorn-js/commits/t556

  • David Seifried
  • cadecairos

    cadecairos June 23rd, 2011 @ 02:35 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    move "position = context.position()," into updatePosition so that the subtitles position is updated correctly.

  • David Seifried

    David Seifried June 23rd, 2011 @ 02:39 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Nice catch,
    alright should be good to go now
    https://github.com/dseif/popcorn-js/commits/t565

  • cadecairos

    cadecairos June 23rd, 2011 @ 03:06 PM

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

    Great!

    Tested subtitle unit tests in Firefox 4 and Chrome 12, all passing.

    passes lint

    PR+

  • Scott Downe

    Scott Downe June 23rd, 2011 @ 03:43 PM

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

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