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
Closed

script.onreadystatechange Does not exist in IE11 #4922

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

BTW, jQuery 1.x just sets both: https://github.com/jquery/jquery/blob/1.x-master/src/ajax/script.js#L57-77

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

script.onreadystatechange = function() {
if (/loaded|complete/.test(script.readyState)) doneWrapper();
};
Expand Down