#895 ✓ staged
gkindel

PopcornJS support for IE8

Reported by gkindel | January 19th, 2012 @ 04:04 PM | in 1.2 (closed)

HTML5 video is out of reach for IE8, but the baseplayer and flash based players should work.

Issues for IE8:
- Array.forEach and Array.indexOf - Non-standard Event model (no document.createEvent, dom.addEventListener) - Object.defineProperty only works on DOM nodes - dom.readyState is read-only

The good news: this work is done for popcorn.js, baseplayer, and youtube. I've got a working release fork, but I need to back-port it to the development branch, commit, and attach here.

Stay tuned.

Comments and changes to this ticket

  • gkindel

    gkindel January 20th, 2012 @ 01:25 PM

    Code Change:
    https://github.com/gkindel/popcorn-js/commit/314ebd3aad6dce1a2577b6...

    While this fix makes the core support IE8, individual plugin authors will need to evaluate their support. Several Popcorn wrappers have been added to help
    this process for those seeking to make plugins legacy-compatible.

    Recommendations:

    < document.addEventListener( 'DOMContentLoaded', ready_fn )

    document.addPopcorn( ready_fn )

    < [1,2,3].indexOf('3)

    Popcorn.indexOf([1,2,3], 3)

    < [1,2,3].forEach( callback_fn )

    Popcorn.forEach([1,2,3], callback_fn )

    < [1,2,3].map( callback_fn )

    Popcorn.map([1,2,3], callback_fn )

    < element.addEventListener("click", callback_fn)

    Popcorn.addEventListener(element, "click", callback_fn)

    < element.removeEventListener("click", callback_fn)

    Popcorn.removeEventListener(element, "click", callback_fn)

  • Rick

    Rick January 20th, 2012 @ 01:37 PM

    Greg,

    Thanks for taking the time to work on this, however Popcorn.js (the core) has a long held principle that it will only support browsers that support both <video> and <audio> elements. The idea for Popcorn.players was not to provide compatibility in older browsers, but instead to provide support for existing content to work with Popcorn.

  • Rick
  • Scott Downe

    Scott Downe January 20th, 2012 @ 02:05 PM

    I would love if we could support this as a module, that will inject the support needed. Otherwise, I suspect this is going to be hard to land.

  • gkindel

    gkindel January 20th, 2012 @ 02:16 PM

    I was asked by Brett to contribute the changes; he indicated that David Humphrey would look at the submit. It is completely understandable that internal resources need to be forward-focused with minimal time spent on proprietary browsers. This work was resourced for our own product, and a the result is freely given as open source.

    I'll leave the fork up for anyone who needs it.

  • Rick

    Rick January 20th, 2012 @ 02:17 PM

    Jon Buckley, Dave Seifried and I discussed this off-thread and we think there is a solution that will fit Popcorn.js principles, while still providing support for IE8. Basically, the idea is that a conditionally included script (popcorn-ie8.js) is created and shims support for the parts needed to make Popcorn.js work without changing anything in the core.

    I suspect most of the contents of popcorn-ie8.js can be an aggregation of existing, tested shims:

  • gkindel

    gkindel January 20th, 2012 @ 02:35 PM

    I started out with a shim, but ended up following the Popcorn.forEach model.

    You'll be able to get most of the way with a shim, but will probably still gut hung on the read-only properties in IE, and on defineProperty not working on a JS object in IE, and it not working on a DOM object in iOS. I believe those need to be worked around in the core.

  • Jon Buckley

    Jon Buckley January 20th, 2012 @ 03:24 PM

    • no changes were found...
  • Rick

    Rick January 20th, 2012 @ 03:26 PM

    iOS is currently of limited concern, considering only one device (iPad) actually supports playback "in-page", whereas iPod and iPhone are fullscreen-only opt-in. In addition to those problems, there are still many others preventing any kind of reasonable Popcorn playback experience in iOS (http://blog.millermedeiros.com/2011/04/unsolved-html5-video-issues-....

    The Popcorn.forEach implementation was purely to allow a single, clean api for iterating arrays, objects and NodeList objects (numerically indexed objects with a length property) - not as a support shim for Array.prototype.forEach (note that actually relies on the native proto method)

    This Processing.js thread has some valuable lessons: https://processing-js.lighthouseapp.com/projects/41284/tickets/1593...

    Their <canvas> is our <video>

    Of related interest: http://blogs.msdn.com/b/ie/archive/2010/09/07/transitioning-existin...

  • David Humphrey

    David Humphrey January 20th, 2012 @ 06:11 PM

    • State changed from “new” to “bugs”
    • Milestone set to 1.2
    • Milestone order changed from “75” to “0”

    I like the idea of a shim. Another way to "package it" would be to also create something like popcorn-legacy.js or popcorn-universal.js (name tbd), which is popcorn.js + popcorn-ie8.js, so that less tech savvy users can simply use that and always work. We could host that on mozillapopcorn.js.

    At any rate, the goals of this patch are great, and we should do what we can to find the right path for getting it to our users. Nice work.

  • Rick

    Rick January 20th, 2012 @ 06:15 PM

    Good call Humph - I also like the idea of having a build path for the shim

  • Scott Downe

    Scott Downe January 23rd, 2012 @ 09:43 AM

    Our module functionality pretty much is a shim, no? Then, if requested, that shim can be included with the build tool. I like where this is going.

    And, if there are one or two things that cannot be done in a shim to make this work, I think a compromise can be considered.

    "You'll be able to get most of the way with a shim, but will probably still gut hung on the read-only properties in IE, and on defineProperty not working on a JS object in IE, and it not working on a DOM object in iOS. I believe those need to be worked around in the core."

    Having one or two changes in core considered, and the rest in the shim, if that's what it takes, would be better than having it all in core, and potentially acceptable. That is, if doing all as a shim is not possible.

    "At any rate, the goals of this patch are great, and we should do what we can to find the right path for getting it to our users. Nice work." My thoughts exactly.

  • Rick

    Rick January 23rd, 2012 @ 10:24 AM

    There are serious aspects of this ticket that need to be discussed thoroughly and have more input from the rest of the core development team.

    Issues that come to mind, in order of severity:

    • Potentially breaking one of our principle development ideals, only supporting browsers that support the <video> and <audio> elements, that we all agreed on nearly a year ago.
    • Potentially modifying the core library for what seems like an extreme edge case for an HTML5 media programming library: playback of Flash video on a 8 year old browser that doesn't support HTML5 media.
    • Potentially committing and releasing code that will not pass the core test suite (of course, we'd have to maintain a special test suite for a shim module anyway)
    • Taking on the care and feeding of this code

    If I think of more, I will post them here.

    In the meantime, a possible solution that avoids touching the core:

    1. Move Popcorn.player out of core and into a module - this will allow the player code to be safely refactored for IE8 support without polluting the core library.
    2. Update the build systems (repo and website) to only include Popcorn.player when any of the players are actually included
    3. Create the new popcorn-ie8.js module
    4. Create the new build path that makes an IE8 safe build of popcorn that roughly breaks down to:
      • popcorn-ie8.js
      • popcorn.js
      • popcorn-player.js ... etc...
    5. Make sure that popcorn-ie8.js shims conditionally, allowing it to be built into popcorn-complete.js seamlessly

    This has the added benefit of reducing the size of the core lib

  • Scott Downe

    Scott Downe January 23rd, 2012 @ 02:01 PM

    Moving Popcorn.player into a module is actually an interesting idea I didn't think of. If we cannot make it a complete module, and need to modify more than a couple lines, it is certainly a possibility. But, I think we are talking worst case scenarios here. Things might change once one of us gets in the code.

    I'm going to be optimistic and say there is a way to do this without touching core, and maintaining one module that may or may not live in our repo. It should live in whatever we decide plugins live in. Best case scenario, we only touch Rick's first issue, and not even in a "we support IE8", but more of a "youtube player supports IE8 via a shim". Future plugin authors can support the shim if they want to or not.

    Also, have we considered plugins that don't support IE8? I can see that being a pretty intense headache, considering the tests would also have to support IE8. We should consider only doing this for youtube via a module, and see how that goes.

    This is not an easy choice, but IMO, it is a good sign if these are the problems we have.

  • Rick

    Rick January 23rd, 2012 @ 02:26 PM

    Also, have we considered plugins that don't support IE8?

    I actually hadn't even thought of it... a good portion of the native JS stuff will be covered by the "popcorn-ie8.js" shim, beyond that, I'm not even sure.

  • David Humphrey

    David Humphrey January 23rd, 2012 @ 02:55 PM

    I think we're always going to be in a YMMV situation with IE8 support and plugins: we don't need/want to guarantee it won't break something. We learned this with IE8 support for processing.js. If we're clear to people that this is an extra thing they can opt into, they can also choose to accept the risk that it might break some plugins in odd ways. That seems like a price worth paying for people who need this to work--they can fix bugs and/or fork/fix their plugins too :)

  • Rick

    Rick January 23rd, 2012 @ 04:14 PM

    @Humph, I support that thinking completely. I feel like "opt-in" goes hand in hand with the plan to keep the changes out of core, allowing devs to "opt-in" by adding the shims. If that means Popcorn.player is kicked from the nest to stand on it's own, then I'm ok with that - because we can just as easily bake it into the build paths.

  • Scott Downe

    Scott Downe January 24th, 2012 @ 05:05 PM

    Would it be possible to include the normal player code by default, but shim in the IE8 player code, then shim in the rest of the IE8 code?

    I am OK with player code being a module as a last resort though, just thinking of possible solutions.

    I also think humph is spot on in the approach to this. I also don't think plugin support is all that critical.

    I am noticing a community that will create their own plugins instead of using our plugins, and our support plugins have become more of an example or something to play with as opposed to something that can make a uniquely rich experience. It really is hard to create a cookie cutter like plugins, and expect people to be able to create their specific experiences with it. But, this is something we have been, and can let evolve bit by bit as it fits. Basically, one millionth tower vs popup video. One millionth tower has a larger use case for IE8 support than a skateboarding video using footnotes.

  • Rick

    Rick January 24th, 2012 @ 05:54 PM

    The shim should only be modifying or filling in for native and host* objects, and would actually have to be loaded in first so it can be parsed by the browser first. This way when the browser parses Popcorn core it will have everything it needs to support the minimal requirements (any missing Object, Array, JSON (backup), XHR etc.)

    Moving Popcorn.player into its own module allows for the changes it requires to be made without disturbing the core. If we left it in, we'd have to load another shim after the core anyway - to shim Popcorn.player.

    Think...

    [Shim] [Popcorn Core] [Popcorn.player] [...]

    vs

    [Shim] [Popcorn Core (w/ Player still contained)] [Popcorn.player Shim] [...]

    • Not happy about it, but I'm willing to make the trade
  • Scott Downe

    Scott Downe January 30th, 2012 @ 01:23 PM

    • State changed from “bugs” to “assigned”
    • Assigned user set to “Scott Downe”

    I am going to give this a go.

    I think it is pretty mutual that .player should be a module. I talked to Jon and dseif, and they agreed. I am going to do that in another ticket.

    I was also thinking about other possible modules, and I think the effects nonsense is not really sticking. I think we should move it to a module to continue to support the few things that do use it, and instead encourage people to roll more specific plugins, than try to make one plugin do everything. Our plugin creation is good and clean, and should be leveraged whenever possible.

  • Scott Downe

    Scott Downe January 30th, 2012 @ 01:27 PM

    Filed #904 to deal with making things modules.

  • Rick

    Rick January 30th, 2012 @ 03:02 PM

    @Scott I support those moves as well.

  • Scott Downe

    Scott Downe February 26th, 2012 @ 02:24 AM

    Pretty rough around the edges, but I got it working.

    I will be cleaning it up shortly, but if anyone interested in how this is progressing, here it is: https://github.com/ScottDowne/popcorn-js/commits/t895

    It is complete, just needs cleaning, fixing, tests, and plugins are another issue.

  • Rick

    Rick February 26th, 2012 @ 10:06 PM

    Looking great so far

  • Scott Downe

    Scott Downe February 28th, 2012 @ 11:33 AM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “Jon Buckley”

    https://github.com/ScottDowne/popcorn-js/commits/t895

    I got it working, and attempting to get the youtube tests passing inside this.

    It is proving difficult.

    That main problems are it is just soo slow. At one point I had a test pass if the timeout was 60 seconds. It took it 60 seconds to pass. Imo, that is a fail if other browsers can do it in less than a second.

    The data attribute is not exposed on the flash object. This is used in Controls and Annotations toggling test. We can do without that for now.

    Finally, ie8 does not allow a youtube instance to be created on an existing youtube instance. This causes tests that require 6 instances to take forever to run (this is the cause of the 60 second test) with each test needing a completely new youtube instance.

    I don't think we care about these issues at the moment. Pretty low priority if you ask me.

    Something that is important to test is that the ie8 unit tests DO pass on other browsers. This is important because we don't want our ie8 support to be only ie8 support. This is one of the reasons I wanted them using the same tests.

    The best thing to test this on is the popcorn.ie8.html page in ie8, then test that page on another browser. Should still work.

    Not all plugins support ie8 either. Subtitle, webpage, attribution, footnote (the basic html ones) things like flickr, twitter (things with external apis) can or may have issues.

  • Jon Buckley

    Jon Buckley March 4th, 2012 @ 04:04 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Jon Buckley” to “Scott Downe”
    • Milestone order changed from “48” to “0”

    popcorn.ie8.js:

    The rest of the code look good, I can't really review the tests though... my internet isn't fast enough, heh.

  • Rick

    Rick March 4th, 2012 @ 04:14 PM

    +1 to using es5-shim wholesale, I guess I was unclear in my post above. I say this even though I'm kind of flattered that you used the MDN Array.prototype.map - I wrote that :D (Any of the shims on MDN that give numbered production steps are my contributions :D )

  • Scott Downe

    Scott Downe March 5th, 2012 @ 03:10 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “Jon Buckley”

    https://github.com/ScottDowne/popcorn-js/commits/t895

    So, I am not yet using es5-shim wholesale only because I don't think we need the whole thing. I do feel that is the direction we are taking, just have not done it yet. I am fully comfortable with the way it is though. Also, if we move to es5-shim we would have to start doing browser detection using document.documentMode === 8. es5-shim is too good that I don't have other ways to detect this. Something we need to consider before we pull it in.

    I do have all the tests passing and updated the whitespace and other none es5-shim requests Jon had.

    I did not do "this is an optional parameter, it's not required" as I do not know what you mean. How can this be optional as at the time this is a reference to the array that map is being called on. Without this, we would have nothing to map.

    Mainly I am requesting review of the tests, which now should work in a timely manner.

  • Rick

    Rick March 5th, 2012 @ 03:41 PM

    @scott I just chatted with Jon, the note about optional thisArg was a misunderstanding - no big deal :D

  • Jon Buckley

    Jon Buckley March 5th, 2012 @ 03:42 PM

    As rick pointed out on IRC, this != thisArg so you can ignore my comment about... this.

  • Jon Buckley

    Jon Buckley March 6th, 2012 @ 12:51 AM

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

    David Seifried March 6th, 2012 @ 09:00 AM

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

    In popcorn.ie8.js:

    • Line 53 is indented too far
    • Line 78 do we need the extra space at the beginning of the string?

    popcorn.ie8.html/unit.html/unit.js all seem to be youtube tests? Is this supposed to be like this?

    PR-

  • Scott Downe

    Scott Downe March 6th, 2012 @ 09:25 AM

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

    https://github.com/ScottDowne/popcorn-js/commits/t895

    Fixed up the whitespace.

    The idea is that if our ie8 shim can pass the youtube tests, it is doing its job.

    So yes, supposed to use the youtube tests, but I went back and double checked, and found something wrong.

    I initially started by copying the youtube directory over, then I decided to link to the youtube.unit.js file directly, but never did remove the not needed ie8.unit.js. I simply removed it.

  • David Seifried

    David Seifried March 6th, 2012 @ 09:47 AM

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

    Nice, so basically my only nit left is the styling of the html files. Other than that you have a SR+ from me.

  • Scott Downe

    Scott Downe March 6th, 2012 @ 12:03 PM

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

    https://github.com/ScottDowne/popcorn-js/commits/t895

    I moved it out of modules. This was for two reasons.

    1. I don't think it is actually a module. I don't want to get into the habit of calling anything we don't want in core a module. Modules should be attached to Popcorn after Popcorn is included, that add functionality. This is more of a comparability layer, that is included before Popcorn. Moved it into its own, as there is nothing else like it, I don't want to promote others like it, and it will be obvious for people looking for it.

    2. Makes adding it to our makefile much easier, because it needs to be included before Popcorn, and the makefile makes the assumption that everything in the modules dir is a module of the same type.

    Fixed styling as well.

  • Rick

    Rick March 6th, 2012 @ 12:05 PM

    Are we still in consensus about creating a new build target for this? eg. popcorn.complete.ie8.js (and min)

    It would be incredibly unfair to ship this in regular popcorn.complete.js

  • Scott Downe

    Scott Downe March 6th, 2012 @ 12:34 PM

    I don't remember what we decided on regarding ie8 and complete.

    Personally, I don't want it in complete, but in my mind the word complete means it has everything.

    I do know the solution here is an option when you build it, to say nay or yay to ie8, what we have to decide on is what is the default to ie8, nay or yay?

    Actually Rick, I think you are right about this.

    Add a flag to the Makefile that checks if ie8 is to be supported, otherwise build complete without ie8.

  • cadecairos

    cadecairos March 6th, 2012 @ 12:38 PM

    we should comment on this in #939

  • David Seifried

    David Seifried March 6th, 2012 @ 12:55 PM

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

    Yeap the makefile stuff can be discussed in 939.

    As far as the review goes this looks great, SR+.

  • Jon Buckley

    Jon Buckley March 6th, 2012 @ 01:47 PM

    • Assigned user changed from “cadecairos” to “Jon Buckley”
  • Jon Buckley
  • Rick
  • Rick

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

Attachments

Referenced by

Pages