We should treat 'id' and 'class' as global attributes for all elements

RESOLVED FIXED in mozilla32



7 years ago
4 years ago


(Reporter: sicking, Assigned: Ms2ger)


({dev-doc-complete, perf})

dev-doc-complete, perf
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments, 12 obsolete attachments)

5.34 KB, patch
Details | Diff | Splinter Review
81.48 KB, patch
Details | Diff | Splinter Review
Created attachment 611384 [details] [diff] [review]

This is now sanctioned by the DOM4 spec, and is also significantly more sane for all parties involved.

The attached patch passes basic local testing. Running tryserver tests now.
Created attachment 611616 [details] [diff] [review]

This builds on linux and osx, but fails horribly on try-server with leaks and failed tests everywhere.
Attachment #611384 - Attachment is obsolete: true
Will this make a difference for the Web developers? (Trying to figure if we need a dev-doc-needed here). Are there elements now that doesn't have id or class attributes?
Yes, this will definitely make a difference to web developers. It mostly will affect people that use "raw" XML documents in either the null-namespace or in custom namespaces.
Keywords: dev-doc-needed
Created attachment 643636 [details] [diff] [review]

Attaching for safekeeping. This still fails a bunch of tests.
Attachment #611616 - Attachment is obsolete: true
Probably better to wait until I land mPrototype removal (probably tomorrow).
Created attachment 648245 [details] [diff] [review]
sync to tip

Synced to tip. I've pushed to try to see if it's gotten magically fixed, but I suspect that's not the case :) If so, help would be appreciated in finishing this up.
Attachment #643636 - Attachment is obsolete: true
Jonas, try push url?
Created attachment 648506 [details] [diff] [review]
Fix a few tests

This fixes a couple of the easy test failures. The remaining problems are:

SVGElement.className no longer overrides Element.className. Unfortunately SVG does this fun thing where SVGElement.className returns an animated value. It used to work to just put SVGElement after Element in the nsDOMClassInfo macros, however that no longer seems to do the trick.

The reftests for 495354-1a.xhtml and 495354-1b.xhtml no longer work. I haven't looked into why at all. Nothing obvious pops out so I suspect there's a real bug here, possibly something in the XBL code.

