Prototype.js - A Review Tuesday, 17 June 2008
I was prompted to do a code review on PrototypeJS. Here is a partial review.
Ten Issues
- Modification of Host Object - Non standard, doesn't work the same in all browsers. Don't do it!
- Class.Methods.addMethods -
- Don't rely on a Function's
toString! - Don't introduce special behavior based on function Decompilation!
- The mimicing of class based inheritance through closures to get
$superto work requires a significantly augmented scope chain.
- Don't rely on a Function's
- The Dollar Function - Meaningless and extremely inefficient
- Element.Methods.visible - misnamed, non-reflective, and buggy/unreliable!
- More Modifications to Host objects
- Object.extend - Don't forget the JScript DontEnum bug!
- $A - Relying on object's toString -
- readAttribute - Don't rely on Function Decompilation
- Ajax.Request - Add an Underscore?
- Broken unescaping of HTML Strings
-
Unnecessary use of
withstatement - Enumerable.include - Strict Equality or 2n with Loose Equality
- getDimensions - Don't Expect clientWidth to be non-zero in IE
- cumulativeOffset - doesn't account for borders, scroll offsets, or magic BODY.
Okay, so it's more than 10, and could be even more, but I have to stop somewhere!
Modification of Host Object
A Host object is an object provided by the Host environment, e.g. document, event, an element.
PrototypeJS relies on modifying Host objects as a part of its core approach. There are several problems with this approach.
- There is no requirement in the EcmaScript specification that requires Host objects to be modifiable (although they usually are).
- There is no requirement in the EcmaScript specification that the Host environment expose for its Host objects a constructor with an associated prototype.
-
There is no requirement in the EcmaScript specification that objects expose
a
__proto__property. -
There is no guarantee by any standard that there will be a
__proto__property present for anElementor what value, if any, it will hold.
The fact that Mozilla exposes the prototypes of Element, et c is useful in that it allows developers to provide quick patches for new or broken features that lack proper support. However, this feature can't be reliably used for websites that are expected to run in multiple browsers.
Class.Methods.addMethods
Class.Methods.addMethods relies on the approach of calling toString on a function,
expecting to get back the source code, as it originally appeared.
if(value.argumentNames().first() == "$super")
A few significant problems:
-
Function.prototype.toStringis not required to return the source code of the functionFunction.prototype.toString is guaranteed only to return an implementation-dependent representation of the function as a FunctionDeclaration [or FunctionExpression] (spec errata, has been fixed). (§15.3.4.2).
Opera mobile follows the spec but takes the liberty of not returning the source code of the function.
-
Even if relying on the name of a function's formal parameter were guaranteed,
doing so would make implementation code fragile because it would be impossible for a human
(or compressor) to rename it (YUI Compressor bug).
Object.extend(Function.prototype, { argumentNames: function() { var names = this.toString().match(/^[\s\(]*function[^(]*\((.*?)\)/)[1].split(",").invoke("strip"); return names.length == 1 && !names[0] ? [] : names; },
Providing a toString that returns helpful debugging info
for objects is generally good advice.
However, the return value of a function's toString cannot be expected to be anything
other than a string value. It should not be relied on.
Unfortunately
argumentNames will choke on a
Function object that has a custom toString. This problem will happen in every browser.
Example: Programmer-defined toString Causes Problem with Object.extend
function WidgetFactory(){}
WidgetFactory.toString = function(){ return"[WidgetFactory constructor]"; };
If argumentNames() is called on WidgetFactory, the first call to match() will return null. Then, when
split() is called, a TypeError will be thrown.
It would be less error-prone for PrototypeJS to use:
var names = Function.prototype.toString.call(someFunction);
This avoids the mistake of relying on the function instance's toString,
however, the approach still has two significant problems:
- relies on the source code and named formal parameters of the function on which it is called (problem 1).
-
Function.prototype.toStringis still not required to return the source code of the function Function.prototype.toString is guaranteed to return an implementation-dependent representation of the function as a FunctionDeclaration [or FunctionExpression] (spec errata). (§15.3.4.2), and in fact, Opera mobile follows the spec correctly, but takes the liberty of not returning the source code of the function.
Relying on Function Objects' toString
Do Not Rely on Return Values from Function Objects' toString!
In general, toString should not be parsed or relied upon for data formats.
toString is useful for debugging. In October 2007, I
pointed out how Dojo and jQuery made this mistake. Hallvord has been writing against relying on Function Decompilation for
well over a year, as it resulted in problems in PrototypeJS and jQuery running in Opera Mobile.
The Dollar Function
PrototypeJS is built on the approach of modifying Host objects. This is the cornerstone of the library's problems. The library will try to modify the built-in prototype of an XPConnect wrapped prototype if it assumes the browser can do that. Otherwise, it will add properties to the element itself.
The primary accessor to these modified Host objects is through that dollar function.
Law of Demeter
Each unit should have only limited knowledge about other units: only units "closely" related to the current unit. (LoD).
I'll come back to explain how Prototype.js' modifications to Host objects violate LoD, and the problems associated with that.
What Does $ Do?
- $ does not have a clearly defined meaning as to what the function actually does.
- The dollar sign is intended to be reserved for machine-generated code.
PrototypeJS $ function gets an element or array of elements. Calling $ results in a bare minimum of three function calls: $, isString, and Element.extend and a maximum of over 135 function calls.
function $(element) {
if (arguments.length > 1) {
for (var i = 0, elements = [], length = arguments.length; i < length; i++)
elements.push($(arguments[i]));
return elements;
}
if (Object.isString(element))
element = document.getElementById(element);
return Element.extend(element);
}
Call Stack of $ (best case)
$ -> isString -> document.getElementById -> Element.extend
Count of Functions from $ (best case)
$................................+1
+--isString.....................+1
+--document.getElementById.....(+1) (when isString(element) is true)
+--Element.extend...............+1
~--Prototype.K||(anonymous)...0 (alias, second time, only. Prototype.K only if SpecificElementExtensions)
Sub-total_________________________3 (4 when isString(element) is true).
Calling $(document) then $(document), there will be many function calls the first time, and never any fewer than three calls the second time.
Element.extend = (function() {
if (Prototype.BrowserFeatures.SpecificElementExtensions)
return Prototype.K;
var Methods = { }, ByTag = Element.Methods.ByTag;
Object.extend(function(element) {
if (!element || element._extendedByPrototype ||
element.nodeType != 1 || element == window) return element;
var methods = Object.clone(Methods),
tagName = element.tagName, property, value;
// extend methods for specific tags
if (ByTag[tagName]) Object.extend(methods, ByTag[tagName]);
for (property in methods) {
value = methods[property];
if (Object.isFunction(value) && !(property in element))
element[property] = value.methodize();
}
element._extendedByPrototype = Prototype.emptyFunction;
return element;
}, {
refresh: function() {
// extend methods for all tags (Safari doesn't need this)
if (!Prototype.BrowserFeatures.ElementExtensions) {
Object.extend(Methods, Element.Methods);
Object.extend(Methods, Element.Methods.Simulated);
}
}
extend.refresh();
return extend;
})();
Element.extend, first call
However, during the first call to $(document), there is a check to Prototype.BrowserFeatures.SpecificElementExtensions. This is done because when SpecificElementExtensions is true, then the Library attempts to add the properties to DOM interface prototypes, e.g. HTMLHeadingElement.prototype. This happens elsewhere in the code, only when a __proto__ property returns a truthy value on created elements. If this feature is supported, the author has makes modifications to the DOM object interfaces, e.g. "HTML" + element.tagName + "Element", et c, but only when the userAgent does not match /Apple.*Mobile.*Safari/. (He makes said modifications elsewhere in the code, Element.addMethods -> findDOMClass).
Element.extend's closure calls Object.extend (needs review) to add the refresh() method to element.extend (rather than perform a simple assignment). The closure then calls extend.refresh() before returning the method extend, which gets assigned to Element.extend.
+ Element.extend's closure -> Object.extend -> extend.refresh -> Object.extend...3
Object.extend...1
subtotal:_____________________________________________9
calls: 5, Depth: 3
These calls are done when the file is loaded. They don't affect performance of $.
After extend has been assigned to Element.extend, it is invoked. Element.extend calls Object.clone, which calls Object.extend, then ByTag, Object.extend, n calls to isFunction, n calls to methodize, where n is the number of properties of methods. The element is then "methodized", which adds Ajax, et c to a FORM element.
$ -> isString..........................................(2)
document.getElementById...........................(+1) (when isString(element) is true)
Element.extend -> Object.clone -> Object.extend..(2)
ByTag -> Object.extend.........(2) (depends on tagName).
isFunction.....................(n = Size(methods) = 64 + 8)
methodize......................(n = Size(methods) = 64 + 8)
Total:_________________________________________________151
Calling $("sp-searchtext") results in 151 function calls the first time it is called. Non-form elements will have 133 calls the first time.
This happens for every object that is got by dollar function for browsers that don't support modifying DOM prototypes.
It considerably more complex and inefficient the first time around, when SpecificElementExtensions is false:
Object.extend(Methods, Element.Methods); Object.extend(Methods, Element.Methods.Simulated);
Where is $ Used Internally?
All of the Element.methods of PrototypeJS functions (the ones that got "methodized" in dollar,
use the dollar function, as well, adding extra overhead of a function call.
This allows each of Element.methods to be used as a static method. This also adds considerable extra
cost, though, to each call.
Element.Methods = {
// (GS) visibility is not display!
visible: function(element) {
// (GS) style properties do not reflect element state.
// The value 'inherit' is not considered.
return $(element).style.display != 'none';
},
Element.Methods.visible does not deal with CSS visibility, but instead
returns the style.display != 'none'. The
display property does not reflect the currentStyle or computed style.
Also, the CSS
display property
can have the value inherit.
Other Element.Methods functions that use the dollar function are more expensive. For example:
// (GS) Avoid this long chains of calls. Hard to debug. // (GS) calling Element.extend on each element is expensive, // up to 1600+ function calls for a FORM with 10 elements in IE. descendants: function(element) { return $A($(element).getElementsByTagName('*')).each(Element.extend); }, ... var Enumerable = { each: function(iterator, context) { var index = 0; iterator = iterator.bind(context); try { this._each(function(value) { iterator(value, index++); }); } catch (e) { if (e != $break) throw e; } return this; }, ... Object.extend(Array.prototype, { _each: function(iterator) { for (var i = 0, length = this.length; i < length; i++) iterator(this[i]); },
Considering a FORM with 10 input elements and nothing else,
there will be:
Element.descendants...................1
$A-->Array........................2
+--$.............................151
+--getElementsByTagName('*')..1
each.............................1 x 10 (ten INPUT elements)
+--bind.........................1 x 10
+--_each........................151 x 10 (151 methods, ten INPUT elements)
TOTAL.................................1685
One thousand, six hundred, and eighty-five function calls.
The call to isFunction in Element.extend might be something that could be refactored:
value = methods[property]; Object.isFunction(value)
Variable methods is a collection of functions.
Calling Object.isFunction(value) should always return true here and this is something
that the author can have control over because he owns methods (Law of Demeter).
Always Use Dollar
When using PrototypeJS, it is uncertain what properties will be present on an Element. This is because PrototypeJS may have already modified that element or its associated prototype. This dilemma of ambiguity can be avoided by the PrototypeJS user always using the dollar function and depending on PrototypeJS.
$("adiv").parentNode.show(), will have unexpected results, and will result in error in IE,
if the parentNode has not been previously modified via Element.extend()
$($('adiv').parentNode), will result in no less than seven function calls, and that is only after
$('adiv') has been called and
$($('adiv').parentNode) has been called.
More Modifications to Host Objects
I'm going to back up a little bit and look at details I skipped over, the modification Host objects.
LoD Recap
If the implementation's
internal code undergoes change, or if other browsers provide similar properties with
slightly different implementation then the code that relies on
assumptions of implementation-specific details based on those properties (col.__proto__)
will fail. Implementations have been known to change, in this regard
(bug 390411)
function findDOMClass(tagName) {
var klass;
var trans = {
"OPTGROUP": "OptGroup", "TEXTAREA": "TextArea", "P": "Paragraph",
// (GS) et c...
};
if (trans[tagName]) klass = 'HTML' + trans[tagName] + 'Element';
// (GS) Does not provide any feature detection about the object.
if (window[klass]) return window[klass];
klass = 'HTML' + tagName + 'Element';
if (window[klass]) return window[klass];
klass = 'HTML' + tagName.capitalize() + 'Element';
if (window[klass]) return window[klass];
window[klass] = { };
// (GS) Does not provide any feature detection about the object.
window[klass].prototype = document.createElement(tagName).__proto__;
return window[klass];
}
if (F.ElementExtensions) {
copy(Element.Methods, HTMLElement.prototype);
copy(Element.Methods.Simulated, HTMLElement.prototype, true);
}
if (F.SpecificElementExtensions) {
for (var tag in Element.Methods.ByTag) {
var klass = findDOMClass(tag);
if (Object.isUndefined(klass)) continue;
copy(T[tag], klass.prototype);
}
}
The findDOMClass assumes that the window will have an object based on tagName
or that if this is not the case, then the element's __proto__ property will be readable.
There is no guarantee by any standard that there will be a __proto__
property present or what value it will hold, if any.
There is no guarantee by any standard of an HTMLTableRowElement on window, nor any guarantee of what modifying its prototype will have. The library makes broad assumptions and performs no capability tests.
Replace the Host Environment's Element?
Not satisfied by altering over certain Host environments' Element
with its own properties,
PrototypeJS seeks to create a constructor to replace those environments' Element
with its own, copying over all enumerable properties from the Host's Element.
In Firefox, this includes all of QueryInterface.
This creates the dilemma of having a debilitated Element. Where previously, in
Firefox,
Element was native code, and Element.prototype.TEXT_NODE had the value
3, now, Element is a PrototypeJS constructor function and
Element.prototype.TEXT_NODE is undefined.
(function() {
var element = this.Element;
this.Element = function(tagName, attributes) {
attributes = attributes || { };
tagName = tagName.toLowerCase();
var cache = Element.cache;
if (Prototype.Browser.IE && attributes.name) {
tagName = '<' + tagName + ' name="' + attributes.name + '">';
delete attributes.name;
return Element.writeAttribute(document.createElement(tagName), attributes);
}
if (!cache[tagName]) cache[tagName] = Element.extend(document.createElement(tagName));
return Element.writeAttribute(cache[tagName].cloneNode(false), attributes);
};
Object.extend(this.Element, element || { });
}).call(window);
Element.cache = { };
Event.extend - Don't do It!
The same approach is used here
Event.extend = (function() {
var methods = Object.keys(Event.Methods).inject({ }, function(m, name) {
m[name] = Event.Methods[name].methodize();
return m;
});
if (Prototype.Browser.IE) {
Object.extend(methods, {
stopPropagation: function() { this.cancelBubble = true },
preventDefault: function() { this.returnValue = false },
inspect: function() { return "[object Event]" }
});
return function(event) {
if (!event) return false;
if (event._extendedByPrototype) return event;
event._extendedByPrototype = Prototype.emptyFunction;
var pointer = Event.pointer(event);
Object.extend(event, {
target: event.srcElement,
relatedTarget: Event.relatedTarget(event),
pageX: pointer.x,
pageY: pointer.y
});
return Object.extend(event, methods);
};
} else {
// (GS) Event.prototype is not guaranteed to be available in
// any browser.
Event.prototype = Event.prototype || document.createEvent("HTMLEvents").__proto__;
Object.extend(Event.prototype, methods);
return Prototype.K;
}
})();
Object.extend - Doesn't Account for JScript DontEnum Bug
Object.extend = function(destination, source) {
for (var property in source)
// (GS) Does not account for JScript DontEnum bug.
destination[property] = source[property];
return destination;
};
In IE, the keys of objects are skipped
without properly checking the DontEnum
flag. The skipped properties include
the useful and often overridden toString and valueOf.
Special and careful considerations should be made to address this problem.
$A - Don't Rely on The Return Value of An Object's toString
if (Prototype.Browser.WebKit) {
$A = function(iterable) {
if (!iterable) return [];
// (GS) Do not on the return value of an object's toString.
if (!(Object.isFunction(iterable) && iterable == '[object NodeList]') &&
iterable.toArray) return iterable.toArray();
var length = iterable.length || 0, results = new Array(length);
while (length--) results[length] = iterable[length];
return results;
};
}
A toString() with the return value of "[object NodeList]" implies nothing else about the object it was called on.
The readAttribute function, branched for Internet Explorer.
Example
Example page: http://www.apple.com/itunes/
javascript:alert($(document.body).readAttribute("onload"))
Will result in partial string representation of a serialized function. The string begins with ")".
_getEv: function(element, attribute) {
// (GS) Do not rely on function decompilation.
var attribute = element.getAttribute(attribute);
// (GS) Do not prevent yourself from renaming a function.
// (GS) Broken - slice starts at wrong position.
return attribute ? attribute.toString().slice(23, -2) : null;
},
Function readAttribute calls getAttribute on the element. In Internet Explorer,
calling getAttribute always reflects the property with the same name.
In this case the property has the value of the function object, loadShortcuts.
It isn't clear why the substring of 23 should be taken from the function's source code. The approach of relying on the toString of an object defined elsewhere in the code makes it hard to rename that function.
Ajax.Request - Add an Underscore?
request: function(url) {
this.url = url;
this.method = this.options.method;
var params = Object.clone(this.options.parameters);
if (!['get', 'post'].include(this.method)) {
// simulate other verbs over post
params['_method'] = this.method;
this.method = 'post';
}
this.parameters = params;
if (params = Object.toQueryString(params)) {
// when GET, append parameters to URL
if (this.method == 'get')
this.url += (this.url.include('?') ? '&' : '?') + params;
// (GS) All browsers support properly-formatted URIs.
// (GS) Do not add extra "_" parameter for some browsers.
else if (/Konqueror|Safari|KHTML/.test(navigator.userAgent))
params += '&_=';
}
Broken Unescaping of HTML Strings
PrototypeJS adds escapeHTML and unescapeHTML to String.prototype.
The problem with this code was discussed quite some time ago on
comp.lang.javascript.
if (Prototype.Browser.WebKit || Prototype.Browser.IE) Object.extend(String.prototype, {
escapeHTML: function() {
return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');
},
unescapeHTML: function() {
return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');
}
});
// (GS) Do not use with for simple property assignment
with (String.prototype.escapeHTML) div.appendChild(text);
The problem is that the escape character for entities is &. This will have the result in browsers identifying as Webkit or MSIE of:-
"&lt;".unescapeHTML() to "<"
The with statement is not needed to provide a more compact approach:
"".escapeHTML.div.appendChild("".escapeHTML.text);
Enumerable.include - Two Loops? Strict Equality, or Loose Equality?
Enumerable.include. This function returns true if the Enumerable contains the object.
include: function(object) {
// (GS) Call the indexOf method on the object.
if (Object.isFunction(this.indexOf))
if (this.indexOf(object) != -1) return true;
// (GS) If not found, search again.
var found = false;
this.each(function(value) {
// (GS) Should use strict equality, ===
if (value == object) {
found = true;
throw $break;
}
});
return found;
},
Enumerable.include first calls this.indexOf(), and if that returns false, the collection
is searched using a similar algorithm to Array.prototype.indexOf, except not using
strict equality. This tactic results in the collection being looped over twice. Function
each calls a function for each iteration. If non-strict equality is desired, then the strict equality check
that can result by calling this.indexOf should be omitted.
A plausible solution would be return this.indexOf(object), and include a
strict === check if indexOf were not a function.
getDimensions - clientWidth != offsetWidth, And Don't Expect clientWidth to be non-zero in IE
getDimensions: function(element) {
element = $(element);
var display = $(element).getStyle('display');
if (display != 'none' && display != null) // Safari bug
return {width: element.offsetWidth, height: element.offsetHeight};
// All *Width and *Height properties give 0 on elements with display none,
// so enable the element temporarily
var els = element.style;
var originalVisibility = els.visibility;
var originalPosition = els.position;
var originalDisplay = els.display;
els.visibility = 'hidden';
els.position = 'absolute';
els.display = 'block';
var originalWidth = element.clientWidth;
var originalHeight = element.clientHeight;
els.display = originalDisplay;
els.position = originalPosition;
els.visibility = originalVisibility;
// (GS) use offsetWidth/Height.
return {width: originalWidth, height: originalHeight};
},
This method has the unique quality of being one of the only methods in PrototypeJS to have a code comment.
The code tries to get the offsetWidth and offsetHeight
of the Element. If the element is not displayed,
then these properties will be 0, and in that case, the function will display the element temporarily,
then calculate its clientWidth - not the offsetWidth before immediately
hiding it. This function can be expected to
provide inconsistent results when the element's CSS display is changing.
What's worse: Unless the element has a CSS width, the clientWidth will be 0 in IE.
Calculating Offsets
Finding an Element's position is hard.
Calculating the position of an element requires adding the offsetTop/Left of
offsetParents, border offsets,
and scroll offsets of parentNodes.
A BODY with margin, position and/or border can affect the offsetTop/Left differently, depending on the browser. This is a harmful effect propagated by the CSSOM Views standard.
cumulativeOffset
PrototypeJS misses both points and just adds offsetTop/Left.
cumulativeOffset: function(element) {
var valueT = 0, valueL = 0;
do {
valueT += element.offsetTop || 0;
valueL += element.offsetLeft || 0;
element = element.offsetParent;
} while (element);
// (GS) Don't call a function that returns an
// Array with top and left properties.
// Better just return an Array or an Object.
return Element._returnOffset(valueL, valueT);
},
The seriousness of the problem is apparent when precision is crucial and off by a pixel or more would be failure, (e.g. make element to overlap another element exactly, activate a drop zone).
The bug also exists in positionedOffset and affects every function that uses
either function, including within, scrollTo, and all the functions
that call those functions, such as others found in Scriptaculous (dragdrop.js, effects.js).
Conclusion
A few core parts of PrototypeJS exhibiting some serious bugs.
There are many more issues, such as Hash,
but this entry is already getting excessively long.
PrototypeJS is designed in a non-standard way around the modification of host objects. The code that exists is complicated and not very efficient. I cannot recommend using this library for anything.
Technorati Tags: JavaScript Code Review Prototype Prototype.js
3. 'The easy solution is to "know" what you're doing : )'
- Absolutely! That is why I would just use document.getElementById, and pass the element to whatever function I needed. If a lot of related functionality were needed for one element, then a Decorator might be a better solution. The idea is that the Decorating object has an Element, and has methods/state variable like id, setActive(), et c. Can also have static members on the constructor, like activeStub, et c.
4. The code on http://github.com/kangax/protolicious/tree/master/experimental/object.for_in.js
would appear to work (I didn't test it), but it would be more compact to just have the DontEnum check inline, in the original function:
if (isDontEnumSkipped) {
for (var i=0; i<length; i++) {
if (hasOwnProperty.call(object, DontEnumProperties[i])) {
iterator.call(context || iterator, prop, object[prop]);
}
}
}
Checking a boolean flag is very fast. Performance will decrease as the scope chain lookup increases.
5. On trying to determine if an object is a NodeList, I would not use (typeof o.item). This is because typeof returns implementation-dependent results for Host objects. IOW, typeof aNodeList is not required to return "function". Internet Explorer has historically returned "object" for Host objects that seem to implement [[Call]].
For a good laugh, try running in IE:
javascript: alert(typeof document.body.childNodes.item).
I can't remember ever having to check to see if an object is a NodeList.
Function argumentNames is called only on some object that has argumentNames in the prototype chain and that is how PrototypeJS uses it. The only error would arise when attempting to call argumentNames on a non-function object.
if(typeof o.argumentNames == "function")
o.argumentNames();
Peter Belesis used this approach in Heirmenus, originally written in the previous century, before try/catch was part of the language:
Function.prototype.isFunction = true;
re: 3:
I agree that "decorator" is exactly what should have been done in the beginning. It's very possible that next major release will finally switch to such approach. "Native extensions" just bring too many complications...
re: 4:
I just remembered why "DontEnum" workaround is still not in the core. hasOwnProperty is not supported by earlier versions of webkit (I think I mentioned it to you in dhtml.js post) There was a proposed patch (and a lengthy discussion) some time ago with an interesting emulation of this method, but it relied on a non-standard (although supported by that version of webkit) __proto__ (http://dev.rubyonrails.org/ticket/9700). Since this "defective" engine version is still supported by prototype.js, it's problematic to decide whether "DontEnum" is actually worth it.
re: 5:
Yes, IE's host objects' behavior is always a good laugh : ) checking for "typeof 'function'" on the other hand yields "true" for RegExp objects (quite a disappointing behavior, since RegExp.prototype is not Function.prototype and such methods as call/apply, etc. simply fail) This is exactly why we recently "fixed" Object.isFunction to account for RegExp's
Best,
kangax
Back to your interest in finding an alternative to $super w/o function decompilation. For that, I usually just call the super's constructor explicitly:-
function Animal(){}
function Human(){
Animal.call(this);
}
It's simpler avoids having an augmented scope chain.
Of course giving a full example with adding on the prototype chain of the super to the sub, I'd use the typical prototype inheritance:-
/**
* @memberOf APE
* @description Prototype inheritance.
* @param {Function} subclass
* @param {Function} superclass
* @param {Object} mixin If present, <var>mixin</var>'s own properties are copied to receiver
* using APE.mixin(subclass.prototoype, superclass.prototype).
*/
extend : function(subclass, superclass, mixin) {
if(arguments.length === 0) return;
var f = arguments.callee, subp;
f.prototype = superclass.prototype;
subclass.prototype = subp = new f;
if(typeof mixin == "object")
APE.mixin(subp, mixin);
subp.constructor = subclass;
return subclass;
}
/**
* Shallow copy of properties; does not look up prototype chain.
* Copies all properties in s to r, using hasOwnProperty.
* @param {Object} r the receiver of properties.
* @param {Object} s the supplier of properties.
* Accounts for JScript DontEnum bug for valueOf and toString.
* @return {Object} r the receiver.
*/
mixin : function(r, s) {
var jscriptSkips = ['toString', 'valueOf'],
prop,
i = 0,
skipped;
for(prop in s) {
if(s.hasOwnProperty(prop))
r[prop] = s[prop];
}
// JScript DontEnum bug.
for( ; i < jscriptSkips.length; i++) {
skipped = jscriptSkips[i];
if(s.hasOwnProperty(skipped))
r[skipped] = s[skipped];
}
return r;
}
That's stolen right out of APE.js
I am still trying to figure out what IE is doing with item() being a callable string value:-
javascript:alert(document.body.childNodes.item.charAt(0) === "[" );
true, in IE. It's like I can't believe it; like I did something dumb and can't recognize it, but there it is: childNodes.item is "[object]" in IE:-
javascript:alert(typeof document.body.childNodes.item)
IE says: "string"
javascript:alert(Object.prototype.toString.call(document.body.childNodes.item))
IE says: "[object String]"
But the function argumentNames could safely call Function.prototype.toString without having to worry if the thisArg were a function (unless an unscrupulous user were to use:- Function.prototype.argumentNames.call({});
It's great to hear that Prototype.js developers might be considering replacing browser/element specific extensions with an approach that uses common features. This should help avoid cross-browser inefficiencies and problem.
Finally, you've got a great attitude posting up here after that review.
Garrett,
Thank you for compiling this very extensive code review. I must admit that I did get a little red in the face after reading it, but your review uncovered some bugs I was not aware of.
I would like to touch base on a few of them to let you know some of the progress we have made and to shed light on areas that are still lacking.
First the good news: The bugs listed that are fixed for the next release :)
1) Element#getDimensions
2) Element#getOffsetParent (this affects, viewportOffset, cumulativeOffset, cumulativeScrollOffset, positionedOffset)
3) Hash objects Hash#get ( only returns properties that are its own)
4) Re-establish the broken Element prototype chain. http://dev.rubyonrails.org/ticket/11004
5) There has been some work on the Element#readAttribute method but not for the issues mentioned here.
6) with statements have been removed.
The bad news: Things we need to get fixed (maybe for the next release):
1) Enumerable#include needs to return the pass or fail if it detects the indexeOf method. This avoids the double looping. The method does not perform strict comparisons on purpose (as explained in the documentation).
2) Remove the readAttribute use of _env (I will have to ask some of the core devs why this was even in there, it seems completely unnecessary)
3) Fix the Safari branch of the $A method to not use the Function toString to detect the NodeList
I would like to mention some things I took issue with in your review.
1) The use of $super does throw off some compressors that ???minify??? the code.
Ref: http://ajax.sys-con.com/read/464826.htm. But, if you use YUI Compressor v2.2 or higher the $super variable is skipped so you don't have to worry about it. I have also made trivial modifications to Dean Edward???s Packer 3 to skip the $super variable as well.
2) Appending &_= in Ajax post calls for Safari stops a bug in Safari 2.x that appends \000 to the end of the postBody and causes issues with Apache servers.
3) The use of __proto__ is done only after checking to see if the browser supports it via Prototype.BrowserFeatures.SpecificElementExtensions. The findDOMClass function is only used if it is supported.
4) document.getElementsByClassName is only added if the browser doesn???t have it. The addition is deprecated because the native getElementsByClassName doesn???t return an extended array, it returns a nodeList. We recommend using the $$() utility method instead.
5) I have tested the reworked cumulativeOffset methods with BODY margins and borders and various other scenarios and have not seen an issue. They seem to be pixel perfectly accurate. If you can produce some failing tests for 1.6.0.2 I would like to see how they hold up against our current edge copy. The more testing the better. The position methods are the bane of my existence!
On a side note:
Prototype???s position/dimension methods do not work in quirksmode. Pages must set proper doctypes. Prototype needs to mention this clearly in the documentation.
6) I don???t understand your confusion when it comes to the $() method. Its function/purpose is clearly stated in the documentation. It is not simply a shortcut for getElementById. It is used to extend the DOM elements (for browsers like IE) and allows end users to pass a DOM Element or ID to any of the Element methods. After an element is extended it is set with a _extendedByPrototype property so it doesn???t get extended again.
Using a framework is about convenience at the cost of performance. This trade off is made with any client side framework. Use the methods and helpers responsibly. I wouldn???t recommend using $$(..) with some complex selector over and over. Results should be stored in variables and use wisely to avoid performance headaches.
7) I think Prototype will always bother some people. Some, like the overly abusive few at comp.lang.javascript, may never like Prototype and that is fine. Prototype extends native prototypes, that???s its shtick. Prototype just needs to do so more responsibly and I think we are moving in that direction.
In the future we plan to remove many of the browser sniffs in favor of capability testing. We plan to optimize the code to make less calls to the $() function (though some cannot and should not be avoided). There are several other areas we plan to tweak and optimize to increase the performance of Prototype.
We are aware that some environments don???t play nice with Function#argumentNames and are looking into ways around it for our Class emulation.
As kangax mentioned Decorator objects for things like elements and events have been on our minds for some time now. It is the only way to go to avoid sooo many headaches :D. There are also ongoing discussions on fixes for hasOwnProperty and the IE DontEnum bug.
Well, I think that???s about it for now. Please feel free to email me if you have any other concerns or additions to this review. As always, you are more than welcome to submit any bugs you find along with patches and unit tests: http://www.prototypejs.org/contribute
Thanks again.
- JDD
Hmmm my single quotes turned into ???.
I also forgot to mention, though I think kangax beat me to it, that the String#escapeHTML/unescapeHTML issue is resolved in the next release as well :)
This was not meant to embarrass anyone or make any enemies.
Providing a public code review gets people looking at the source code. It is a good thing and a good way to learn.
I realize that this has potential to set me apart from other developers in ways that are good and ways that are not.
> First the good news: The bugs listed that are fixed for the next release :)
> 2) Element#getOffsetParent (this affects, viewportOffset, cumulativeOffset, cumulativeScrollOffset, positionedOffset)
I didn't actually review getOffsetParent. Taking a look...
getOffsetParent: function(element) {
if (element.offsetParent) return $(element.offsetParent);
if (element == document.body) return $(element);
while ((element = element.parentNode) && element != document.body)
if (Element.getStyle(element, 'position') != 'static')
return $(element);
return $(document.body);
}
1) Calls dollar on the element, which methodizes the element (if it hasn't been methodized already).
2) won't necessarily return the offsetParent consistently in all browsers [1]
3) Will error out with document or document.documentElement.
Recent versions of Opera still include border-width of the parentNode in offsetTop/Left.
[1] Offset* is a complete mess in the browsers.
offsetParent is different in IE7 vs Opera, Mozilla and Webkit. In IE7, BODY does not have to be offsetParent. This seems to be a reasonable expectation; BODY doesn't need position. BODY can have position, margin, border.
In IE, a positioned BODY will be an offsetParent for relative or absolute positioned elements. But if the BODY does not have position, then it won't be an offsetParent, unless the node in question is a child of BODY and has position: static, then BODY will be the offsetParent.
However, in the other browsers, and in CSSOM, BODY is always offsetParent.
IE will treat a non-positioned container as an offsetParent for descendant D iff D has position: static.
If getOffsetParent is called repeatedly, as in a loop, to calculate the offsetParent, that would be very inefficient, especially since getOffsetParent calls dollar (and dollar makes up to 151 other function calls). I can see that 1.6.0.2 does something worse: it calls a browser-branched, wrap'd getOffsetParent repeatedly in a loop.
else if (Prototype.Browser.IE) {
// IE doesn't report offsets correctly for static elements, so we change them
// to "relative" to get the values, then change them back.
Element.Methods.getOffsetParent = Element.Methods.getOffsetParent.wrap(
function(proceed, element) {
element = $(element);
var position = element.getStyle('position');
if (position !== 'static') return proceed(element);
element.setStyle({ position: 'relative' });
var value = proceed(element);
element.setStyle({ position: position });
return value;
}
);
You're saying this has been fixed, and I'm now reviewing the current release (1.6.0.2). I can see the bug there. Changing an element's position to relative will make the offsetParent different. For example:-
<body><div id='o'><div id='i'><div></div></body>
div#i's offsetParent would be div#o, if both were static. However, if div#i were relative then the offsetParent would be BODY. But then if div#o had position relative, div#o's offsetParent would be HTML.
For the next release, it would be more efficient to instead just use offsetParent directly, where needed. Perform any necessary feature tests to take browser bugs into consideration. getBoundingClientRect, where available, is much faster.
I can also share:
http://dhtmlkitchen.com/ape/example/dom/getOffsetCoords.html
continued...
(continued)
> The bad news: Things we need to get fixed (maybe for the next release):
> 1) Enumerable#include needs to return the pass or fail if it detects the indexeOf method. This avoids the double looping. The method does not perform strict comparisons on purpose (as explained in the documentation).
Not good. Array.prototype.indexOf uses strict equality. If the Enumerable is an Array, then it will have different behavior. In many cases, Enumerable will be an Array. So it seems better to not attempt to call the indexOf method at all then, as doing so would do strict equality.
> I would like to mention some things I took issue with in your review.
Yes, I know that YUICompressor (which I use) has created a special patch for $super. This was a decision I do not agree with.
IMO variable name should be changeable by a programmer or compiler; that's inherent in the term 'variable' A variable named $super should also be changeable. I acknowlege that YUI considered it a bug and I disagree with that decision.
> 2) Appending &_= in Ajax post calls for Safari stops a bug in Safari 2.x that appends \000 to the end of the postBody and causes issues with Apache servers.
So the null character is appended? What is it about adding "_=" that prevents "\000" from being added to the POST body?
> 3) The use of __proto__ is done only after checking to see if the browser supports it via Prototype.BrowserFeatures.SpecificElementExtensions. The findDOMClass function is only used if it is supported.
What does "supported" mean?
SpecificElementExtensions:
document.createElement('div').__proto__ &&
document.createElement('div').__proto__ !==
document.createElement('form').__proto__
}
SpecificElementExtensions could be true when document.createElement('form').__proto__ is undefined and document.createElement('div').__proto__ is Object.prototype.
This result would be perfecty standards-compliant.
> 5) I have tested the reworked cumulativeOffset methods with BODY margins and borders and various other scenarios and have not seen an issue. They seem to be pixel perfectly accurate. If you can produce some failing tests for 1.6.0.2 I would like to see how they hold up against our current edge copy. The more testing the better. The position methods are the bane of my existence!
I am aware of many issues with calculating offsets. Just adding a border to a parentNode throws cumulativeOffsets wildly off.
Position functions are a pain, aren't they?
<div style='border:100px solid;' id='a'><div style='position:relative;border: 1px solid red'>a</div></div>
cumulativeOffsets Returns [8,8]
These problems were discussed the w3c css-d list this year.
I wrote a test suite for this:
http://dhtmlkitchen.com/ape/test/tests/dom/position-f-test.html
Replace dom.getOffsetCoords with Element.Methods.cumulativeOffset and set 'container' to document.
> 6) I don't understand your confusion when it comes to the $() method. Its function/purpose is clearly stated in the documentation. It is not simply a shortcut for getElementById. It is used to extend the DOM elements (for browsers like IE) and allows end users to pass a DOM Element or ID to any of the Element methods. After an element is extended it is set with a _extendedByPrototype property so it doesn't get extended again.
I don't understand why you're labelling my review as 'confusion'. What part of the review was unclear?
> Using a framework is about convenience at the cost of performance.
There may be some truth to that, however, I do not think that sets a very high standard for designing a framework.
For me, a framework is about organizing code into testable, reusable units. On most script-heavy projects, I'll usually need DOM functions like a getElementsWithClass and getEventTarget. I'll probably need an Event Registry. I may need any number of other things. I've used a number of these things over the years and I find myself in situations where I need to use them again. So the problem is about organizing the code into reusable components. I'm not copy-pasting code around. I need these things to be simple, unit-testable, and smoking fast.
I don't "hate" any library code, but I do know what to look out for when I'm looking for a project.
It appears that you are committed to improving PrototypeJS.
I don't want to distract from the review. I am glad you did this review. I said "confusion" because you said "$ does not have a clearly defined meaning as to what the function actually does.". This was not a stab at your code review. I said I was "red in the face" not out of embarrassment, I was trying to express that the review made some good points and that some where hard to swallow (but nonetheless valid) :). I never mentioned the word "hate" and I am glad you don't hate any of them.
RE 2: Yes the &_= fixes the issue because it is appended to the "_" variable. At least thats what I gather from digging through the bug tickets.
RE 3: Thank you again for pointing this out. When I have some time I will look into that some more.
RE 5: Thank you for providing test cases. I will see how they stack up to the reworked methods.
RE 6B: I was making a statement about frameworks in general (at least the ones I have used). All of the helpers/bells and whistles usually come at the cost of some performance. This was not meant as a statement on Prototype's core design standard (I don't want to put words in there mouth). As I mentioned performance improvements are in the works.
Once again thanks for this great code review.
-JDD
jd> RE 2: Yes the &_= fixes the issue because it is appended to the "_" variable. At least thats what I gather from digging through the bug tickets.
It seems strange that "_" makes Webkit not append null character. Browsers often have strange bugs. What I found works is to have a test case for triggering that the strange bug, so that the code fails. In this case, the test would assert that a post-body that does not end in "_" is exactly as expected (no, "\000", et c). If I can make the test case fail, then it's patchable. The test case is linked in the bug report and the patch is checked in with comments containing test case/bug report. It does seem unlikely, however, that the post body cannot be sent without adding "_".
When I said: "$ does not have a clearly defined meaning as to what the function actually does," I meant that. Developers can understand what dollar does by looking at the source code or documentation, not the name.
Methods should be named by what they do. Method getElementById, createElement, for example.
I made a similar point with Element.methods.visible. It's a bad name. Based on what it does, it could be renamed isDisplayed, but then it would not be possible to be certain, because it checks the - element.style.display - not the computed/currentStyle.
It's quite possible that the stylesheet has display: none for that element; checking - element.style.display - won't reflect the value in the stylesheet. But that's another point.
Hello Garrett,
RE $ name: I am all for verbose names (that's one reason I like Prototype over others), but I see the advantage of using a short utility method. For your personal use can always veryDescriptiveFunctionName = $;
The "visible" method name has been in debate for a while. I think, for now, they choose to keep it simple, but you are right it is misleading. I will see about patching it to use "getStyle" which checks cimputed/currentStyle.
RE "_" variable: The variable doesn't have to be named "_" that is just one they choose, and it should propably be changed (there has been 1 issue with some Oracle server software and the use of the "_"). It seems to stop issues because the \000 is being assigned to a "throw away" variable and not to a variable that is used for something else.
RE offsetParent: Yes the bugs you mentioned are fixed for the next release. However calling the $ method is done on purpose so you can pass an ID or element to the method. The method, as per the documentation, will try to return the offsetParent or return BODY (not null). The patched version will not return HTML (it checks for that). The wrapped addition in IE has been reduced, it no longer modifies the elements position (this was causing other odd display issues). The wrap may even be removed. As of git edge the wrap only serves to check if the element is attached to the DOM so IE doesn't error out.
RE offsetTop, offsetLeft: In your review you state issues with cumulativeOffset and other methods. I think its just because you handle them differently than Prototype. Here is an example of some pixel perfect work: http://www.nickstakenburg.com/tests/cumulativeoffset/prototip1.6.0.2/
Prototype calculates values based on the outside edge of the element border. This is also done when it calculates dimensions (it uses offsetWidth, which includes the element border).
I haven't seen the Opera parentNode borderWidth/offset(Left|Top) bug. Prototype supports Opera >= 9.25 (I think 1.6.0.2 still has some Opera issues, we almost all of the known issues resolved in the edge. I will produce a unit test for this.
- JDD
I'll echo the thanks from kangax and JDD. Rather than address each critique separately, I'll simply say that:
* Some of these are legitimate bugs that have not yet been fixed.
* Some are legitimate bugs that will be fixed in the next release (1.6.0.3) or perhaps the one after that (1.6.1).
* Many of the design decisions you criticize are the result of maintaining backward-compatibility with earlier versions of Prototype. That doesn't mean they aren't valid, but it does mean they can't be revisited until Prototype 2 (which won't be backward-compatible with Prototype 1).
* I'm not unsympathetic to performance gripes, but I find them far more persuasive when expressed in milliseconds rather than function calls or additions to the call stack.
Anyway, I appreciate the time you've taken.
RE Element#visible: CSS "display" is not inheritable
http://www.w3.org/TR/REC-CSS2/visuren.html#display-prop
and Prototype ignores display being set from css to avoid issues when it sets the display = '';
@JDD
I've mailed you a simple example demonstrating the problems with cumulativeOffset. I should probably post them up here, though.
Regarding display being not inheritable - I think you misread or misunderstood the CSS2 spec. Explanation below.
The problem with ignoring display='' is that the display of the element is not determined. When setting display = ''; the browser must apply the next rule up in the cascade (not the style attribute). This would come in a stylesheet and could be any of:
#el { display: none; }
#el { display: inline; }
#el { display: block; }
Now, if Prototype JS were to set display = '', and the stylesheet had #el { display: none; }, then the element would not be displayed, and Element.Methods.visible("el") would return true.
visible: function(element) {
return $(element).style.display != 'none';
},
http://www.w3.org/TR/REC-CSS2/visuren.html#display-prop
Regarding renaming dollar, I do not agree that - veryDescriptiveFunctionName - is a good name for what dollar does. I have a great story on the naming of ambiguous "kitchen sink" objects. It's really funny, looking back now, but I was seriously frustrated at the time. Might be a good idea for a new entry. It really is a funny story. LOL. You'll see.
=====================================================
Reading the CSS 2 / CSS 2.1 specs.
=====================================================
9.2.5 The 'display' property
'display'
Value: inline | block | list-item | run-in |
compact | marker | table | inline-table |
table-row-group | table-header-group |
table-footer-group | table-row | table-column-group |
table-column | table-cell | table-caption | none |
inherit
Initial: inline
Applies to: all elements
Inherited: no
Percentages: N/A
Media: all
(line breaks mine)
Granted, 'inherit' won't work right even in IE7, but it's perfectly valid.
Where it says: "Inherited: no", that means, by default, the value is not inherited.
http://www.w3.org/TR/CSS21/cascade.html#value-def-inherit
6.2.1 The 'inherit' value
Each property may also have a specified value of 'inherit', which means that, for a given element, the property takes the same computed value as the property for the element's parent. The 'inherit' value can be used to strengthen inherited values, and it can also be used on properties that are not normally inherited.
If the 'inherit' value is set on the root element, the property is assigned its initial value.
Some values are inherited, for example, font-size. CSS2.1 spec:
http://www.w3.org/TR/CSS21/fonts.html#font-size-props
15.7 Font size: the 'font-size' property
'font-size'
Value: <absolute-size> | <relative-size> |
<length> | <percentage> | inherit
Initial: medium
Applies to: all elements
Inherited: yes
Percentages: refer to parent element's font size
Media: visual
Computed value: absolute length
Ahh thanks for clearing the "display" inherited issue up. I will look into patching it for the value "inherit".
In reguards to setting element.style.display = "", Prototype does that by design to allow a css specified value to be inherited, and they have warned about using display:none; in the css.
http://www.prototypejs.org/api/element/show
RE Offsets: The example you sent me is fine in Prototype. It reports 20, 20 which is accurate to how Prototype calculates offsets which is to the outer edge of the border on elements (I mentioned this earlier). The rest of Prototype.js methods work assuming the offset is from the outer edge of the element border and maintain pixel perfect calculations. Developers have used these methods to produce pixel perfect tooltips, menus and many other things.
RE Offset bugs: I was digging through your position extension to APE and I noticed you have
"// XXX Opera <= 9.2 - parent border widths are included in offsetTop.
IS_PARENT_BODY_BORDER_INCLUDED_IN_OFFSET,
// XXX Opera <= 9.2 - body offsetTop is inherited to children's offsetTop
Is this for version 9.2 and less or 9.25.
I also noticed you had fixed for some offset bugs in mozilla (I need to test to see if those effect Prototype's supported versions).
Thanx,
JDD
Ahhh I ran your simple offset example in Firefox and quickly saw the Mozilla bug. Nice :).
JDD> In regards to setting element.style.display = "",
> Prototype does that by design to allow a css
> specified value to be inherited, and they have warned
> about using display:none; in the css.
> http://www.prototypejs.org/api/element/show
The documetation states:
============================================
Element.show cannot display elements hidden via CSS stylesheets. Note that this is not a Prototype limitation but a consequence of how the CSS display property works.
============================================
Actually this *is* a limitation of PrototypeJS.
The name "visible" doesn't mean "user can see it." It doesn't mean CSS visibility. It doesn't even reflect the CSS display property. The documentation page could probably be removed if the method were simply named - isDisplayed( element );
Since there is a getStyle method, it would be best to use that. However, on looking at getStyle, I see that that function is branched based on browser detection (based on something unrelated to the problem at hand).I see:
if (Prototype.Browser.IE) {
Followed by a redefining of getStyle.
It would be a better idea to have a conditional for determining approach to use: getComputedStyle or currentStyle.
if(COMPUTED_STYLE_SUPPORT) {
getStyle = function(){ ... }
}
- and provide feature tests for known issues (Plethora of Opera bugs), which can be checked in conditionals.
It is better because when IE has computedStyle support, it will get that other branch of code.
> RE Offsets: The example you sent me is fine in
> Prototype. It reports 20, 20 which is accurate to how
> Prototype calculates offsets which is to the outer
> edge of the border on elements (I mentioned this
> earlier). The rest of Prototype.js methods work
> assuming the offset is from the outer edge of the
> element border and maintain pixel perfect
> calculations. Developers have used these methods to
> produce pixel perfect tooltips, menus and many other things.
Those developers must have gotten very lucky, as that function would fail in many cases.
The information on the css-d list includes a variation of the function in PrototypeJS (similar approach) and explanations for why it doesn't work.
http://lists.w3.org/Archives/Public/www-style/2008Mar/0265.html
http://lists.w3.org/Archives/Public/www-style/2008Mar/0147.html
http://lists.w3.org/Archives/Public/www-style/2008Apr/0440.html
http://www.w3.org/Search/Mail/Public/advanced_search
?keywords=cssom+offsetTop&hdr-1-name=subject&hdr-1-query=
&hdr-2-name=from&hdr-2-query=&hdr-3-name=message-id
&hdr-3-query=&period_month=&period_year=2008
&index-grp=Public__FULL&index-type=t
&type-index=www-style&resultsperpage=20&sortby=date
Anne van Kesteren seems to be content in continuing with the approach of standardizing offset* in a way that is inconsistent with what MSIE actually does.
Unfortunately, his tact is not making scripting web pages any easier.
Take a look at some other libraries' code and you'll see conditions for scroll offsets, et c. YAHOO.Dom.getXY, jQuery offsets, Matt Kruse's object position, et c. The library that has the most tests for it's position-finding-function, though, is APE.
JDD> RE Offset bugs: I was digging through your position extension
> to APE and I noticed you have
"// XXX Opera <= 9.2 - parent border widths are included in offsetTop.
IS_PARENT_BODY_BORDER_INCLUDED_IN_OFFSET,
// XXX Opera <= 9.2 - body offsetTop is inherited to children's offsetTop
JDD> Is this for version 9.2 and less or 9.25.
> I also noticed you had fixed for some offset bugs in
> mozilla (I need to test to see if those effect
> Prototype's supported versions).
That variable, IS_PARENT_BODY_BORDER_INCLUDED_IN_OFFSET, will be true for any environment where the behavior where setting: body.style.border = "1px solid" - affects the offsetTop of its first child. The part of that which is omitted is that BODY has position: relative. BODY having position: relative seems to makes a difference on whether or not the borderTop of the body is added to its child's offsetTop.
The comment above the variable:-
// XXX Opera <= 9.2 - parent border widths are included in offsetTop.is true, but does not tell the whole story of that variable (as I just explained). I try to make a comment for why I put the code in. In this case, I discovered the problem first in Opera. I later realized that it affects other browsers in other cases.
This variable and other load-time constant variables get their values assigned in an anonymous function that appears below.
Garett,
> SpecificElementExtensions could be true when document.createElement('form').__proto__ is undefined and document.createElement('div').__proto__ is Object.prototype.
Would you clarify in which environments this could happen? I have never seen such "combination".
> Since there is a getStyle method, it would be best to use that. However, on looking at getStyle, I see that that function is branched based on browser detection (based on something unrelated to the problem at hand)
I agree that many of Element.Methods.* branches could be refactored into branch-less versions. Most of these changes will most likely be incorporated into an upcoming 1.6.1 (currently we are somewhat stuck with 1.6.0.3 bugfix-only release)
The problem with Element#visible being so "naive" lies partially in performance concerns (though it might indeed seem strange to pay attention to #visible yet hog performance in IE with #wrap and #methodize). Checking "style.display" seems to be quite sufficient and bring no overhead.
I remember proposals to make a #visible respect ancestors' styles which were postponed for the very same reasons - frequently iterating potentially deep DOM hierarchy could quickly bring app to its knees.
Having said that, I am always in search of a sniff-less way to achieve certain things. Unfortunately this is not always possible.
@Kangax
There was a consideration in a Webkit bug where document.constructor.__proto__:
https://bugs.webkit.org/show_bug.cgi?id=11315#c5
Using that approach Atlas guys used a non-standard approach that led to that result.
Having two functions can sometimes avoid the small runtime performance hit of having all of the logic in one long function. However, a branch of code that says:- if(Prototype.Browser.IE) - and then following that up with code that is for a specific type of getStyle that deals with currentStyle - could be better replaced by more specific feature tests.
@Andrew
If you want to know how much slower it is to call 1600 functions than calling 2, it is not hard to test. I set this up with 17 elements. I tested in IE7, which is faster than IE6. I added 17 form elements, so more function calls.
Create a simple form with 17 elements,
save timestamp
call $(f.id).descendants()
return new Date - timestamp;
javascript:void(function(){var f = document.createElement('form'); for(var i = 0; i < 17; i++) f.appendChild(document.createElement('input')); document.body.appendChild(f);document.title = f.id='form1' ; var d = new Date; var r = $(f.id).descendants(); alert(new Date-d); }())
About 50ms in IE7.
Compare that to getting the elements of the Form:-
javascript:void(function(){var f = document.createElement('form'); for(var i = 0; i < 17; i++) f.appendChild(document.createElement('input')); document.body.appendChild(f);document.title = f.id='form1' ; var d = new Date; var r = document.getElementById(f.id).elements; alert(new Date-d); }())
0ms in IE7.
Try with 100 elements:
javascript:void(function(){var f = document.createElement('form'); for(var i = 0; i < 100; i++) f.appendChild(document.createElement('input')); document.body.appendChild(f);document.title = f.id='form1' ; var d = new Date; var r = document.getElementById(f.id).elements; alert(new Date-d); }())
0ms.
Try with 1000 elements:-
javascript:void(function(){var f = document.createElement('form'); for(var i = 0; i < 100; i++) f.appendChild(document.createElement('input')); document.body.appendChild(f);document.title = f.id='form1' ; var d = new Date; var r = document.getElementById(f.id).elements; alert(new Date-d); }())
0ms.
So we can see a significant and measurable difference using PrototypeJS over raw DOM performance. This is no surprise.
==================================================
Element.Methods.descendants() takes 50ms to execute under normal circumstances.
javascript:(function(){ var d = new Date; var r = $('excerpt').descendants()[3].cumulativeOffset(); alert(new Date-d); })()
31ms in IE7, run against PrototypeJS website documentation.
For what it is doing, it is taking a measurably long time. Given context in an application (sorting rows, dragging sortable rows with multiple drop targets), these fractions of a second can add up to something noticeable to the user.
Doing something less inefficiently would make sense if the inefficient way were simpler. But what PrototypeJS does with $ is not simple.


AnimTree
Garrett,
Thanks for an exhaustive overview.
Many points you bring are valid, but some information is outdated. I understand that review is based on the latest public version (1.6.0.2), but things changed quite drastically in a latest trunk version. I'll clarify on a few things and give few general comments as well:
1) Extending elements' prototypes is definitely a risky business. Prototype.js chose to go non-standard way by limiting its supported browser scope to a set of those that implement desired behavior somewhat reliably (as backed up with time/tests).
2) I find function decompilation in prototype's Function#argumentsNames less critical than the one from jQuery's $.isFunction. $.isFunction is a more general-purpose method (which is intended to work reliably with any passed object/primitive). Function#argumentNames on the other hand is defined as a method of Function.prototype and is intended to be called from within Function objects. I think there's a less chance of failure in a latter one (though the chance is still there). Function.prototype.toString.call(this) could, indeed, make for a more reliable replacement, but that brings more complications. Since Function.prototype.toString throws error when "this" value is not a Function object, we'd need to wrap statement in a "try/catch". "Try/catch" is expensive, especially in such a low-level helper. Other than that, I would gladly hear any alternative to implementing $super functionality without using function decompilation.
3) You are right about $ being expensive in browsers without exposed elements' prototypes. The easy solution is to "know" what you're doing : ). Extending element once and storing a reference to it for later use is a trivial thing to do and solves a problem nicely. The cases when $ is called hundreds of times in some of Element.Methods (e.g. when performing batching) could still be a performance hog.
4) DontEnum bug is a known issue. It's actually somewhat worked around for in Class.addMethods, though inefficiently and only partially :/ Declaring Object.extend with DontEnum bug in mind brings, once again, performance concerns. Changing such frequently called method could cripple overall performance. I've been playing with this some time ago (http://github.com/kangax/protolicious/tree/master/experimental/object.for_in.js) and am still to test how it affects library's performance.
5) Checking NodeList's with '[object NodeList]' is indeed dirty. How would you go about determining a NodeList? The only solution that comes to mind is checking for "length" and "item" members:
function isNodeList(o) {
return o && typeof o.length == 'number' && typeof o.item == 'function';
}
6) Underscore in Ajax.Request was, afaik, for older versions of Safari which had problems with empty request bodies (I will need to find out more about this).
7) String#unescapeHTML was refactored some time ago (http://github.com/sstephenson/prototype/commit/57a901febacfd831bdb6c9b5b95753f4d256a65b)
8) with statement was removed (http://github.com/sstephenson/prototype/commit/6bae548e0a408a21c76ac6039f83df086a91767a)
I'll try to comment on other sections when I have more time.
Best,
kangax