User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre Build Identifier: Fails test. Reproducible: Always

Bogus test assumes IE's broken behavior wrt text nodes in the DOM (in particular removing-them-but-not-really in various cases).

Did they change the test since this bug was filed? For me it now fails on |parentNode.childNodes[i].id === undefined|. Using parentNode.childNodes[i].getAttribute("id") instead leads to PASS.

That's the same thing I was talking about. parentNode.childNodes[i] is a Text node. So .id is undefined. .getAttribute("id") throws, but the test claims "PASS" if an exception is thrown, so that explains what you're seeing in comment 2.

I actually changed the exception PASS to EXCEPTION, and I tested with both the original loop and one that used ++i and |if (node instanceof Text) continue;| (node factored out), and got PASS in both cases. This suggests the Text node skipping happens to work for us, but the .id -> .getAttribute("id") mapping doesn't.

Er, I lied. The test now seems to step 2 at a time. The reason it still fails is that <altGlyphDef> doesn't create an SVGElement so has no id property. That seems to be a bug on our end,

And in particular, iirc we don't create SVGElements for things in the SVG namespace that we don't have specific classes for. And we don't implement SVG fonts so don't have a specific class for altGlyphDef. Perhaps we should to the extent of creating an SVGElement?

All with missing .id: altGlyphDef, altGlyphItem, animateColor, glyphRef

s/missing/undefined/

Not implementing the element allows authors to test whether we've got that functionality by creating an element of that type with an id and then seeing whether getting the id works.

OK, then do we need to push back on Microsoft about this test and them implementing the element?

Unless they really have implemented the elements and their functionality that seems the most sensible thing to do.

(In reply to comment #6) > And in particular, iirc we don't create SVGElements for things in the SVG > namespace that we don't have specific classes for. And we don't implement SVG > fonts so don't have a specific class for altGlyphDef. Perhaps we should to the > extent of creating an SVGElement? Why should we do this differently in SVG and HTML?

> Why should we do this differently in SVG and HTML? I have no idea. It's caused issues in our code in the past, for what it's worth (because the eSVG type check is not equivalent to a namespace check). I'd be really happy to just make all SVG-namespace elements instances of SVGElement, personally.

I suppose the SVG and HTML specs, read literally, do probably lead towards the conclusion that they should be handled differently, although I couldn't find a clear processing model for DOM object creation in either.

(In reply to comment #9) > Not implementing the element allows authors to test whether we've got that > functionality by creating an element of that type with an id and then seeing > whether getting the id works. I'm not sure that's the way I'd recommend testing since it's a bit of an indirect test. I'd rather recommend users do checks like |'SVGAltGlyphDefElement' in window| or, for an existing element, check |element instanceof SVGAltGlyphDefElement| (with an appropriate try-catch). I think I agree with Boris that it would be better to have all elements in the SVG namespace implement SVGElement regardless of tag name.

Or to be more specific I think getElementById should work for elements in the SVG namespace regardless of tag name, as it does in HTML. I've seen authors tripped up by it not working, and this way SVG would be consistent with HTML.

(In reply to Boris Zbarsky (:bz) from comment #14) > I'd be really happy to just make all SVG-namespace elements instances of > SVGElement, personally. (In reply to Jonathan Watt [:jwatt] from comment #16) > I think I agree with Boris that it would be better to have all elements in > the SVG namespace implement SVGElement regardless of tag name. So, perhaps something like this? This patch adds an SVGUnknownElement, like HTMLUnknownElement. It fixes both the id attribute and getElementById, and the unimplemented elements aren't in |window| (so |'AltGlyphDef' in window| still works as an is-supported test). SVGUnknownElement is visible in |window| , though, and isn't in the SVG 1.1 spec. Includes a bonus OCD-test that makes sure IDs work for every element in the SVG spec.

Comment on attachment 576552 [details] [diff] [review] Make elements in the SVG namespaces instances of SVGElement > SVGUnknownElement is visible in |window| Is this still the case if you name the new interface nsISVGUnknownElement? That should prevent us from sticking it on the window.... That said, why do you even need the new interface? can you not use nsIDOMSVGElement directly the way <html:span> uses nsIDOMHTMLElement? Apart from that, this looks fine, I think. As a followup (separate patch), you should be able to change nsIContent->IsSVG() to look more like IsHTML() and nuke the eSVG stuff from IsNodeOfType....

Updated patch based on feedback (In reply to Boris Zbarsky (:bz) from comment #19) > > SVGUnknownElement is visible in |window| > Is this still the case if you name the new interface nsISVGUnknownElement? > That should prevent us from sticking it on the window.... I tried that, but that didn't prevent it from being stuck on the window. > That said, why do you even need the new interface? can you not use > nsIDOMSVGElement directly the way <html:span> uses nsIDOMHTMLElement? It didn't occur to me that I could do that. The new patch does it this way, which is less code and solves the issue above with SVGUnknownElement being on the window.

(In reply to Boris Zbarsky (:bz) from comment #19) > As a followup (separate patch), > you should be able to change nsIContent->IsSVG() to look more like IsHTML() > and nuke the eSVG stuff from IsNodeOfType.... This patch does just that. I didn't change all of the checks that |(nsIContent thing)->GetNameSpaceID() == kNameSpaceID_SVG|, because there were quite a few. I can do that (as part 3), if you'd like, or file a follow-up as needed.

Comment on attachment 578398 [details] [diff] [review] Make elements in the SVG namespaces instances of SVGElement, v2 (Part 1) I'd prefer the classinfo be callsd SVGUnknownElement. Other than that, looks good.

Comment on attachment 578403 [details] [diff] [review] Make nsIContent::IsSVG like IsHTML, and clean up the eSVG node type (part 2) Looks good. r=me on this part.

(In reply to Boris Zbarsky (:bz) from comment #22) > I'd prefer the classinfo be called SVGUnknownElement. I have a local patch that makes that change, but only changing the classinfo name causes two behavioral changes: 1: |'SVGUnknownElement' in window| returns true 2: An assertion is hit: ASSERTION: Class name and proto chain interface name mismatch!: 'nsCRT::strcmp(CutPrefix(name), mData->mName) == 0', file c:/mozilla-src/obj-ff-debug/dom/base/../../../src/dom/base/nsDOMClassInfo.cpp, line 4738 Would it be preferable to create the empty nsI(DOM)SVGUnknownElement interface, or leave the classinfo with a different name (and add a comment explaining)? (Or, something I haven't thought of?)

> I have a local patch that makes that change Hard to say exactly what change you made, but the change you probably should make looks like this: In nsSVGUnknownElement: DOMCI_NODE_DATA(SVGUnknownElement, nsSVGUnknownElement) In nsDOMClassInfo.cpp: NS_DEFINE_CLASSINFO_DATA_WITH_NAME(SVGUnknownElement, SVGElement, nsElementSH, ELEMENT_SCRIPTABLE_FLAGS)

Oh, and of course: DOM_CLASSINFO_MAP_BEGIN(SVGUnknownElement, nsIDOMSVGElement)

(In reply to Boris Zbarsky (:bz) from comment #25) > In nsDOMClassInfo.cpp: > > NS_DEFINE_CLASSINFO_DATA_WITH_NAME(SVGUnknownElement, SVGElement, > nsElementSH, > ELEMENT_SCRIPTABLE_FLAGS) That was what I missed. I was just using NS_DEFINE_CLASSINFO_DATA(SVGUnknownElement, nsElementSH, ELEMENT_SCRIPTABLE_FLAGS) before. Using the _WITH_NAME variant works much better, eliminating both of the issues from comment 24.

Comment on attachment 579705 [details] [diff] [review] Make elements in the SVG namespaces instances of SVGElement, v3 (part 1) r=me. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c73dc22592e https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1506a4b201 Thanks for the patches!

And followups to fix test orange: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d142531a70 (which is totally my fault) and https://hg.mozilla.org/integration/mozilla-inbound/rev/93bcebfb5d40 (need to actually have the element QI to nsIClassInfo).

(In reply to Brian O'Keefe from comment #21) > > This patch does just that. I didn't change all of the checks that > |(nsIContent thing)->GetNameSpaceID() == kNameSpaceID_SVG|, because there > were quite a few. I can do that (as part 3), if you'd like, or file a > follow-up as needed. Thanks for working on this Brian. I'd love to see a follow-up that mass replaces this.

https://hg.mozilla.org/mozilla-central/rev/5c73dc22592e https://hg.mozilla.org/mozilla-central/rev/6c1506a4b201 https://hg.mozilla.org/mozilla-central/rev/f1d142531a70 https://hg.mozilla.org/mozilla-central/rev/93bcebfb5d40

Do we have anybody who can get SVGUnknownElement into a spec? Heycam?

> Do we have anybody who can get SVGUnknownElement into a spec? Why? What would it be useful for?

(In reply to Robert Longson from comment #31) > Thanks for working on this Brian. I'd love to see a follow-up that mass > replaces this. I've filed bug 708846 to take care of that.

(In reply to Boris Zbarsky (:bz) from comment #34) > > Do we have anybody who can get SVGUnknownElement into a spec? > > Why? What would it be useful for? If it's not useful, why are we exposing it to the web unprefixed?

We're not. That's the point of comment 18 through comment 20. The web sees these elements as SVGElement.