Last Comment Bug 589640 - (ietestcenter) HTML5 Foreign Content 14/24: <altGlyphDef> is not an SVGElement
: (ietestcenter) HTML5 Foreign Content 14/24: <altGlyphDef> is not an SVGElement
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Brian O'Keefe [:bokeefe]
:
: Jet Villegas (:jet)
Mentors:
http://samples.msdn.microsoft.com/iet...
Depends on: 740811
Blocks: ietestcenter
  Show dependency treegraph
 
Reported: 2010-08-22 17:38 PDT by Darxus
Modified: 2012-04-06 20:55 PDT (History)
17 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
example with HTML and SVG (431 bytes, text/html; charset=UTF-8)
2011-02-11 21:38 PST, David Baron :dbaron: ⌚️UTC-8
no flags Details
Make elements in the SVG namespaces instances of SVGElement (16.87 KB, patch)
2011-11-23 11:08 PST, Brian O'Keefe [:bokeefe]
bzbarsky: feedback+
Details | Diff | Splinter Review
Make elements in the SVG namespaces instances of SVGElement, v2 (Part 1) (13.23 KB, patch)
2011-12-01 14:38 PST, Brian O'Keefe [:bokeefe]
bzbarsky: feedback+
Details | Diff | Splinter Review
Make nsIContent::IsSVG like IsHTML, and clean up the eSVG node type (part 2) (5.66 KB, patch)
2011-12-01 14:42 PST, Brian O'Keefe [:bokeefe]
bzbarsky: review+
bzbarsky: feedback+
Details | Diff | Splinter Review
Make elements in the SVG namespaces instances of SVGElement, v3 (part 1) (13.41 KB, patch)
2011-12-07 08:50 PST, Brian O'Keefe [:bokeefe]
bzbarsky: review+
Details | Diff | Splinter Review

Description Darxus 2010-08-22 17:38:15 PDT
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 [:bz] (still a bit busy) 2010-08-22 23:46:55 PDT
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) 2011-02-10 17:08:51 PST
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 [:bz] (still a bit busy) 2011-02-10 18:16:36 PST
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) 2011-02-10 18:32:32 PST
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 [:bz] (still a bit busy) 2011-02-10 18:35:57 PST
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 [:bz] (still a bit busy) 2011-02-10 18:38:22 PST
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) 2011-02-10 19:14:42 PST
All with missing .id: altGlyphDef, altGlyphItem, animateColor, glyphRef
Comment 8 jag (Peter Annema) 2011-02-10 19:15:16 PST
s/missing/undefined/
Comment 9 Robert Longson 2011-02-10 23:48:05 PST
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 [:bz] (still a bit busy) 2011-02-11 07:25:01 PST
OK, then do we need to push back on Microsoft about this test and them implementing the element?
Comment 11 Robert Longson 2011-02-11 08:09:53 PST
Unless they really have implemented the elements and their functionality that seems the most sensible thing to do.
Comment 12 David Baron :dbaron: ⌚️UTC-8 2011-02-11 21:34:42 PST
(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-8 2011-02-11 21:38:48 PST
Created attachment 511917 [details]
example with HTML and SVG
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-02-11 21:51:40 PST
> 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-8 2011-02-11 22:56:14 PST
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] 2011-02-21 18:02:46 PST
(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] 2011-02-21 18:05:28 PST
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] 2011-11-23 11:08:18 PST
Created attachment 576552 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-11-23 11:46:14 PST
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] 2011-12-01 14:38:37 PST
Created attachment 578398 [details] [diff] [review]
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] 2011-12-01 14:42:55 PST
Created attachment 578403 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-12-01 14:56:07 PST
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 [:bz] (still a bit busy) 2011-12-01 15:13:42 PST
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] 2011-12-07 06:15:16 PST
(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 [:bz] (still a bit busy) 2011-12-07 06:23:15 PST
> 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 [:bz] (still a bit busy) 2011-12-07 06:24:07 PST
Oh, and of course:

  DOM_CLASSINFO_MAP_BEGIN(SVGUnknownElement, nsIDOMSVGElement)
Comment 27 Brian O'Keefe [:bokeefe] 2011-12-07 08:50:03 PST
Created attachment 579705 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-12-07 12:21:21 PST
Comment on attachment 579705 [details] [diff] [review]
Make elements in the SVG namespaces instances of SVGElement, v3 (part 1)

r=me.  Thanks!
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-12-08 00:33:27 PST
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 2011-12-08 06:00:49 PST
(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 33 :Ms2ger (⌚ UTC+1/+2) 2011-12-08 10:56:45 PST
Do we have anybody who can get SVGUnknownElement into a spec? Heycam?
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2011-12-08 13:08:49 PST
> Do we have anybody who can get SVGUnknownElement into a spec?

Why?  What would it be useful for?
Comment 35 Brian O'Keefe [:bokeefe] 2011-12-08 14:38:54 PST
(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 (⌚ UTC+1/+2) 2011-12-09 05:34:32 PST
(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 [:bz] (still a bit busy) 2011-12-09 10:29:08 PST
We're not.  That's the point of comment 18 through comment 20.  The web sees these elements as SVGElement.

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