589640 - (ietestcenter) HTML5 Foreign Content 14/24: <altGlyphDef> is not an SVGElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Darxus]

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

Fails test.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[jag (Peter Annema)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[jag (Peter Annema)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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,

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[jag (Peter Annema)]

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

Comment 8

[jag (Peter Annema)]

s/missing/undefined/

Comment 9

[Robert Longson [:longsonr]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Robert Longson [:longsonr]]

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: example with HTML and SVG

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[Jonathan Watt [:jwatt]]

(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.

Comment 17

[Jonathan Watt [:jwatt]]

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.

Comment 18

[Brian O'Keefe [:bokeefe]]

Attachment: Make elements in the SVG namespaces instances of SVGElement

(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 19

[Boris Zbarsky [:bzbarsky]]

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....

Comment 20

[Brian O'Keefe [:bokeefe]]

Attachment: Make elements in the SVG namespaces instances of SVGElement, v2 (Part 1)

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.

Comment 21

[Brian O'Keefe [:bokeefe]]

Attachment: Make nsIContent::IsSVG like IsHTML, and clean up the eSVG node type (part 2)

(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 22

[Boris Zbarsky [:bzbarsky]]

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 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Brian O'Keefe [:bokeefe]]

(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?)

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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)

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, and of course:

  DOM_CLASSINFO_MAP_BEGIN(SVGUnknownElement, nsIDOMSVGElement)

Comment 27

[Brian O'Keefe [:bokeefe]]

Attachment: Make elements in the SVG namespaces instances of SVGElement, v3 (part 1)

(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 28

[Boris Zbarsky [:bzbarsky]]

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

r=me.  Thanks!

Comment 29

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c73dc22592e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1506a4b201

Thanks for the patches!

Comment 30

[Boris Zbarsky [:bzbarsky]]

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).

Comment 31

[Robert Longson [:longsonr]]

(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.

Comment 32

[Ed Morley [:emorley]]

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

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Why?  What would it be useful for?

Comment 35

[Brian O'Keefe [:bokeefe]]

(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.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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?

Comment 37

[Boris Zbarsky [:bzbarsky]]

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