Closed Bug 741295 Opened 13 years ago Closed 11 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: sicking, Assigned: Ms2ger)

References

Details

(Keywords: dev-doc-complete, perf)

Attachments

(2 files, 12 obsolete files)

Interdiff v1 -> v2
5.34 KB, patch
Details | Diff | Splinter Review
Treat 'id' and 'class' as global attributes for all elements
81.48 KB, patch
Details | Diff | Splinter Review
Attached patch WIP (obsolete) — — Splinter 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.
Attached patch WIP (obsolete) — — Splinter 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
Attached patch WIP (obsolete) — — Splinter 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).
Attached patch sync to tip (obsolete) — — Splinter Review
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?
Attached patch Fix a few tests (obsolete) — — Splinter Review
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: https://tbpl.mozilla.org/?tree=Try&rev=a44a6f1e2654
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? http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#5041 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: https://tbpl.mozilla.org/?tree=Try&rev=dbf2138ea811 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.)
Assignee: nobody → Ms2ger
Depends on: 825282
Attached patch WIP (obsolete) — — Splinter 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....
Attached patch WIP (obsolete) — — Splinter 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)
Attached patch WIP (obsolete) — — Splinter Review
Now it appears to break windows reftests: https://tbpl.mozilla.org/?tree=Try&rev=7f2185bf1a70
Attachment #775318 - Attachment is obsolete: true
Attached patch WIP (obsolete) — — Splinter 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)
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)
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?
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)
Yes, I'd be happy to. Applying the patch on Windows right now and getting a build going...
Flags: needinfo?(ehsan)
This windows.h include is coming from Accessible2.h which is included in AccessibleWrap.h. This patch unbreaks that header: https://hg.mozilla.org/integration/mozilla-inbound/rev/bac0b83a2394
Flags: needinfo?(khuey)
Keywords: leave-open
ms2ger: What's remaining here? Just tests?
Attached patch Patch v1 (obsolete) — — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=38b23ade3651 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-
Attached patch Patch v2 (obsolete) — — Splinter Review
Attachment #8392949 - Flags: review?(bzbarsky)
Attachment #8391835 - Attachment is obsolete: true
(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?
Attachment #8392949 - Attachment is obsolete: true
This fixes the Windows b2g-desktop build error that M2ger hit on try: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fa0e946574
Thanks all! https://hg.mozilla.org/mozilla-central/rev/e6f113c83095
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
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.
Depends on: 1035667
Yep, bug 1035667.
Flags: needinfo?(Ms2ger)
Also, this landed without tests :(
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: