Last Comment Bug 741295 - We should treat 'id' and 'class' as global attributes for all elements
: We should treat 'id' and 'class' as global attributes for all elements
Status: RESOLVED FIXED
: dev-doc-complete, perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla32
Assigned To: :Ms2ger
:
Mentors:
: 810677 (view as bug list)
Depends on: 825282 1035667
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 01:12 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2015-01-10 08:28 PST (History)
15 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (117.76 KB, patch)
2012-04-02 01:12 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
WIP (139.86 KB, patch)
2012-04-02 15:23 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
WIP (139.86 KB, patch)
2012-07-18 15:32 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
sync to tip (137.71 KB, patch)
2012-08-02 01:18 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
Fix a few tests (141.02 KB, patch)
2012-08-02 15:01 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
WIP (83.34 KB, patch)
2012-12-28 11:16 PST, :Ms2ger
no flags Details | Diff | Splinter Review
WIP (85.87 KB, patch)
2013-07-14 04:01 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
WIP (88.36 KB, patch)
2013-07-24 11:16 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
WIP (81.17 KB, patch)
2014-01-30 12:10 PST, :Ms2ger
no flags Details | Diff | Splinter Review
Rebased patch on top of inbound (84.45 KB, patch)
2014-03-06 09:25 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch v1 (80.74 KB, patch)
2014-03-16 01:26 PDT, :Ms2ger
bzbarsky: review-
Details | Diff | Splinter Review
Patch v2 (81.29 KB, patch)
2014-03-18 09:33 PDT, :Ms2ger
bzbarsky: review+
Details | Diff | Splinter Review
Interdiff v1 -> v2 (5.34 KB, patch)
2014-03-18 09:34 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
Treat 'id' and 'class' as global attributes for all elements (81.48 KB, patch)
2014-05-22 13:30 PDT, :Ms2ger
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2012-04-02 01:12:56 PDT
Created attachment 611384 [details] [diff] [review]
WIP

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.
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-02 15:23:55 PDT
Created attachment 611616 [details] [diff] [review]
WIP

This builds on linux and osx, but fails horribly on try-server with leaks and failed tests everywhere.
Comment 2 Jean-Yves Perrier [:teoli] 2012-04-02 15:30:45 PDT
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?
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-02 15:39:38 PDT
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.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-18 15:32:30 PDT
Created attachment 643636 [details] [diff] [review]
WIP

Attaching for safekeeping. This still fails a bunch of tests.
Comment 5 Olli Pettay [:smaug] 2012-07-18 16:31:21 PDT
Probably better to wait until I land mPrototype removal (probably tomorrow).
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-02 01:18:33 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2012-08-02 08:07:20 PDT
Jonas, try push url?
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-02 15:01:04 PDT
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
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-03 15:14:23 PDT
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
Comment 10 Boris Zbarsky [:bz] 2012-08-03 20:03:03 PDT
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.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-04 23:27:31 PDT
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!
Comment 12 :Aryeh Gregor (away until August 15) 2012-10-14 07:23:52 PDT
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?
Comment 13 :Aryeh Gregor (away until August 15) 2012-10-15 00:21:05 PDT
(I mean this bug causes failures in the test suite, not that the patch does.  The patch will fix the failures.)
Comment 14 :Ms2ger 2012-12-28 11:16:57 PST
Created attachment 696357 [details] [diff] [review]
WIP

Rebased, failing content/smil/test/test_smilValues.xhtml because of shadowing; should be fixed by bug 825282.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2012-12-28 12:36:06 PST
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.
Comment 16 Boris Zbarsky [:bz] 2012-12-28 12:38:19 PST
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....
Comment 17 :Ms2ger 2013-03-17 07:41:16 PDT
*** Bug 810677 has been marked as a duplicate of this bug. ***
Comment 18 :Ms2ger 2013-07-14 04:01:23 PDT
Created attachment 775318 [details] [diff] [review]
WIP

This breaks layout/reftests/bugs/495354-1{a,b}.xhtml, probably because the two elements with id=x.
Comment 19 Boris Zbarsky [:bz] 2013-07-14 20:17:26 PDT
Yeah, I was lazy.  Change the binding id to "y" in both tests and update the -moz-binding rule?  ;)
Comment 20 :Ms2ger 2013-07-24 11:16:24 PDT
Created attachment 780513 [details] [diff] [review]
WIP

Now it appears to break windows reftests: https://tbpl.mozilla.org/?tree=Try&rev=7f2185bf1a70
Comment 21 :Ms2ger 2014-01-30 12:10:56 PST
Created attachment 8368109 [details] [diff] [review]
WIP

