Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

script.onreadystatechange Does not exist in IE11 #4922

Closed
wants to merge 1 commit into from

Conversation

runxc1
Copy link

@runxc1 runxc1 commented Nov 12, 2013

In order for Jsonp to work correctly in IE11 this is needed.

In order for Jsonp to work correctly in IE11 this is needed.
@@ -135,7 +135,7 @@ function createHttpBackend($browser, XHR, $browserDefer, callbacks, rawDocument,
script.type = 'text/javascript';
script.src = url;

if (msie) {
if (msie && script.onreadystatechange) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSDN says IE11 should use "onload" for script tags. What about this?

if (msie) {
  script[script.onreadystatechange ? 'onreadystatechange' : 'onload'] = function() {
    if (/loaded|complete/.test(script.readyState)) doneWrapper();
  };
}

Or is it okay to just ignore it for IE11?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that's exactly one of those reasons for which browser sniffing is a bad idea. It's unfortunate if code can break just because a new version of a particular browser is released and the sniffing now takes incorrect assumptions.

If this code used feature detection, it would just work in IE11 without having to introduce any changes post-factum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess prior versions of MSIE didn't fire the load event for script tags which aren't added to the DOM or something? Not really sure, MSDN doesn't really offer much useful information about past versions, and I haven't found any issues about it in previous versions from google --- so I'm curious why we're actually using onreadystatechange at all there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caitp It's needed for IE8 and older, the load event is not fired there on script elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why a similar approach couldn't be adopted here until XP disappears from the face of the earth

@petebacondarwin
Copy link
Member

According to this page: http://pieisgood.org/test/script-link-events/
IE9 and IE10 support both onload and onreadystatechange so I don't think we can simply attach both event handlers.
Also, in the jQuery code, there is an isAbort parameter added to the handler. I can't find any documentation of this parameter anywhere, for any of onload, onerror or onreadystatechange so I reckon this is a hangover from the past that is not valid any more.
Since onreadystatechange is only needed for IE8 and below, I suggest that we do sniff but only for msie<=8.

@petebacondarwin
Copy link
Member

Also, the jQuery code sets the handlers to null once they have been called (an IE memory leak bug, it says). This seems like a sensible thing to do.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Nov 22, 2013
IE8, IE9 and IE10 can use `script.onreadystate` so up till now we have been using this
if the sniffer says we are on IE.
But IE11 now does not support `script.onreadystate` and only supports the more standard
`script.onload` and `script.onerror`.
IE9 and IE10 do support `script.onload` and `script.onerror`. So now we only test whether
we are on IE8 or earlier before using `script.onreadystate`.
See http://pieisgood.org/test/script-link-events/

jQuery just uses all these handlers at once and hopes for the best, but since IE9 and IE10
support both sets of handlers, this could cause the handlers to be run more than once.

jQuery also notes that there is a potential memory leak in IE unless we remove the handlers
from the script object once they are run.  So we are doing this too, now.

Closes angular#4523
Closes angular#4527
Closes angular#4922
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…SONP

IE8, IE9 and IE10 can use `script.onreadystate` so up till now we have been using this
if the sniffer says we are on IE.
But IE11 now does not support `script.onreadystate` and only supports the more standard
`script.onload` and `script.onerror`.
IE9 and IE10 do support `script.onload` and `script.onerror`. So now we only test whether
we are on IE8 or earlier before using `script.onreadystate`.
See http://pieisgood.org/test/script-link-events/

jQuery just uses all these handlers at once and hopes for the best, but since IE9 and IE10
support both sets of handlers, this could cause the handlers to be run more than once.

jQuery also notes that there is a potential memory leak in IE unless we remove the handlers
from the script object once they are run.  So we are doing this too, now.

Closes angular#4523
Closes angular#4527
Closes angular#4922
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…SONP

IE8, IE9 and IE10 can use `script.onreadystate` so up till now we have been using this
if the sniffer says we are on IE.
But IE11 now does not support `script.onreadystate` and only supports the more standard
`script.onload` and `script.onerror`.
IE9 and IE10 do support `script.onload` and `script.onerror`. So now we only test whether
we are on IE8 or earlier before using `script.onreadystate`.
See http://pieisgood.org/test/script-link-events/

jQuery just uses all these handlers at once and hopes for the best, but since IE9 and IE10
support both sets of handlers, this could cause the handlers to be run more than once.

jQuery also notes that there is a potential memory leak in IE unless we remove the handlers
from the script object once they are run.  So we are doing this too, now.

Closes angular#4523
Closes angular#4527
Closes angular#4922
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants