92071 - HTMLSpanElement does not inherit from Core Objects

Status: VERIFIED FIXED

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

Description

[tfriesen]

This happens in recent builds (July 23)

Example:

<div onmouseup="alert(this.test)">DIV Tag</div>
<span onmouseup="alert(this.test)">SPAN Tag</span>


<script>

Element.prototype.test="testing";
//HTMLSpanElement.prototype.test="testing";

</script>

Comment 1

[basic]

reporter please attach a testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase -- click on the text to see what's up

Comment 3

[Boris Zbarsky [:bzbarsky]]

seeing this on linux build 2001-07-23-08 as well.

Odd, since nsHTMLDivElement and nsHTMLSpanElement inherit from the same things...

Comment 4

[Johnny Stenback (:jst)]

Odd, but accepting...

Comment 5

[Johnny Stenback (:jst)]

Moving to mozilla0.9.5

Comment 6

[Fabian Guisset]

/me would like to see the fix if you still have it, it'd be interesting :-)

Comment 7

[Johnny Stenback (:jst)]

Attachment: Proposed fix

Comment 8

[Johnny Stenback (:jst)]

The problem here is that the span class does not have a leaf interface as its
primary interface, because of this the code that sets up the prototype chain on
DOM objects ends up not setting the prototype of the span class' prototype so
that the chain is linked togethere. Here's a image that shows the difference
between div and span, note the lack of a nsIDOMHTMLSpanElement leaf interface:

 HTMLDivElement                 HTMLSpanElement
        |                              |
nsIDOMHTMLDivElement           nsIDOMHTMLElement
        |                              |
 nsIDOMHTMLElement               nsIDOMElement
        |                              |
  nsIDOMElement                   nsIDOMNode
        |                              |
   nsIDOMNode                     nsISupports
        |
   nsISupports

Comment 9

[Sivakiran Tummala]

johnny,
is this fix going to effect any other elements from the inheritance point of view.

Comment 10

[Johnny Stenback (:jst)]

Sivakiran, yes, this will affect other elements too, the elements should be:

HEAD, SUB, SUP, SPAN, BDO, TT, I, B, U, S, STRIKE, BIG, SMALL, EM, STRONG, DFN,
CODE, SAMP, KBD, VAR, CITE, ACRONYM, ABBR, DD, DT, NOFRAMES, NOSCRIPT, ADDRESS,
CENTER

But mozilla doesn't really support all of those correctly, so I'm not sure how
much that list means here.

Comment 11

[Fabian Guisset]

i knew it! it's class info's fault! it's always class info's fault! ;-)

Comment 12

[Sivakiran Tummala]

thanks johnny,
it helps me in knowing which area i need to concentrate more for testing.

Comment 13

[Hixie (not reading bugmail)]

So, er, why no nsIDOMHTMLSpanElement?

Comment 14

[Fabian Guisset]

Now that I think of it... doesn't this happen to all DOM classes whose
mProtoChainChainInterface interface is nsIDOMHTMLElement?
namely HTMLDelElement, HTMLInsElement, HTMLSpacerElement, HTMLUnknownElement
(not sure this one should), HTMLWBRElement.
Is there a global solution to fix them all at once?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> So, er, why no nsIDOMHTMLSpanElement?

Because it's not in DOM2 HTML.  See
http://www.w3.org/TR/2001/WD-DOM-Level-2-HTML-20011025/html.html#ID-58190037 and
the table of contents of
http://www.w3.org/TR/2001/WD-DOM-Level-2-HTML-20011025/html.html (which does not
list HTMLSpanElement)

Comment 16

[Johnny Stenback (:jst)]

Fabian, yes, all those will be fixed.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Better fix.

Comment 18

[Johnny Stenback (:jst)]

The attached patch fixes this problem. The patch adds a mHasClassInterface to
nsDOMClassInfoData so every class knows if it's proto chain interface is
specific to that class or not. For those classes where we don't have a class
interface we need to use the name of the proto chain interface for setting up
ctor.prototype.__proto__, whereas in the case of a class that *does* have a
class interface we use the parent of the proto chain interface when setting up
ctor.prototype.__proto__.

So in the case where we wrap a span element, our proto chain interface is
nsIDOMHTMLElement (since there is no nsIDOMHTMLSpanElement),
nsDOMClassInfo::PostCreate() ends up resolving "HTMLSpanElement" (before the
patch it incorrectly ended up resolving "HTMLElement" so the HTMLSpanElement
ctor was never properly set up). When resolving "HTMLSpanElement" we'll see that
the class doesn't have a class interface so we use the proto chain interface
nsIDOMHTMLElement directly (in stead of using its parent nsIDOMElement) for
figuring out what to set HTMLSpanElement.prototype.__proto__ to.

Oh, and storing mScriptableFlags in 31 bits is ok, see nsIXPCScriptable::RESERVED.

Comment 19

[Johnny Stenback (:jst)]

Actually the patch doesn't add mHasClassInterface, that was added by mistake in
a checkin unrelated to this bug. That's why you don't see the code that
specifies what classes don't have a class interface in this patch.

Here's the patch that was checked in earlier by mistake:

 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsDOMClassInfo.cpp&root=/cvsroot&subdir=mozilla/dom/src/base&command=DIFF_FRAMESET&rev1=1.72&rev2=1.73

Comment 20

[Fabian Guisset]

Comment on attachment 56702 [details] [diff] [review]
Better fix.

r=fabian after jst fixed 
iim->GetInfoForIID(sClassInfoData[mID].mProtoChainInterface
to be
iim->GetInfoForIID(ci_data.mProtoChainInterface

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 56702 [details] [diff] [review]
Better fix.

r=peterv

Comment 22

[John Bandhauer]

Comment on attachment 56702 [details] [diff] [review]
Better fix.

sr=jband

Comment 23

[Johnny Stenback (:jst)]

Attachment: Same as above, but even better.

Comment 24

[Johnny Stenback (:jst)]

So I had to tweak this a bit before checking in since I found one more minor
problem, I found more classes that had class interfaces (in theory, at least)
but whose name (due to conflicts with other class names) didn't match the name
of the proto chain interface. I also managed to eliminate a nsIID copy, so I did
that while I was poking around in this code. With this patch,
ComputedCSSStyleDeclaration.prototype.__proto__ ===
CSSStyleDeclaration.prototype, yay! That didn't work before this latest change.

The latest patch is checked in, if someone feels the need to do a post-checkin
review of the latest modifications, be my guest.

Oh, the latest patch is missing what Fabian pointed out, but the checked in
version did contain that fix.

Marking FIXED.

Comment 25

[Fabian Guisset]

Just wanted to let you know that there is now a warning that dbg_rv is not used.
And I'm afraid the compiler is right ;-)

Comment 26

[Johnny Stenback (:jst)]

Ah, that is true (in debug builds), I'll take out the unused variable...

Comment 27

[Sivakiran Tummala]

verified fixed