Closed Bug 390411 Opened 17 years ago Closed 8 years ago

Can't override getElementsByClassName by setting it on HTMLElement.prototype

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: RyanVM, Unassigned)

References

(Depends on 1 open bug, )

Details

Attachments

(3 files)

Attached file ajaxform.js
When trying to submit a new support request in the Dreamhost control panel, I receive the following error in the console which prevents me from continuing past the first page.

Security Error: Content at https://secure.newdream.net/newpanel/js/ajaxform.js may not load data from https://panel.dreamhost.com/index.cgi?tree=support.msg&.

Followed by:
Error: this.divid.getElementsByClassName("buttonbox").each is not a function
Source File: https://secure.newdream.net/newpanel/js/ajaxform.js
Line: 33

On the off chance it's relevant, I'm attaching ajaxform.js as well.
Flags: blocking1.9?
We need a full testcase here. The js file doesn't help much without knowing what page it operates on.

And why is this filed on the XBL component?
What makes you think it's an XBL issue?
This regressed between the 2007-02-01 and 2007-02-02 nightlies. Regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-01+04%3A00%3A00&maxdate=2007-02-02+10%3A00%3A00&cvsroot=%2Fcvsroot

I have no idea what component this should be filed under. I guess if we can decide what caused it, it can be changed then.
Here's a guess: whatever JS library Dreamhost uses adds getElementsByClassName to Element.prototype, *but only if it's not defined.*  The object returned by the getElementsByClassName provided by Dreamhost is a JS array.  If Dreamhost uses a JS library like, say, Prototype, that adds properties to Array.prototype (in this case a property "each"), then Dreamhost's getElementsByClassName would have a working each method because its return values have Array.prototype as their prototype.  Firefox's, however, would be NodeLists, and whatever JS library Dreamhost uses doesn't add "each" to NodeList.

This shouldn't be too hard to check -- just open Dreamhost's JS files, search for getElementsByClassName, see the conditions under which it's set and the type of its return value, and do the same for Array and each.
Attached file prototype.js
So I think the security warning is irrelevant to this bug.
Attached file scriptaculous.js
So prototype.js redefines getElementsByClassName on HTMLElement.prototype, and then it calls that function and expects it to work.  However, from what Ryan says this overriding doesn't actually work, because if runs:

javascript:alert(document.body.getElementsByClassName);

...he gets the [native code] version.  Judging by the following URL, it looks like the override on HTMLElement.prototype isn't working:

data:text/html,<script>window.onload%20=%20function()%20{%20alert(HTMLElement.prototype.getElementsByClassName);%20HTMLElement.prototype.getElementsByClassName%20=%20function%20(name)%20{%20return%2012;%20};%20alert(HTMLElement.prototype.getElementsByClassName);%20alert(document.body.getElementsByClassName("foo"));%20};%20</script>

According to bz, this is because all the methods live on the "most-derived" interface, in this case most likely HTMLDivElement at a guess, so the HTMLElement version doesn't shine through (and never will, if we define getElementsByClassName natively).

I tend to think this is a site bug (and that Prototype shouldn't do what it's doing), but others might think otherwise.
Moving to a slightly more reasonable component for now...
Component: XBL → DOM: Mozilla Extensions
QA Contact: xbl → general
It does suck though that you can't override stuff by setting it on HTMLElement.property. That's definitely how I would have expected it to work. Is there any chance we can get that fixed, or is that a moz2.0 project?
Summary: Unable to submit a new support request using Dreamhost Control Panel → Can't override getElementsByClassName by setting it on HTMLElement.prototype
There've been some e-mail discussions about it, including a thread on webapi, as I recall.  And yes, we should get it fixed.
Is there an explanation for the "regression" mentioned in comment 3? The "can't override methods using HTMLElement.prototype" issue that this bug seems to have morphed into isn't a regression. Was that comment only about the security warning?
The "regession" was that we implemented a function with the same name as the site was already using, thereby hiding the site version with our own.
Not a blocker. And besides, fixing this is really hard (TM), we'd need to redesign how XPConnect exposes prototype objects etc.
Flags: blocking1.9? → blocking1.9-
Is this really that hard to fix? Just asking. I think we want to change our DOM to do what Webkit does, FWIW. See

