Skip to content

Commit

Permalink
fixes hasMethod for native IE dom
Browse files Browse the repository at this point in the history
  • Loading branch information
@msweeney
msweeney committed May 14, 2010
1 parent 8f4ec98 commit a3b173f
Show file tree
Hide file tree
Showing 8 changed files with 19 additions and 16 deletions.
4 changes: 3 additions & 1 deletion build/node/node-base-debug.js
View file
Expand Up @@ -1076,7 +1076,9 @@ Y.mix(Y_Node.prototype, {

hasMethod: function(method) {
var node = this._node;
return !!(node && method in node && node[method].apply);
return !!(node && method in node &&
(typeof node[method] === 'function' ||
String(node[method]).indexOf('function') === 1)); // IE reports as object, prepends space

This comment has been minimized.

Sign in to view

Copy link
@kangax

kangax May 15, 2010

String-converting host objects is known to blow up in IE, so this is not a very good idea. Function decompilation is another thing that's best avoided.

This comment has been minimized.

Sign in to view

Copy link
@msweeney

msweeney May 15, 2010

Author Contributor

Its not pretty, but it works in IE6 and isn't blowing up any test cases. Let me know if you have a failure case. Better yet, suggest a cleaner hasMethod test.

This undocumented method will likely be marked deprecated in a future release.

This comment has been minimized.

Sign in to view

Copy link
@GarrettS

GarrettS May 17, 2010

Based on the change, you seem to have realized that host objects aren't functions and don't have an apply method. Most realized that 5 years ago or more. Where have you been?

Converting host property object to string and parsing the string to see what it looks like doesn't really provide indication for the capability of that property.

when node is a form:
hasMethod("item")

And yes, string conversion of host object is known to blow up in IE, even [[Get]] can cause unknown error. The in operator may provide misleading results with catchall. For most consistent results, use typeof.

This comment has been minimized.

Sign in to view

Copy link
@msweeney

msweeney May 17, 2010

Author Contributor

Thanks for the feedback.

If you're assertion is true ("For most consistent results, use typeof") then you have nothing to worry about as IE should never reach the OR condition that converts to string.

Unfortunately, that is consistently wrong, as typeof on a host object in IE is "object".

As mentioned, this undocumented method will be likely be deprecated. However, I would love to be enlightened by those in the "know", so please submit a use-case that makes YUI::Node:hasMethod blow up.

This comment has been minimized.

Sign in to view

Copy link
@GarrettS

GarrettS May 17, 2010

The typeof operator takes an operand and results in string; it is not consistently wrong. typeof on a host object in IE is not consistently "object". Try the example I provided in my last message. Again:

when node is a form:
hasMethod("item")

A program expecting MSIE to consistently result "object" for methods will fail if the type is something other than object. The typeof operator returns implementation dependent string for host object in ES3.

But carry on as you like.

This comment has been minimized.

Sign in to view

Copy link
@msweeney

msweeney May 17, 2010

Author Contributor

Thanks Garrett.

The example you provide is exactly why this method is currently undocumented, and why it will likely be deprecated. Short of try/catching a call to the method, I haven't found a reliable way to determine whether the object is a function or not in all cases.

Unfortunately, in my testing, no native dom node reports anything as typeof "function", so all checks fall to the String coercion. The only assumptions made here are that calling the String constructor on native dom properties/methods is safe. I've heard anecdotally that this may "blow up" in some cases, however none of these case have surfaced with this implementation. A testcase to the contrary would be helpful.

Rather than commenting on checkins, a better way to contribute would be to help solve this problem. While I'm not convinced Node::hasMethod belongs in the library, Y.Lang.isFunction could benefit with some help here.

This comment has been minimized.

Sign in to view

Copy link
@GarrettS

GarrettS May 17, 2010

Anything where typeof results in "unknown".
Here:

typeof document.createElement("xml").onreadystatechange; // "unknown"
document.createElement("xml").onreadystatechange; // boom!

The ES3 specification states that for host object, the result of typeof is implementation-dependent. Notable changes are seen between Opera versions and IE. Moreover, string conversion of host objects, as stated, is not required by ES3 and that can blow up, too.

Using the value of string converting a host object to determine program decisions is ridiculous. No standard requires string conversion of host object. The result is nonstandard, proprietary, undocumented. You should not be doing that.

For example, in IE, I can see the string value of alert, which is a host method, has "function" in it.

javascript: alert( String( alert ) )

My suggestion for helping out with isFunction is to refactor anything that uses it to not do that, to deprecate isFunction, and to remove it. It is broken by design.

/**
     * Determines whether or not the provided object is a function.
     * Note: Internet Explorer thinks certain functions are objects:
     */

The comment further indicates that the author believes that getAttribute is a function in IE. There is no reason for believing that to be true and in fact it is false.

Y.Lang.isFunction does not and can not, in the general sense, fulfill what is promised in its preceding code comment.

It fails due to the reasons that been discussed over the years on c.l.js and touched upon here. It boils down to typeof having different results with host object and the [[Class]] property being implementation-dependent for host object. It fails for other reasons where a built-in may have also a [[Call]] property, or be somehow callable without having a [[Call]] property (as spidermonkey RegExp).

The function is a failed attempt because the author demonstrates misleading information about the a callable object being a function. An object that is callable is not necessarily a function. Failing to make that distinction in a code comment, and, moreover, providing a false statement with getAttribute, shows that the author does not know that the function is not clear on what can be expected of it.

As such, use of that function should be avoided.

More details of "isFunction" functions, typeof, and host methods have been discussed over the years on c.l.js.

This comment has been minimized.

Sign in to view

Copy link
@msweeney

msweeney May 17, 2010

Author Contributor

The "boom" in your test is a "not supported" JS error due to node[method] access on a field of type "unknown", not String conversion. Treating "unknown" as "undefined" resolves this issue and puts the hasMethod result inline with other vendor implementations for the xmlNode.hasMethod('onreadystatechange') test case.

As I mentioned, while not sold on the Node::hasMethod, we are interested in beefing up support for Lang::isFunction.

This discussion would be more appropriate on YUI Forums or within our ticket system.

This comment has been minimized.

Sign in to view

Copy link
@GarrettS

GarrettS May 17, 2010

You wrote: "Treating "unknown" as "undefined" resolves this issue and puts the hasMethod result inline with other vendor implementations "

That is completely wrong and false. The unknown type represents ActiveX object.

Didn't you just publish a chapter in book? And you're on the YUI team....

Unbelievable.

This comment has been minimized.

Sign in to view

Copy link
@msweeney

msweeney May 17, 2010

Author Contributor

Thanks Garrett. Calling Node::hasMethod against your xml test case is now "false", as expected.

Please file a bug for any failing case you have with this implementation. Calling it "wrong" doesn't get any closer to a resolution.

This comment has been minimized.

Sign in to view

Copy link
@GarrettS

GarrettS May 17, 2010

Quote: "Calling Node::hasMethod against your xml test case is now "false", as expected."

No, and for example, a true result for:-

typeof hostObject.property === "unknown"

- is not an indication that the property is undefined; quite the contrary. In IE, some host objects are implemented as ActiveX objects. When an ActiveX object is supplied to typeof operator, the result is "unknown". And so the example typeof hostObject.property === "unknown" would be indicative that the property is an ActiveX object.

This matches behavior documented in MS-ES3, for SafeArray. MSDN links are broken, but a copy of MS-ES3 may be found on the FAQ:
http://jibbering.com/faq/#resources
}
}, true);

