New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made vjs.get work with xDomain in IE < 10 #1095

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@skogby

skogby commented Mar 19, 2014

This fix makes the videojs get function work in older versions of IE by using XDomainRequest

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Mar 19, 2014

Member

Thanks! What is the error you're seeing in the older versions of IE that this fixes?

Member

mmcc commented Mar 19, 2014

Thanks! What is the error you're seeing in the older versions of IE that this fixes?

@skogby

This comment has been minimized.

Show comment
Hide comment
@skogby

skogby Mar 20, 2014

It enables IE to send cross domain xml requests like an ad request to Google DFP:
http://pubads.g.doubleclick.net/gampad/ads?env=vp&gdfp_req=1&impl=s&output=xml_vast2&iu=/6062/video-demo&sz=400x300&unviewed_position_start=1&ciu_szs=728x90,300x250&url=[ref_url]&correlator=[timestamp]

Most modern browsers can use the "withCredentials" flag, which older IE versions does not support.

skogby commented Mar 20, 2014

It enables IE to send cross domain xml requests like an ad request to Google DFP:
http://pubads.g.doubleclick.net/gampad/ads?env=vp&gdfp_req=1&impl=s&output=xml_vast2&iu=/6062/video-demo&sz=400x300&unviewed_position_start=1&ciu_szs=728x90,300x250&url=[ref_url]&correlator=[timestamp]

Most modern browsers can use the "withCredentials" flag, which older IE versions does not support.

@mmcc mmcc added the enhancement label Mar 20, 2014

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Mar 20, 2014

Member

Cool! When @heff gets back we'll take a look and see about getting this one pulled in.

Member

mmcc commented Mar 20, 2014

Cool! When @heff gets back we'll take a look and see about getting this one pulled in.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Mar 26, 2014

Member

Very cool, thanks! I'm not familiar enough with this to confirm so going to try to find one other person to give the thumbs up.
@dmlap, @gkatsev, @BCjwhisenant - You guys have been working with cross domain stuff. Can you take a look at this?

Member

heff commented Mar 26, 2014

Very cool, thanks! I'm not familiar enough with this to confirm so going to try to find one other person to give the thumbs up.
@dmlap, @gkatsev, @BCjwhisenant - You guys have been working with cross domain stuff. Can you take a look at this?

@BCjwhisenant

This comment has been minimized.

Show comment
Hide comment
@BCjwhisenant

BCjwhisenant Mar 26, 2014

Contributor

We can find someone on the team to take a look tomorrow.
Thanks!

Contributor

BCjwhisenant commented Mar 26, 2014

We can find someone on the team to take a look tomorrow.
Thanks!

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Mar 26, 2014

Member

looks like it's the way to go http://www.html5rocks.com/en/tutorials/cors/#toc-making-a-cors-request.
I'm sure that setting onprogress to a noop function is necessary. Also, unncessary, is the need for duplicating the request.open call. It's exactly the same past the creation to the XHR/XDomainRequest object.

Member

gkatsev commented Mar 26, 2014

looks like it's the way to go http://www.html5rocks.com/en/tutorials/cors/#toc-making-a-cors-request.
I'm sure that setting onprogress to a noop function is necessary. Also, unncessary, is the need for duplicating the request.open call. It's exactly the same past the creation to the XHR/XDomainRequest object.