The try run (which doesn't include all of the fixes, but shows the above problems) are here: https://tbpl.mozilla.org/?tree=Try&rev=7a595722301e
Attachment #648245 - Attachment is obsolete: true
I think we tracked down the override problem to a problem in the quickstubs code. Pushed a new patch to try:

Hrm.  I thought quickstubs in fact did the same ordering thing as XPConnect, and the comment explains why.  The point is that classinfo can be used to _override_ the default XPConnect order, no?

I wonder whether the right fix there is to just fix quickstubs to use classinfo order.

> The reftests for 495354-1a.xhtml and 495354-1b.xhtml no longer work.

That's because those tests look like this:

  ...<binding id="x">....
  <body onload="document.getElementById('x').innerHTML = '9';">
  <span id="x" style="-moz-binding: url(#x)"><div>123</div></span>

so we used to set the innerHTML of the <span> but now we try to do it to the <binding> and that of course doesn't do all that much.  ;)

Just giving the span and the binding different IDs should fix those.
I think quickstubs is using classinfo order? Isn't this call doing that?


My recollection was that XPConnect using the *last* interface in the classinfo list, which is opposite of what the comment says. But looking at the code that doesn't appear to be the case. I'll try to write a test which confirms by using an xray wrapper.

Good catch on the reftest thing!

For those following along at home, here's the latest try push which I expect will be fully green:


If that is fully green I'll just have to confirm which order XPConnect uses and we should be good to go!
This is going to cause failures in <http://w3c-test.org/webapps/DOMCore/tests/approved/Node-properties.html>, FWIW.  Was the try run green after all?
(I mean this bug causes failures in the test suite, not that the patch does.  The patch will fix the failures.)


6 years ago
Assignee: nobody → Ms2ger


6 years ago
Depends on: 825282

Comment 14

6 years ago
Created attachment 696357 [details] [diff] [review]

Rebased, failing content/smil/test/test_smilValues.xhtml because of shadowing; should be fixed by bug 825282.
Attachment #648506 - Attachment is obsolete: true
You should be able to get the shadowing right without waiting for paris bindings. The order that interfaces are listed in classinfo *should* determine which interface shadows what.

As I now recall it, I got stuck on the fact that XPConnect and quickstubs are treating the interface order in the classinfo datastructures differently. I.e. one matches the first interface in the list, one matches the last.

Fixing this should be relatively easy though. What I got stuck on was writing tests IIRC. Though with SpecialPowers.wrap that should be pretty easy too.
nsIDOMElement and nsIDOMSVGElement are not listed separately in classinfo.  Only the latter is listed.

And quickstubs define properties from most derived to least derived up the parent chain of things listed in classinfo, so the least derived wins.

I suppose we could explicitly list nsIDOMElement in classinfo in addition to nsIDOMSVGElement....


6 years ago
Duplicate of this bug: 810677

Comment 18

5 years ago
Created attachment 775318 [details] [diff] [review]

This breaks layout/reftests/bugs/495354-1{a,b}.xhtml, probably because the two elements with id=x.
Attachment #696357 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Yeah, I was lazy.  Change the binding id to "y" in both tests and update the -moz-binding rule?  ;)
Flags: needinfo?(bzbarsky)

Comment 20

5 years ago
Created attachment 780513 [details] [diff] [review]

Now it appears to break windows reftests: https://tbpl.mozilla.org/?tree=Try&rev=7f2185bf1a70
Attachment #775318 - Attachment is obsolete: true

Comment 21

5 years ago
Created attachment 8368109 [details] [diff] [review]

Doesn't build on windows because GetClassNameW. Kyle, I'd really appreciate it if you could take a look.
Attachment #780513 - Attachment is obsolete: true
Flags: needinfo?(khuey)

Comment 22

5 years ago
You can use a pattern such as <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#99> to #undef GetClassNameW.  We do this in a whole bunch of places already.
I don't think I'm going to get to look at this before the week of the 17th :(
Flags: needinfo?(khuey)

Comment 24

5 years ago
That's fine. Do you mind if I just leave it in your queue until then?
Flags: needinfo?(khuey)
That's fine, just setting expectations.
Why is this blocked on info from Kyle's? Doesn't comment 22 provide the answer?

Comment 27

5 years ago
Because I don't think that figuring out where those undefs should go on try would be a good use of resources
I'd much rather spend try's resources than Kyle's. This is why we have try after all.
That said, I'm definitely grateful that you're working on finishing this up. So if you prefer to wait for Kyle's feedback then that's totally up to you. But don't feel shy about using try resources either.
Ehsan, can you help Ms2ger here?  I don't think I'm going to get to this :/
Flags: needinfo?(ehsan)

Comment 31

5 years ago
Yes, I'd be happy to.  Applying the patch on Windows right now and getting a build going...
Flags: needinfo?(ehsan)

Comment 32

5 years ago
This windows.h include is coming from Accessible2.h which is included in AccessibleWrap.h.  This patch unbreaks that header:

Flags: needinfo?(khuey)
Keywords: leave-open

Comment 33

5 years ago
Created attachment 8386882 [details] [diff] [review]
Rebased patch on top of inbound
ms2ger: What's remaining here? Just tests?

Comment 36

5 years ago
Created attachment 8391835 [details] [diff] [review]
Patch v1


One remaining issue: B2G Desktop Windows turns out to have windows.h issues as well :/
Attachment #8368109 - Attachment is obsolete: true
Attachment #8386882 - Attachment is obsolete: true
Attachment #8391835 - Flags: review?(bzbarsky)
Comment on attachment 8391835 [details] [diff] [review]
Patch v1

>+  mozilla::dom::DOMString attr;                                               \

Just add an overload of Element::GetClassName that takes an nsAString?  That's how GetId() works.

It looks like with this change the atom version of GetID() will stop being callable from outside libxul, right?  I guess that's ok; people can use the string version...

> Attr::GetIsId

Seems like this method should not longer do the GetElement() bit at all?

>+      doc->RemoveFromIdTable(this, DoGetID());

Why not aId?  If there's a reason, at least document it.

>@@ -1206,16 +1225,20 @@ Element::BindToTree(nsIDocument* aDocume

The changes to this method don't make sense to me.  There's already a call to AddToIdTable there, and you're adding another one?

>+  virtual const nsAttrValue* GetAnimatedClassName() const {

Does this need to be virtual?

>+    // XXX: In the .cpp, we inherit from nsStyledElement.

Why don't we in the header?

The rest looks fine, but I'd like an explanation of what's going on in Element::BindToTree.
Attachment #8391835 - Flags: review?(bzbarsky) → review-

Comment 38

5 years ago
Created attachment 8392949 [details] [diff] [review]
Patch v2
Attachment #8392949 - Flags: review?(bzbarsky)

Comment 39

5 years ago
Created attachment 8392951 [details] [diff] [review]
Interdiff v1 -> v2
Attachment #8391835 - Attachment is obsolete: true

Comment 40

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #37)
> Comment on attachment 8391835 [details] [diff] [review]
> Patch v1
> >+      doc->RemoveFromIdTable(this, DoGetID());
> Why not aId?  If there's a reason, at least document it.

No reason AFAICT; in fact, this function only has one caller left at this point, so I merged it into the no-argument form.

> >+    // XXX: In the .cpp, we inherit from nsStyledElement.
> Why don't we in the header?

No idea; can I claim pre-existing?
Comment on attachment 8392949 [details] [diff] [review]
Patch v2

r=me based on the interdiff
Attachment #8392949 - Flags: review?(bzbarsky) → review+
> No idea; can I claim pre-existing?

Just fix the header, please.
Ms2ger: This is so close! Any chance you can fix review comments and get this landed?

Comment 45

5 years ago
Created attachment 8427262 [details] [diff] [review]
Treat 'id' and 'class' as global attributes for all elements


5 years ago
Attachment #8392949 - Attachment is obsolete: true

Comment 46

5 years ago
This fixes the Windows b2g-desktop build error that M2ger hit on try: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fa0e946574

Comment 48

5 years ago
Thanks all!

Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32


5 years ago
Keywords: leave-open
Thanks for bringing this to the finish line!

Though I would have appreciated being mentioned in the checkin comment given the work I did :(

Anyhow, isn't this still missing tests?

Comment 50

4 years ago
I have a dumb question. There's a comment in HTMLElement.webidl that references the dupe (bug 810677) because HTMLElements still have a className property. Can that comment just be removed or is there something left to do here?
Flags: needinfo?(Ms2ger)
We should remove className from HTMLElement.


4 years ago
Depends on: 1035667

Comment 52

4 years ago
Yep, bug 1035667.
Flags: needinfo?(Ms2ger)
Also, this landed without tests :(
You need to log in before you can comment on or make changes to this bug.