http://lists.w3.org/Archives/Public/public-webapi/2007Jun/0025.html

/be
> Is this really that hard to fix?

Right now XPConnect puts all the methods it exposes on the immediate prototype of the XPCWrappedNative.  That is, that's where it installs its various hooks, etc.

Changing that in an alpha might be doable.  Changing it now is Hard.

If we do want to fix it, we should remove the XPConnect IDL definition (or make it noscript) and implement this in classinfo somehow (and try to detect the function up the proto chain as needed).
This + getElementByClassName looks to have broken apple.com (bug 412150).  Is there some other way around this?
Flags: blocking1.9- → blocking1.9?
Yeah, could we special-case this method somehow?
Bug 412150 is a different issue.  It's using a framework (prototype.js) which does:

  if (!document.getElementsByClassName) 
    document.getElementsByClassName = function(instanceMethods){

etc.  So fixing this bug would have no effect on apple.com.

It's too bad the edge case behavior of prototype's getElementsByClassName is so different.
Removing my own nom request based on comment 20
Flags: blocking1.9? → blocking1.9-
Blocks: 357450
If the ability to override (or even just extend) DOM types, by modifying/adding properties to their prototype chains, is important to interoperability with other browsers, some specification needs to define how it should work. I believe IE doesn't (or didn't) allow it all, and they aren't violating any spec by not allowing it (besides maybe the ECMAScript specs).

The most likely candidate for this is probably HTML5, although it doesn't have a nice section on ECMAScript bindings like the W3C specs do, e.g.
http://www.w3.org/TR/DOM-Level-3-Core/ecma-script-binding.html

If this isn't considered important to interoperability, this bug should be marked invalid. If this is considered import to interoperability, we should work to get this specified in detail and then conform to those specifications.

Also, I'm wondering how this will be affected by JavaScript 2 (ECMAScript 4). When Mozilla gets that rolled out, will DOM types be defined using classes and/or interfaces? Would that affect the prototype chain in some way?
would this fix bug 405903? (ref bug 405903 comment 29)
Blocks: iWeb
Interestingly, the following code works:

document.createElement('div') instanceof HTMLElement
// returns true

Yet nowhere in the prototype chain of that element is there HTMLElement.prototype:

var found = false
var div = document.createElement('div')
while (div = div.__proto__) {
  if (div == HTMLElement.prototype) found = true
}
// found === false at the end

Maybe this is a new bug, but instanceof should NOT be returning true if the corresponding function's prototype is not in the prototype chain of the object. 

