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

chore(*): cleanup msie handling; add support comments #15407

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ var
angularModule,
uid = 0;

// Support: IE 9-11 only
/**
* documentMode is an IE-only property
* http://msdn.microsoft.com/en-us/library/ie/cc196988(v=vs.85).aspx
Expand Down
3 changes: 2 additions & 1 deletion src/auto/injector.js
Original file line number Diff line number Diff line change
Expand Up @@ -849,8 +849,9 @@ function createInjector(modulesToLoad, strictDi) {
}

function isClass(func) {
// Support: IE 9-11 only
// IE 9-11 do not support classes and IE9 leaks with the code below.
if (msie <= 11 || typeof func !== 'function') {
if (msie || typeof func !== 'function') {
return false;
}
var result = func.$$ngIsClass;
Expand Down
1 change: 1 addition & 0 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1949,6 +1949,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
for (var i = 0; i < nodeList.length; i++) {
attrs = new Attributes();

// Support: IE 11 only
// Workaround for #11781 and #14924
if (msie === 11) {
mergeConsecutiveTextNodes(nodeList, i, notLiveList);
Expand Down
5 changes: 3 additions & 2 deletions src/ng/directive/attrs.js
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,11 @@ forEach(['src', 'srcset', 'href'], function(attrName) {

attr.$set(name, value);

// on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
// Support: IE 9-11 only
// On IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
// then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
// to set the property as well to achieve the desired effect.
// we use attr[attrName] value since $set can sanitize the url.
// We use attr[attrName] value since $set can sanitize the url.
if (msie && propName) element.prop(propName, attr[name]);
});
}
Expand Down
1 change: 1 addition & 0 deletions src/ng/rootScope.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ function $RootScopeProvider() {

function cleanUpScope($scope) {

// Support: IE 9 only
if (msie === 9) {
// There is a memory leak in IE9 if all child scopes are not disconnected
// completely when a scope is destroyed. So this code will recurse up through
Expand Down
1 change: 1 addition & 0 deletions src/ng/sce.js
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ function $SceProvider() {

this.$get = ['$parse', '$sceDelegate', function(
$parse, $sceDelegate) {
// Support: IE 9-11 only
// Prereq: Ensure that we're not running in IE<11 quirks mode. In that mode, IE < 11 allow
// the "expression(javascript expression)" syntax which is insecure.
if (enabled && msie < 8) {
Expand Down
3 changes: 2 additions & 1 deletion src/ng/sniffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,13 @@ function $SnifferProvider() {
// We are purposefully using `!(android < 4)` to cover the case when `android` is undefined
history: !!(hasHistoryPushState && !(android < 4) && !boxee),
hasEvent: function(event) {
// Support: IE 9-11 only
// IE9 implements 'input' event it's so fubared that we rather pretend that it doesn't have
// it. In particular the event is not fired when backspace or delete key are pressed or
// when cut operation is performed.
// IE10+ implements 'input' event but it erroneously fires under various situations,
// e.g. when placeholder changes, or a form is focused.
if (event === 'input' && msie <= 11) return false;
if (event === 'input' && msie) return false;

if (isUndefined(eventSupport[event])) {
var divElm = document.createElement('div');
Expand Down
1 change: 1 addition & 0 deletions src/ng/urlUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ var originUrl = urlResolve(window.location.href);
function urlResolve(url) {
var href = url;

// Support: IE 9-11 only
if (msie) {
// Normalize before parse. Refer Implementation Notes on why this is
// done in two steps on IE.
Expand Down
7 changes: 7 additions & 0 deletions src/ngScenario/dsl.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ angular.scenario.dsl('binding', function() {
*/
angular.scenario.dsl('input', function() {
var chain = {};

// Support: IE 9-11 only
// IE9 implements 'input' event it's so fubared that we rather pretend that it doesn't have
// it. In particular the event is not fired when backspace or delete key are pressed or
// when cut operation is performed.
// IE10+ implements 'input' event but it erroneously fires under various situations,
// e.g. when placeholder changes, or a form is focused.
var supportInputEvent = 'oninput' in window.document.createElement('div') && !msie;

chain.enter = function(value, event) {
Expand Down
5 changes: 5 additions & 0 deletions test/helpers/matchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -357,15 +357,19 @@ beforeEach(function() {

toBeMarkedAsSelected: function() {
// Selected is special because the element property and attribute reflect each other's state.

// Support: IE 9 only
// IE9 will wrongly report hasAttribute('selected') === true when the property is
// undefined or null, and the dev tools show that no attribute is set

return {
compare: function(actual) {
var errors = [];
if (actual.selected === null || typeof actual.selected === 'undefined' || actual.selected === false) {
errors.push('Expected option property "selected" to be truthy');
}

// Support: IE 9 only
if (msie !== 9 && actual.hasAttribute('selected') === false) {
errors.push('Expected option to have attribute "selected"');
}
Expand All @@ -383,6 +387,7 @@ beforeEach(function() {
errors.push('Expected option property "selected" to be falsy');
}

// Support: IE 9 only
if (msie !== 9 && actual.hasAttribute('selected')) {
errors.push('Expected option not to have attribute "selected"');
}
Expand Down
10 changes: 8 additions & 2 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1127,7 +1127,8 @@ describe('$compile', function() {
expect(element).toHaveClass('class_2');
}));

if (!msie || msie > 11) {
// Support: IE 9-11 only
if (!msie) {
// style interpolation not working on IE (including IE11).
it('should handle interpolated css style from replacing directive', inject(
function($compile, $rootScope) {
Expand Down Expand Up @@ -10698,6 +10699,8 @@ describe('$compile', function() {
expect(element.text()).toBe('102030');
expect(newWatcherCount).toBe(3);

// Support: IE 11 only
// See #11781 and #14924
if (msie === 11) {
expect(element.find('ng-transclude').contents().length).toBe(1);
}
Expand All @@ -10719,9 +10722,10 @@ describe('$compile', function() {
expect(element.attr('src')).toEqual('http://example.com/image2.png');
}));

// Support: IE 9 only
// IE9 rejects the video / audio tag with "Error: Not implemented" and the source tag with
// "Unable to get value of the property 'childNodes': object is null or undefined"
if (!msie || msie > 9) {
if (msie !== 9) {
they('should NOT require trusted values for $prop src', ['video', 'audio'],
function(tag) {
inject(function($rootScope, $compile, $sce) {
Expand Down Expand Up @@ -11154,6 +11158,8 @@ describe('$compile', function() {
}));
});

// Support: IE 9-10 only
// IE <=11 don't support srcdoc
Copy link
Member

Choose a reason for hiding this comment

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

<= --> <

if (!msie || msie >= 11) {
Copy link
Member

Choose a reason for hiding this comment

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

This could be !msie || msie === 11 (for consistency).

describe('iframe[srcdoc]', function() {
it('should NOT set iframe contents for untrusted values', inject(function($compile, $rootScope, $sce) {
Expand Down
6 changes: 4 additions & 2 deletions test/ng/directive/booleanAttrsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ describe('ngSrc', function() {
}));


// Support: IE 9-11 only
if (msie) {
it('should update the element property as well as the attribute', inject(
function($compile, $rootScope, $sce) {
Expand Down Expand Up @@ -283,8 +284,9 @@ describe('ngHref', function() {
expect(element.attr('href')).toEqual(undefined);
}));

if (msie) {
// IE11/10/Edge fail when setting a href to a URL containing a % that isn't a valid escape sequence
// Support: IE 9-11 only, Edge 12-14+
if (msie || /\bEdge\/[\d\.]+\b/) {
Copy link
Contributor

@Narretz Narretz Nov 17, 2016

Choose a reason for hiding this comment

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

Wouldn't it make more sense to put Edge in a variable, same as msie? This will also be useful once we finally test on Edge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need a .test(...)? Right now it will be true 100% of the time...

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbedard Nice catch. This means, though, that we can just remove the if and run the test everywhere.

@Narretz A variable would make it easier to reuse and we should avoid UA-sniffing (it's more acceptable in tests but still); it's not robust, it can catch more browsers than intended and cause compatibility concerns. This is actually a good example as the if was unnecessary, I'll remove it.

IE sniffing is different as:

  1. Browsers are trying to pretend they have nothing to do with IE; differently to e.g. Chrome which is imitated by lots of browsers.
  2. We don't do it via the lying user agent string but the non-standard document.documentMode that other browsers will never implement.
  3. There are lots of differences between IE & modern browsers and the gap will only get larger. Edge gets updated and doesn't lag so much so it won't apply to it.

IE sniffing is, therefore, one of the few safe browser sniffing available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, no, this particular condition was necessary, the tests are now failing. Fixing...

// IE/Edge fail when setting a href to a URL containing a % that isn't a valid escape sequence
// See https://github.com/angular/angular.js/issues/13388
it('should throw error if ng-href contains a non-escaped percent symbol', inject(function($rootScope, $compile) {
element = $compile('<a ng-href="http://www.google.com/{{\'a%link\'}}">')($rootScope);
Expand Down
1 change: 1 addition & 0 deletions test/ng/directive/inputSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ describe('input', function() {


describe('IE placeholder input events', function() {
// Support: IE 9-11 only
//IE fires an input event whenever a placeholder visually changes, essentially treating it as a value
//Events:
// placeholder attribute change: *input*
Expand Down
16 changes: 10 additions & 6 deletions test/ng/locationSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ describe('$location', function() {
/* global urlParsingNode: true */
var urlParsingNodePlaceholder;

beforeEach(inject(function($sniffer) {
if (msie) return;
beforeEach(function() {
// Support: non-Windows browsers
// These tests expect a Windows environment which we can only guarantee
// on IE & Edge.
if (msie || /\bEdge\/[\d\.]+\b/) return;

urlParsingNodePlaceholder = urlParsingNode;

Expand All @@ -57,13 +60,14 @@ describe('$location', function() {
search: '',
setAttribute: angular.noop
};
}));
});

afterEach(inject(function($sniffer) {
if (msie) return;
afterEach(function() {
// Support: non-Windows browsers
if (msie || /\bEdge\/[\d\.]+\b/) return;
//reset urlParsingNode
urlParsingNode = urlParsingNodePlaceholder;
}));
});


it('should not include the drive name in path() on WIN', function() {
Expand Down
3 changes: 2 additions & 1 deletion test/ng/parseSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2180,8 +2180,9 @@ describe('parser', function() {
expect(scope.$eval('getter()()')).toBe(33);
});

// Support: IE 9 only
// There is no "strict mode" in IE9
if (!msie || msie > 9) {
if (msie !== 9) {
it('should set no context to functions returned by other functions', function() {
scope.getter = function() { return function() { expect(this).toBeUndefined(); }; };
scope.$eval('getter()()');
Expand Down
7 changes: 4 additions & 3 deletions test/ng/rootScopeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ describe('Scope', function() {


it('should expose the constructor', inject(function($rootScope) {
if (msie < 11) return;
// eslint-disable-next-line no-proto
expect($rootScope.__proto__).toBe($rootScope.constructor.prototype);
expect(Object.getPrototypeOf($rootScope)).toBe($rootScope.constructor.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the msie < 11 check gone?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was there only because of the Annex B (i.e. compatibility "don't use" stuff) __proto__ that wasn't present in IE <11. The standard ES5+ way to get the prototype is Object.getPrototypeOf(object) which works in IE9+.

}));


Expand Down Expand Up @@ -125,6 +123,8 @@ describe('Scope', function() {
function Listener() {
expect(this).toBeUndefined();
}
// Support: IE 9 only
// IE 9 doesn't support strict mode so its `this` will always be defined.
if (msie < 10) return;
Copy link
Member

Choose a reason for hiding this comment

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

This could be msie === 9.

$rootScope.$watch(Getter, Listener);
$rootScope.$digest();
Expand Down Expand Up @@ -1227,6 +1227,7 @@ describe('Scope', function() {
}));


// Support: IE 9 only
if (msie === 9) {
// See issue https://github.com/angular/angular.js/issues/10706
it('should completely disconnect all child scopes on IE9', inject(function($rootScope) {
Expand Down
3 changes: 2 additions & 1 deletion test/ng/snifferSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,12 @@ describe('$sniffer', function() {


it('should claim that IE9 doesn\'t have support for "oninput"', function() {
// Support: IE 9-11 only
// IE9 implementation is fubared, so it's better to pretend that it doesn't have the support
// IE10+ implementation is fubared when mixed with placeholders
mockDivElement = {oninput: noop};

expect($sniffer.hasEvent('input')).toBe(!(msie && msie <= 11));
expect($sniffer.hasEvent('input')).toBe(!msie);
});
});

Expand Down