Show outdated Hide outdated src/js/lib.js
try {
request.open('GET', url);
request.withCredentials = true;

This comment has been minimized.

@dmlap

dmlap Mar 27, 2014

Member

Should this always be set?

@dmlap

dmlap Mar 27, 2014

Member

Should this always be set?

@skogby

This comment has been minimized.

Show comment
Hide comment
@skogby

skogby Mar 28, 2014

The withCredentials flag allows the request to include cookies, but could be configured with a parameter. Just noted yesterday as well that IE11 requires the request to be open before we can set the withCredentials flag.
request.open('GET', url);
request.withCredentials = true;

skogby commented Mar 28, 2014

The withCredentials flag allows the request to include cookies, but could be configured with a parameter. Just noted yesterday as well that IE11 requires the request to be open before we can set the withCredentials flag.
request.open('GET', url);
request.withCredentials = true;

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Mar 28, 2014

Member

That's weird that IE11 requires that. However, yes, it should be a configuration. We don't want to always set credentials to true because sometimes cookies are disabled or whatever.

Member

gkatsev commented Mar 28, 2014

That's weird that IE11 requires that. However, yes, it should be a configuration. We don't want to always set credentials to true because sometimes cookies are disabled or whatever.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 14, 2014

Member

@skogby do we still need to update this to open the request before setting withCredentials? And can we make it so that setting is configurable?

Member

heff commented Apr 14, 2014

@skogby do we still need to update this to open the request before setting withCredentials? And can we make it so that setting is configurable?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Apr 14, 2014

Member

Yes, we only want the withCredentials flag set if required. Otherwise, a lot of requests will fail.

Member

gkatsev commented Apr 14, 2014

Yes, we only want the withCredentials flag set if required. Otherwise, a lot of requests will fail.

@skogby

This comment has been minimized.

Show comment
Hide comment
@skogby

skogby Apr 15, 2014

Just pushed some updates that changed the order of "open" and "withCredentials" in addition to making it not being sent as default.

skogby commented Apr 15, 2014

Just pushed some updates that changed the order of "open" and "withCredentials" in addition to making it not being sent as default.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 2, 2014

I think I'm seeing this problem too with IE9 and subtitles hosted on a different sub-domain. Are these changes likely to get merged and released any time soon?

floehopper commented May 2, 2014

I think I'm seeing this problem too with IE9 and subtitles hosted on a different sub-domain. Are these changes likely to get merged and released any time soon?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 2, 2014

Member

Looks good. Can you also add the withCredentials flag (if needed) to the XDomainRequest? Line 590?

Member

gkatsev commented May 2, 2014

Looks good. Can you also add the withCredentials flag (if needed) to the XDomainRequest? Line 590?

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 4, 2014

@gkatsev XDomainRequest does not support withCredentials, so I don't think any change is needed. @skogby is that right?

floehopper commented May 4, 2014

@gkatsev XDomainRequest does not support withCredentials, so I don't think any change is needed. @skogby is that right?

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 4, 2014

As I mentioned above, I've run into this problem on IE9 when I have subtitles hosted on a different sub-domain. I've just built a test page using the code in this PR and I can confirm that it fixes my problem. Is there anything I can do to help get this merged and released?

floehopper commented May 4, 2014

As I mentioned above, I've run into this problem on IE9 when I have subtitles hosted on a different sub-domain. I've just built a test page using the code in this PR and I can confirm that it fixes my problem. Is there anything I can do to help get this merged and released?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 4, 2014

Member

Ah, cool. For some reason I thought it did support that property. Carry on then.

Member

gkatsev commented May 4, 2014

Ah, cool. For some reason I thought it did support that property. Carry on then.

floehopper added a commit to Futurelearn/videojs_rails that referenced this pull request May 6, 2014

Incorporate IE get xDomain fix.
The commit integrates the changes in [1] which are in turn based on this
video.js pull request [2]. This should fix a problem whereby cross-domain
Ajax GET requests from within video.js did not work for Internet
Explorer < v10.

This should mean that subtitles (which are hosted on a sub-domain of
`futurelearn.com` in production, i.e. `ugc.futurelearn.com`) work with
the video.js player on earlier versions of Internet Explorer.

[1] https://github.com/Futurelearn/video.js/compare/v4.5.2-with-IE-get-xDomain-fix
[2] videojs/video.js#1095
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 6, 2014

Member

This is working for me in everything but IE8, where it errors out now when loading captions. Can anyone else test this in IE8 and verify?

If you run a local server and test the index.html file (copied from index.html.example) in /sandbox/, you should be able to play the video, then turn on captions and see captions. But this now breaks in IE8.

I logged out the onError function in vjs.get, and in true IE8 fashion, no information is passed to the error handler. So I have no idea what might be causing this. But it is clear that IE8 is now using XDomainRequest in instead of the XMLHttpRequest/Msxml2.XMLHTTP shim it previously was. If there was a way to continue that when IE8 is used, that might fix this issue. I'm not sure if that's the best approach though.

screen shot 2014-05-06 at 4 28 29 pm

Member

heff commented May 6, 2014

This is working for me in everything but IE8, where it errors out now when loading captions. Can anyone else test this in IE8 and verify?

If you run a local server and test the index.html file (copied from index.html.example) in /sandbox/, you should be able to play the video, then turn on captions and see captions. But this now breaks in IE8.

I logged out the onError function in vjs.get, and in true IE8 fashion, no information is passed to the error handler. So I have no idea what might be causing this. But it is clear that IE8 is now using XDomainRequest in instead of the XMLHttpRequest/Msxml2.XMLHTTP shim it previously was. If there was a way to continue that when IE8 is used, that might fix this issue. I'm not sure if that's the best approach though.

screen shot 2014-05-06 at 4 28 29 pm

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 6, 2014

Member

So to clarify a little, IE8 does support native XMLHttpRequests (xhr1 not xhr2). So apparently XDomainRequest doesn't work in all the same situations XMLHttpRequest does. Are there clear times where we should be using one and not the other?

Member

heff commented May 6, 2014

So to clarify a little, IE8 does support native XMLHttpRequests (xhr1 not xhr2). So apparently XDomainRequest doesn't work in all the same situations XMLHttpRequest does. Are there clear times where we should be using one and not the other?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 7, 2014

Member

Basically, XDomainRequest is needed because XHR 1 doesn't understand CORS headers. So, XDomainRequest should be used whenever you're making a request to another domain, which is set up with CORS headers.

However, I think XDRs should be able to do whatever XHR 1 does for same domain requests as well (as long as the requests are simple as only GET and POST are supported by XDR.

Member

gkatsev commented May 7, 2014

Basically, XDomainRequest is needed because XHR 1 doesn't understand CORS headers. So, XDomainRequest should be used whenever you're making a request to another domain, which is set up with CORS headers.

However, I think XDRs should be able to do whatever XHR 1 does for same domain requests as well (as long as the requests are simple as only GET and POST are supported by XDR.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 7, 2014

Member

That's good to know. But for some reason XDR isn't working in the index.html example, while XHR is. If someone can try the example and verify that'd be helpful, so I know it's not just me.

Found this writeup, but I don't believe any of these should be affecting the index.html example.

XDomainRequest Gotchas
Some other things I came across that will save you a headache.
The server protocol must be the same as the calling page protocol.
This means you can’t make requests from file:// to http ://, http to https, or https to http (don’t ever do the latter)
I found this the hard way by trying to run my code locally without first spinning up my localhost server.
Only “text/plain” is supported for the request’s “Content Type header
No custom headers can be added to the request

Member

heff commented May 7, 2014

That's good to know. But for some reason XDR isn't working in the index.html example, while XHR is. If someone can try the example and verify that'd be helpful, so I know it's not just me.

Found this writeup, but I don't believe any of these should be affecting the index.html example.

XDomainRequest Gotchas
Some other things I came across that will save you a headache.
The server protocol must be the same as the calling page protocol.
This means you can’t make requests from file:// to http ://, http to https, or https to http (don’t ever do the latter)
I found this the hard way by trying to run my code locally without first spinning up my localhost server.
Only “text/plain” is supported for the request’s “Content Type header
No custom headers can be added to the request

heff and others added some commits May 7, 2014

More explicit MIME type note
Added a more explicit MIME type note in setup. Moved to a footnote and made comment more verbose to avoid confusion.
@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 8, 2014

@heff I haven't had a chance to try it on the example, but I have my own version working OK in IE8 including subtitles. In fact the reason I needed the fix was because my subtitles are hosted on a different domain. I don't know whether it's significant, but I'm instantiating the player via JS and passing a tracks array in the options. I'll try to test the example later today if get a chance.

floehopper commented May 8, 2014

@heff I haven't had a chance to try it on the example, but I have my own version working OK in IE8 including subtitles. In fact the reason I needed the fix was because my subtitles are hosted on a different domain. I don't know whether it's significant, but I'm instantiating the player via JS and passing a tracks array in the options. I'll try to test the example later today if get a chance.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 8, 2014

Member

Thanks @floehopper, that'd be helpful. I tried forcing my local server to send text/plain for vtt files, and that didn't help the issue. And IE8 is still passing zero information to the onerrror handler, so I'm not sure what to try next. I don't think anythings out of the ordinary with my example, especially since xmlhttprequest is working. I wonder if it's worth checking if the file is cross domain and only using xdomainrequest in that case.

Member

heff commented May 8, 2014

Thanks @floehopper, that'd be helpful. I tried forcing my local server to send text/plain for vtt files, and that didn't help the issue. And IE8 is still passing zero information to the onerrror handler, so I'm not sure what to try next. I don't think anythings out of the ordinary with my example, especially since xmlhttprequest is working. I wonder if it's worth checking if the file is cross domain and only using xdomainrequest in that case.

heff added some commits May 8, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 8, 2014

Member

I made some updates and submitted a pull request to this branch. dbmedialab/video.js#1

If anyone can test if these changes still work for them, that'd be great.

Member

heff commented May 8, 2014

I made some updates and submitted a pull request to this branch. dbmedialab/video.js#1

If anyone can test if these changes still work for them, that'd be great.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 9, 2014

@heff As I've mentioned [1] over on your PR, subtitles don't work any more on a different sub-domain since your most recent changes.

[1] dbmedialab#1 (comment)

floehopper commented May 9, 2014

@heff As I've mentioned [1] over on your PR, subtitles don't work any more on a different sub-domain since your most recent changes.

[1] dbmedialab#1 (comment)

Erik Skogby
Merge pull request #1 from heff/dbmedialab-feature/IE-get-xDomain
Changed to used XDomainRequest only when url is cross domain
Tested and it seems to be working, I currently don't have access to a IE9 computer though.
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 12, 2014

Member

Ok, I think I have this fixed in dbmedialab/video.js#2

Member

heff commented May 12, 2014

Ok, I think I have this fixed in dbmedialab/video.js#2

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 13, 2014

I'm going to test this now.

floehopper commented May 13, 2014

I'm going to test this now.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper May 13, 2014

Just to re-iterate what I've said on @heff's latest PR, I've created a new test page [1] for my use case (where subtitles are on a sub-domain) and tested that subtitles work OK on the following: Win XP/IE8, Win7/IE9, Win8/IE10, Win8.1/IE11, OSX10.9/Firefox29, OSX10.9/Safari7 & OSX10.9/Chrome34. Good work and thanks!

[1] http://code.gofreerange.com/video-test/video.js-vzaar-subtitles-3.html

floehopper commented May 13, 2014

Just to re-iterate what I've said on @heff's latest PR, I've created a new test page [1] for my use case (where subtitles are on a sub-domain) and tested that subtitles work OK on the following: Win XP/IE8, Win7/IE9, Win8/IE10, Win8.1/IE11, OSX10.9/Firefox29, OSX10.9/Safari7 & OSX10.9/Chrome34. Good work and thanks!

[1] http://code.gofreerange.com/video-test/video.js-vzaar-subtitles-3.html

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 13, 2014

Member

Woo! Thanks for testing it out. Going to pull this in then.

Member

heff commented May 13, 2014

Woo! Thanks for testing it out. Going to pull this in then.

@heff heff closed this in 047bd8f May 13, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 13, 2014

Member

This has been merged into master and will go out with the next minor release.

Member

heff commented May 13, 2014

This has been merged into master and will go out with the next minor release.

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