Expand Down
2 changes: 1 addition & 1 deletion build/node/node-base-min.js
View file

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion build/node/node-base.js
View file
Expand Up @@ -1068,7 +1068,9 @@ Y.mix(Y_Node.prototype, {

hasMethod: function(method) {
var node = this._node;
return !!(node && method in node && node[method].apply);
return !!(node && method in node &&
(typeof node[method] === 'function' ||
String(node[method]).indexOf('function') === 1)); // IE reports as object, prepends space
}
}, true);

Expand Down
4 changes: 3 additions & 1 deletion build/node/node-debug.js
View file
Expand Up @@ -1076,7 +1076,9 @@ Y.mix(Y_Node.prototype, {

hasMethod: function(method) {
var node = this._node;
return !!(node && method in node && node[method].apply);
return !!(node && method in node &&
(typeof node[method] === 'function' ||
String(node[method]).indexOf('function') === 1)); // IE reports as object, prepends space
}
}, true);

Expand Down
4 changes: 2 additions & 2 deletions build/node/node-min.js
View file

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion build/node/node.js
View file
Expand Up @@ -1068,7 +1068,9 @@ Y.mix(Y_Node.prototype, {

hasMethod: function(method) {
var node = this._node;
return !!(node && method in node && node[method].apply);
return !!(node && method in node &&
(typeof node[method] === 'function' ||
String(node[method]).indexOf('function') === 1)); // IE reports as object, prepends space
}
}, true);

Expand Down
4 changes: 3 additions & 1 deletion src/node/js/node.js
View file
Expand Up @@ -1074,7 +1074,9 @@ Y.mix(Y_Node.prototype, {

hasMethod: function(method) {
var node = this._node;
return !!(node && method in node && node[method].apply);
return !!(node && method in node &&
(typeof node[method] === 'function' ||
String(node[method]).indexOf('function') === 1)); // IE reports as object, prepends space
}
}, true);

Expand Down
9 changes: 1 addition & 8 deletions src/node/tests/node.html
View file
Expand Up @@ -9,10 +9,6 @@
font:13px/1.22 arial;
}

.yui-log {
height:1000px;
}

#doc {
min-height:200px;
width:1000px;
Expand Down Expand Up @@ -171,7 +167,7 @@ <h2>test</h2>
ArrayAssert = Y.ArrayAssert,
suite = new Y.Test.Suite("yuisuite");

new Y.Console({node: '#yui-log', height: '95%' }).render('#yui-log');
new Y.Console({node: '#yui-log', height: '1000px' }).render('#yui-log');

var byId = function(id) {
return document.getElementById(id);
Expand Down Expand Up @@ -1191,11 +1187,8 @@ <h2>test</h2>

Y.Test.Runner.add(suite);

// allow window scroll event to happen post-onload
Y.on('load', function() {
// setTimeout(function() {
Y.Test.Runner.run();
// }, 1);
}, window);
});
</script>
Expand Down

0 comments on commit a3b173f

Please sign in to comment.