Doesn't build on windows because GetClassNameW. Kyle, I'd really appreciate it if you could take a look.
Comment 22 :Ehsan Akhgari 2014-01-30 12:24:53 PST
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.
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2014-02-06 17:48:18 PST
I don't think I'm going to get to look at this before the week of the 17th :(
Comment 24 :Ms2ger 2014-02-07 01:02:46 PST
That's fine. Do you mind if I just leave it in your queue until then?
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2014-02-11 02:01:50 PST
That's fine, just setting expectations.
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2014-02-19 14:08:49 PST
Why is this blocked on info from Kyle's? Doesn't comment 22 provide the answer?
Comment 27 :Ms2ger 2014-02-20 13:07:43 PST
Because I don't think that figuring out where those undefs should go on try would be a good use of resources
Comment 28 Jonas Sicking (:sicking) PTO Until July 5th 2014-02-20 14:59:47 PST
I'd much rather spend try's resources than Kyle's. This is why we have try after all.
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2014-02-20 16:12:15 PST
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.
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2014-03-05 08:33:53 PST
Ehsan, can you help Ms2ger here?  I don't think I'm going to get to this :/
Comment 31 :Ehsan Akhgari 2014-03-06 08:11:02 PST
Yes, I'd be happy to.  Applying the patch on Windows right now and getting a build going...
Comment 32 :Ehsan Akhgari 2014-03-06 09:23:26 PST
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
Comment 33 :Ehsan Akhgari 2014-03-06 09:25:36 PST
Created attachment 8386882 [details] [diff] [review]
Rebased patch on top of inbound
Comment 34 Carsten Book [:Tomcat] 2014-03-07 04:18:08 PST
https://hg.mozilla.org/mozilla-central/rev/bac0b83a2394
Comment 35 Jonas Sicking (:sicking) PTO Until July 5th 2014-03-15 11:12:33 PDT
ms2ger: What's remaining here? Just tests?
Comment 36 :Ms2ger 2014-03-16 01:26:50 PDT
Created attachment 8391835 [details] [diff] [review]
Patch v1

https://tbpl.mozilla.org/?tree=Try&rev=38b23ade3651

One remaining issue: B2G Desktop Windows turns out to have windows.h issues as well :/
Comment 37 Boris Zbarsky [:bz] 2014-03-17 20:43:41 PDT
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.
Comment 38 :Ms2ger 2014-03-18 09:33:39 PDT
Created attachment 8392949 [details] [diff] [review]
Patch v2
Comment 39 :Ms2ger 2014-03-18 09:34:21 PDT
Created attachment 8392951 [details] [diff] [review]
Interdiff v1 -> v2
Comment 40 :Ms2ger 2014-03-18 09:37:45 PDT
(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 41 Boris Zbarsky [:bz] 2014-03-18 09:40:49 PDT
Comment on attachment 8392949 [details] [diff] [review]
Patch v2

r=me based on the interdiff
Comment 42 Boris Zbarsky [:bz] 2014-03-18 09:42:24 PDT
> No idea; can I claim pre-existing?

Just fix the header, please.
Comment 43 Jonas Sicking (:sicking) PTO Until July 5th 2014-05-13 14:16:56 PDT
Ms2ger: This is so close! Any chance you can fix review comments and get this landed?
Comment 45 :Ms2ger 2014-05-22 13:30:17 PDT
Created attachment 8427262 [details] [diff] [review]
Treat 'id' and 'class' as global attributes for all elements
Comment 46 :Ehsan Akhgari 2014-05-22 15:10:35 PDT
This fixes the Windows b2g-desktop build error that M2ger hit on try: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fa0e946574
Comment 47 Carsten Book [:Tomcat] 2014-05-23 06:47:40 PDT
https://hg.mozilla.org/mozilla-central/rev/f2fa0e946574
Comment 48 :Ms2ger 2014-05-30 02:12:04 PDT
Thanks all!

https://hg.mozilla.org/mozilla-central/rev/e6f113c83095
Comment 49 Jonas Sicking (:sicking) PTO Until July 5th 2014-05-30 13:52:08 PDT
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 :Gijs Kruitbosch 2014-07-07 15:08:37 PDT
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?
Comment 51 Boris Zbarsky [:bz] 2014-07-07 23:17:05 PDT
We should remove className from HTMLElement.
Comment 52 :Ms2ger 2014-07-08 02:06:29 PDT
Yep, bug 1035667.
Comment 53 Jonas Sicking (:sicking) PTO Until July 5th 2014-07-08 17:03:53 PDT
Also, this landed without tests :(

Note You need to log in before you can comment on or make changes to this bug.