chore(*): cleanup msie handling; add support comments#15407
Conversation
1. The conditions checking the msie variable value have been simplified. There is e.g. no point to check if `msie <= 11` since there IE 12 won't ever exist. 2. Edge UA-sniffing has been added to tests (only!) where appropriate 3. Support comments for IE/Edge have been added.
| 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/) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Doesn't this need a .test(...)? Right now it will be true 100% of the time...
There was a problem hiding this comment.
@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:
- Browsers are trying to pretend they have nothing to do with IE; differently to e.g. Chrome which is imitated by lots of browsers.
- We don't do it via the lying user agent string but the non-standard
document.documentModethat other browsers will never implement. - 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.
There was a problem hiding this comment.
Ah, no, this particular condition was necessary, the tests are now failing. Fixing...
| 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); |
There was a problem hiding this comment.
Why is the msie < 11 check gone?
There was a problem hiding this comment.
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+.
|
@Narretz PR updated; PTAL. |
|
|
||
| var ALL_COLONS = /:/g; | ||
| function timezoneToOffset(timezone, fallback) { | ||
| // Support: IE 9-11 only, Edge 13-14+ |
There was a problem hiding this comment.
What is the difference between Edge 13-14+ and Edge 13+?
There was a problem hiding this comment.
In the former we've checked that it, indeed, is broken in both v13 & v14. In the latter we've just checked v13 and the v14 state is unknown.
| }); | ||
|
|
||
| // Support: IE 9-10 only | ||
| // IE <=11 don't support srcdoc |
|
|
||
| // Support: IE 9-10 only | ||
| // IE <=11 don't support srcdoc | ||
| if (!msie || msie >= 11) { |
There was a problem hiding this comment.
This could be !msie || msie === 11 (for consistency).
| } | ||
| // Support: IE 9 only | ||
| // IE 9 doesn't support strict mode so its `this` will always be defined. | ||
| if (msie < 10) return; |
| if (msie) return; | ||
| afterEach(function() { | ||
| // Support: non-Windows browsers | ||
| if (msie || /\bEdge\/[\d\.]+\b/.test(window.navigator.userAgent)) return; |
There was a problem hiding this comment.
It would be nice to have this as a reusable flag in tests.
There was a problem hiding this comment.
I guess it doesn't hurt when it's used just in tests... (I would definitely not want sth like that in source). But would you like to have it in tests globally or just in locationSpec? If globally then we should define similar variables for other browsers.
I'm a little afraid of making it too easy to use UA-sniffing, though.
There was a problem hiding this comment.
No, it doesn't hurt. It is just about the mental overhead of "deciphering" /\bEdge\/[\d\.]+\b/.test(window.navigator.userAgent) vs isEdge.
I'm a little afraid of making it too easy to use UA-sniffing, though.
Fair enough 😃
|
@Narretz PTAL, your review is blocking this. :) |
1. The conditions checking the msie variable value have been simplified. There is e.g. no point to check if `msie <= 11` since there IE 12 won't ever exist. 2. Edge UA-sniffing has been added to tests (only!) where appropriate 3. Support comments for IE/Edge have been added. Closes angular#15407
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Chore
What is the current behavior? (You can also link to an open issue here)
N/A
What is the new behavior (if this is a feature change)?
N/A
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
There is e.g. no point to check if
msie <= 11since there IE 12 won't everexist.