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

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Darxus, Assigned: bokeefe)

Tracking

({dev-doc-needed})

unspecified
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Fails test.

Reproducible: Always
(Reporter)

Updated

7 years ago
Blocks: 554013
Version: unspecified → Trunk

Comment 1

7 years ago
Bogus test assumes IE's broken behavior wrt text nodes in the DOM (in particular  removing-them-but-not-really in various cases).
Assignee: nobody → english-us
Status: UNCONFIRMED → NEW
Component: General → English US
Ever confirmed: true
Product: Core → Tech Evangelism
QA Contact: general → english-us
Version: Trunk → unspecified

Comment 2

7 years ago
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

7 years ago
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

7 years ago
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

7 years ago
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,
Assignee: english-us → nobody
Component: English US → SVG
Product: Tech Evangelism → Core
QA Contact: english-us → general

Comment 6

7 years ago
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?

Updated

7 years ago
Summary: (ietestcenter) HTML5 Foreign Content 14/24: Test passes if the word "PASS" appears below → (ietestcenter) HTML5 Foreign Content 14/24: <altGlyphDef> is not an SVGElement

Comment 7

7 years ago
All with missing .id: altGlyphDef, altGlyphItem, animateColor, glyphRef

Comment 8

7 years ago
s/missing/undefined/

Comment 9

7 years ago
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

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

Comment 11

7 years ago
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?
Created attachment 511917 [details]
example with HTML and SVG

Comment 14

7 years ago
> 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.
(Assignee)

Comment 18

6 years ago
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.
Attachment #576552 - Flags: feedback?(jwatt)
Attachment #576552 - Flags: feedback?(bzbarsky)

Comment 19

6 years ago
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....
Attachment #576552 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 20

6 years ago
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.
Attachment #576552 - Attachment is obsolete: true
Attachment #578398 - Flags: feedback?(bzbarsky)
Attachment #576552 - Flags: feedback?(jwatt)
(Assignee)

Comment 21

6 years ago
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.
Attachment #578403 - Flags: feedback?(bzbarsky)

Comment 22

6 years ago
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.
Attachment #578398 - Flags: feedback?(bzbarsky) → feedback+

Comment 23

6 years ago
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.
Attachment #578403 - Flags: review+
Attachment #578403 - Flags: feedback?(bzbarsky)
Attachment #578403 - Flags: feedback+

Updated

6 years ago
Assignee: nobody → bokeefe
(Assignee)

Comment 24

6 years ago
(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?)
Status: NEW → ASSIGNED

Comment 25

6 years ago
> 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

6 years ago
Oh, and of course:

  DOM_CLASSINFO_MAP_BEGIN(SVGUnknownElement, nsIDOMSVGElement)
(Assignee)

Comment 27

6 years ago
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.
Attachment #578398 - Attachment is obsolete: true
Attachment #579705 - Flags: review?(bzbarsky)

Comment 28

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

r=me.  Thanks!
Attachment #579705 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All

Comment 29

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

Thanks for the patches!
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla11

Comment 30

6 years ago
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

6 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: dev-doc-needed
Do we have anybody who can get SVGUnknownElement into a spec? Heycam?
Keywords: dev-doc-needed
Whiteboard: dev-doc-needed

Comment 34

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

Why?  What would it be useful for?
(Assignee)

Comment 35

6 years ago
(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?

Comment 37

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

Updated

5 years ago
Depends on: 740811
You need to log in before you can comment on or make changes to this bug.