As far as overriding properties goes, you can solve this by going through all the derivative classes (HTMLDivElement, etc.) and setting the override on each of them instead of at the HTMLElement level. If you want to add _new_ properties to all HTMLElements, it also seems to work to go through each derivative class and set __proto__ to HTMLElement.prototype first.
(In reply to comment #24)
> var found = false
> var div = document.createElement('div')
> while (div = div.__proto__) {
>   if (div == HTMLElement.prototype) found = true
> }
> // found === false at the end

This works for me (found === true for me).
> instanceof should NOT be returning true if the corresponding function's
> prototype is not in the prototype chain

That's not workable given that the DOM involves multiple inheritance.  But in any case, HTMLElement.prototype is in fact in the prototype chain for a <div>.
(In reply to comment #25)
> (In reply to comment #24)
> > var found = false
> > var div = document.createElement('div')
> > while (div = div.__proto__) {
> >   if (div == HTMLElement.prototype) found = true
> > }
> > // found === false at the end
> 
> This works for me (found === true for me).
> 

Hmm it also works for me, I ran that in a console after messing around with some stuff, I probably used an '=' somewhere instead of '=='. My fault, please disregard my comment.
(In reply to comment #26)
> > instanceof should NOT be returning true if the corresponding function's
> > prototype is not in the prototype chain
> 
> That's not workable given that the DOM involves multiple inheritance.  But in
> any case, HTMLElement.prototype is in fact in the prototype chain for a <div>.
> 

We shouldn't be changing the semantics of ECMAScript to suit bindings to an external library, even if its an important one like DOM. ECMAScript3 (and 4 incidentally) say instanceof should follow the prototype chain. If there is an interface or something that is implemented by the object but not part of the prototype chain, instanceof should return false. Are there any examples where it currently returns true (my HTMLElement example doesn't count anymore :))
> ECMAScript3 (and 4 incidentally) say instanceof should follow the prototype
> chain.

Oh, really?  Ignoring for the moment that all this is completely off-topic for this bug, ES3 has the following to say, in section 11.8.6:

  The production Relational Expression: Relational Expression instanceof Shift
  Expression is evaluated as follows: 
  1. Evaluate Relational Expression. 
  2. Call GetValue(Result(1)). 
  3. Evaluate Shift Expression. 
  4. Call GetValue(Result(3)). 
  5. If Result (4) is not an object, throw a TypeError exception. 
  6. If Result (4) does not have a [[HasInstance]] method, throw a TypeError
     exception. 
  7. Call the [[HasInstance]] method of Result(4) with parameter Result(2). 
  8. Return Result(7) . 

See section 8.6.2 for the description of [[HasInstance]] and note the "host objects may implement these internal methods with any implementation-dependent behavior" bit.  I assure you that HTMLElement.prototype is a host object, not a native ECMAScript object.

But in any case, there is no mention of the prototype chain anywhere here, and in fact per ES3 spec something like |{} instanceof Object.prototype| should throw TypeError (and does, in Gecko).  Something you could have easily verified before making a clearly false statement like "instanceof should follow the prototype chain".

I see nothing obvious in ES4 that changes any of this.
I just Googled "ECMAScript 3 instanceof" and found the following quote in the spec for ES4 relational operators:

"The instanceof operator behaves in the same way as in ECMAScript 3 — a instanceof b follows a’s prototype chain."
http://docs.huihoo.com/web/js/es4/core/expressions.html

I assumed that the page claiming to be an ES4 spec was making a correct statement about the ES3 spec, though looking more closely, this is Netscape's ES4 spec? I'm not familiar enough with the progress of ES4 to independently verify the claims on the page or whether they have changed since that page was written, so I'll defer to you on that. However, I didn't just make something up, thanks.

As far as |{ } instanceof Object.prototype| goes, Object.prototype is an object, not a function, so it should certainly throw an error. |{ } instanceof Object| however, should return true (and does.)

In any case, thanks for the clarification and sorry for the confusion on the bug report.
(In reply to comment #16)
> > Is this really that hard to fix?
> 
> Right now XPConnect puts all the methods it exposes on the immediate prototype
> of the XPCWrappedNative.  That is, that's where it installs its various hooks,
> etc.
> 
> Changing that in an alpha might be doable.  Changing it now is Hard.

How about now (mozilla-central for 3.7)?

> If we do want to fix it, we should remove the XPConnect IDL definition (or make
> it noscript) and implement this in classinfo somehow (and try to detect the
> function up the proto chain as needed).

Isn't a sufficient fix to find the class's prototype, or in multiple-inheritance cases the next concrete class down, and define the method there? Maciej's recap of what WebKit does (or did) from

http://lists.w3.org/Archives/Public/public-webapi/2007Jun/0025.html

"Generally, we make the same prototype chain you describe, but we put  
methods on the JS [[Class]] that defines them. In cases of multiple  
interfaces or multiple inheritance, we fold the set of methods into  
the next concrete class to inherit from the interfaces."

Sounds doable, not saying it's easy.

Why would the IDL definition go away or become noscript?

/be
> How about now (mozilla-central for 3.7)?

If we have someone (or two) to do the work and a plan for how to handle the multiple-inheritance situation, sure.  We're in an alpha cycle, after all.

> Why would the IDL definition go away or become noscript?

That suggestion was scoped in a "if we want to fix this without fundamental changes to XPConnect" block.

I have no idea what Maciej means by "fold".  What's needed to fix this, in general, is lookup along an ancestor tree (due to multiple inheritance), not ancestor chain.  That certainly seems to be the way Webkit does it, one way or another.
Is this bug still relevant?
Yes, though bug 580070 will fix it.
Depends on: ParisBindings
Component: DOM: Mozilla Extensions → DOM
Fixed by putting all Nodes on Web IDL bindings.  Not going to worry about looking up that bug number...
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.