Skip to content
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

Fix 2 issues breaking IE8 compatibility #764

Merged
merged 6 commits into from
Mar 17, 2017
65 changes: 53 additions & 12 deletions velocity.js
Original file line number Diff line number Diff line change
Expand Up @@ -525,29 +525,70 @@
return result;
}

/**
* Shim for "fixing" IE's lack of support (IE < 9) for applying slice
* on host objects like NamedNodeMap, NodeList, and HTMLCollection
* (technically, since host objects have been implementation-dependent,
* at least before ES2015, IE hasn't needed to work this way).
* Also works on strings, fixes IE < 9 to allow an explicit undefined
* for the 2nd argument (as in Firefox), and prevents errors when
* called on other DOM objects.
*/
var _slice = (function() {
var slice = Array.prototype.slice;
var _fslice = Array.prototype.slice;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need to change the function name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll change that.


try {
// Can't be used with DOM elements in IE < 9
slice.call(document.documentElement);
_fslice.call(document.documentElement);
} catch (e) { // Fails in IE < 9
// This will work for genuine arrays, array-like objects,
// NamedNodeMap (attributes, entities, notations),
// NodeList (e.g., getElementsByTagName), HTMLCollection (e.g., childNodes),
// and will not fail on other DOM objects (as do DOM elements in IE < 9)
slice = function() {
var i = this.length,
clone = [];
Array.prototype.slice = function(begin, end) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't override global Array.slice - other things might be doing it - make sure you find places using Array.slice() and have them use this instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Array.slice() is failing in the first place (because it doesn't exist) I don't see the harm in making it work by defining it globally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no reason for it to fail, if you look at https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/slice#Browser_compatibility then it's pretty much always been supported - so if it's not there then assigning it is actually changing something that is out of Velocity's scope (such as replacing Array entirely) - hence safer to leave it broken and sit quietly in our own little corner and ignoring the broken site ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree. Slice is supported in modern browsers, but that excludes IE8 (a relic that we still support for the next couple of years). And thus users using slice on IE8 will be using a similar shim to gain that ability on IE8. Modern browsers won't have their implementation of slice replaced at all (because the catch() block won't be called).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not our place to shim anything globally - if they want to shim it, then they can decide to do so, not get it as a side-effect of using an animation library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll rework that.

// IE < 9 gets unhappy with an undefined end argument
end = (typeof end !== 'undefined') ? end : this.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(end !== undefined) - fully supported and slightly nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I'll make that change.


// For native Array objects, we use the native slice function
if (Object.prototype.toString.call(this) === '[object Array]'){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just if(this.slice){ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that's better. I'll make that change.

return _fslice.call(this, begin, end);
}

// For array like object we handle it ourselves.
var i, cloned = [], size, len = this.length;

while (--i > 0) {
clone[i] = this[i];
// Handle negative value for "begin"
var start = begin || 0;
start = (start >= 0) ? start : Math.max(0, len + start);

// Handle negative value for "end"
var upTo = (typeof end == 'number') ? Math.min(end, len) : len;
if (end < 0) {
upTo = len + end;
}
return clone;

// Actual expected size of the slice
size = upTo - start;

if (size > 0) {
cloned = new Array(size);
if (this.charAt) {
for (i = 0; i < size; i++) {
cloned[i] = this.charAt(start + i);
}
} else {
for (i = 0; i < size; i++) {
cloned[i] = this[start + i];
}
}
}

return cloned;
};
}
return slice;
})(); // TODO: IE8, Cache of Array.prototype.slice that works on IE8

return Array.prototype.slice;
})();

function sanitizeElements(elements) {
/* Unwrap jQuery/Zepto objects. */
Expand Down Expand Up @@ -1447,7 +1488,7 @@
getUnit: function(str, start) {
var unit = (str.substr(start || 0, 5).match(/^[a-z%]+/) || [])[0] || "";

if (unit && CSS.Lists.units.indexOf(unit) >= 0) {
if (unit && $.inArray(unit, CSS.Lists.units) >= 0) {
Copy link
Collaborator

@Rycochet Rycochet Mar 17, 2017

Choose a reason for hiding this comment

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

Gah - just had a look, and $.inArray is jQuery only - we don't shim it - if you take these out I'll add a shim when I get home then do the release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll remove that from my branch.

return unit;
}
return "";
Expand Down Expand Up @@ -3856,7 +3897,7 @@

/* Find shorthand color properties that have been passed a hex string. */
/* Would be quicker to use CSS.Lists.colors.includes() if possible */
if (CSS.Lists.colors.indexOf(propertyName) >= 0) {
if ($.inArray(propertyName, CSS.Lists.colors) >= 0) {
/* Parse the value data for each shorthand. */
var endValue = valueData[0],
easing = valueData[1],
Expand Down