HTMLSpanElement does not inherit from Core Objects

VERIFIED FIXED in mozilla0.9.6

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: tfriesen, Assigned: jst)

Tracking

Trunk
mozilla0.9.6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(4 attachments)

(Reporter)

Description

16 years ago
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

16 years ago
reporter please attach a testcase
Created attachment 43368 [details]
testcase -- click on the text to see what's up
seeing this on linux build 2001-07-23-08 as well.

Odd, since nsHTMLDivElement and nsHTMLSpanElement inherit from the same things...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows ME → All
(Assignee)

Comment 4

16 years ago
Odd, but accepting...
Status: NEW → ASSIGNED
Hardware: PC → All
Target Milestone: --- → mozilla0.9.4
(Assignee)

Comment 5

16 years ago
Moving to mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 6

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

Comment 7

16 years ago
Created attachment 49666 [details] [diff] [review]
Proposed fix
(Assignee)

Comment 8

16 years ago
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
(Assignee)

Updated

16 years ago
Whiteboard: [HAVE FIX]

Comment 9

16 years ago
johnny,
is this fix going to effect any other elements from the inheritance point of view.
(Assignee)

Comment 10

16 years ago
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

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

Comment 12

16 years ago
thanks johnny,
it helps me in knowing which area i need to concentrate more for testing.
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6
So, er, why no nsIDOMHTMLSpanElement?

Comment 14

16 years ago
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?
> 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)
(Assignee)

Comment 16

16 years ago
Fabian, yes, all those will be fixed.
(Assignee)

Comment 17

16 years ago
Created attachment 56702 [details] [diff] [review]
Better fix.
(Assignee)

Comment 18

16 years ago
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.
(Assignee)

Comment 19

16 years ago
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

16 years ago
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
Attachment #56702 - Flags: review+
Comment on attachment 56702 [details] [diff] [review]
Better fix.

r=peterv

Comment 22

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

sr=jband
Attachment #56702 - Flags: superreview+
(Assignee)

Comment 23

16 years ago
Created attachment 56860 [details] [diff] [review]
Same as above, but even better.
(Assignee)

Comment 24

16 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 25

16 years ago
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 ;-)
(Assignee)

Comment 26

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

Comment 27

16 years ago
verified fixed
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.