237020 - Implement <svg:use> and <svg:symbol> elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Thomas Passin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040205

Mozilla fails to render "use" elements.  IE/Adobe and Batik render them as expected.
The "use" elements references point to elements defined in "defs" elements.

Reproducible: Always
Steps to Reproduce:
1.Load following svg example-

<svg xmlns="http://www.w3.org/2000/svg"
	xmlns:xxlink='http://www.w3.org/1999/xlink'>

<defs>
	<line id='line2' x1='100' y1='100' x2='300' y2='100'
	style='stroke:red;stroke-width:5'/>
</defs>

<line x1='100' y1='100' x2='300' y2='100'
	style='stroke:blue;stroke-width:5'/>

<use xlink:href='#line2' y='30'/>
</svg>
2.
3.

Actual Results:  
Mozilla/SVG renders a single blue horizontal line, which is the line element
that is not referred to by a "use" element.  The expected red line, which is
specified usng a "use" element, is not rendered.

Expected Results:  
A second, red line should have been rendered.  Batik and IE/Adobe do render both
lines as expected.

Comment 1

[Thomas Passin]

Sorry, typo on the xlink namespace - use 

xmlns:xlink='http://www.w3.org/1999/xlink'

instead of the incorrect version in the test case code.

Comment 2

[Jonathan Watt [:jwatt]]

Mass reassign of SVG bugs that aren't currently being worked on by Alex to
[e-mail]. If you think someone should be assigned to your bug you can
join the #svg channel on mozilla.org's IRC server ( irc://irc.mozilla.org/svg )
where you can try to convince one of the SVG hackers to work on it. We aren't
always there, so if you don't get a response straight away please try again later. 

Comment 3

[tor]

Attachment: use/symbol work-in-progress snaphot

Comment 4

[tor]

Attachment: use/symbol work-in-progress snapshot

Changes to the original content are reflected in the <svg:use> - brute force,
but can be improved if it starts being a performance problem.

Cloned content is not anonymous.

Comment 5

[tor]

Attachment: use/symbol anonymous

Comment 6

[tor]

Attachment: changes to shared mozilla files

Comment 7

[tor]

Attachment: changes/new svg-only files

Comment 8

[jonathan chetwynd]

Will mozSVG be supporting <use xlink:href='http://www.peepo.co.uk/res/watch.svg'
y='30'/>

please note this has scripting, which may involve security issues.
so may be <use xlink:href='../res/watch.svg' y='30'/>

However from a copyright perspective it is pretty much essential.
given that resources are likely to include CSS RDF scripting and SVG there
wouldn't appear to be a simple way to separate and identify aspects of code that
have a different copyright licence, apart from the above.

peepo.co.uk is public domain, but we are awaiting a suitable way to include
trademarks, non-public domain creative commons licence, and unattributable
resources.

Comment 9

[jonathan chetwynd]

oops I forgot

of course we also want to <use> schepers superb firefox SVG file as a link

http://svg-whiz.com/svg/firefox-logo.svg to our preferred browser

no script in that one though.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 163252 [details] [diff] [review]
changes to shared mozilla files

r=jwatt, but be aware I don't understand what's going on in
nsCSSFrameConstructor.cpp very well. I'll try and get attachment 163252 [details] [diff] [review]
reviewed tomorrow.

Comment 11

[jonathan chetwynd]

ooops

sorry wrong syntax my comment #8 #9 referred to the <image> element not the
<use> element. will check and if necessary open new bug

apologies

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 163253 [details] [diff] [review]
changes/new svg-only files

r=jwatt with the following issues addressed. Some of these aren't important,
but they're things I would like changed if they're okay with you Tim. Let me
know which changes you won't make (if any).

Can you move the members mHref etc. so they are the last thing (i.e. put them
after LookupHref and TriggerReclone)?

Remove the white space at the beginning of the definition of the
nsSVGUseElement constructor:
  nsSVGUseElement::nsSVGUseElement(nsINodeInfo *aNodeInfo)

At the two lines which consist of:
			 100.0, nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE);
in nsSVGUseElement::Init() change the 100.0 to 100.0f?

In nsSVGUseElement::ParentChainChanged() I would prefer to use "svg_elem" or
something instead of the more ambigous name "dom_elem". 

I would prefer that the definitions of nsSVGUseElement's (and other classes)
member functions come in the same order as they are declared. Especially can
you put DidModifySVGObservable directly after WillModifySVGObservable. 

In nsSVGUseElement::WillModifySVGObservable() perhaps it would be good to have
NS_ASSERTION(s, ...) after the QI? Also if you make LookupHref() returns an
nsresult (see below) we should check it too. 

In nsSVGUseElement::CreateAnonymousContent() use GetCurrentDoc() instead of
GetDocument(), and check if it returns nsnull. Also perhaps have an
NS_ASSERTION(nodeInfoManager, ...) in case NodeInfoManager() returns nsnull? 

For style I prefer ++i over i++, but I guess it doesn't matter for PRUint32.

CreateAnonymousContent() as it stands will allow any element to be referenced,
not just 'svg', 'symbol', 'g', graphics elements or another 'use' element. I
would prefer that you restrict it so that only these elements can be
referenced. I know you're trying to get this patch out of your tree so perhaps
you can leave this bug open (or open another) and fix it up after checking in
the bulk of this, since this restriction isn't critical. 

Can you move the include:
#include "nsIDOMSVGTransformable.h"
from nsSVGGFrame.h back to nsSVGGFrame.cpp please. 

Is there any need to have nsSVGUseFrame inherit nsSVGGFrame? Why not just
inherit nsSVGDefsFrame directly? Or perhaps frames need some renaming and
restructuring anyway. The nsISupports thing seems wierd, not sure what that's
up to. Can you line up the argument names in the declaration of Init please.
I'd prefer "if (it == nsnull)" to "if (nsnull == it)". Should
Init(nsPresContext*  aPresContext,...) check the rv from Init()? In
nsSVGUseFrame::CreateAnonymousContent can you line up the arguments please. 

Again, nothing jumps out at me from nsSVGUseFrame.cpp as being very wrong, but
I'm not very familiar with layout.

Comment 13

[tor]

Attachment: changes/new svg-only files - updated per jwatt's comments

Comment 14

[tor]

(In reply to comment #12)
> For style I prefer ++i over i++, but I guess it doesn't matter for PRUint32.

I think this about the only thing I didn't do.

> Is there any need to have nsSVGUseFrame inherit nsSVGGFrame? Why not just
> inherit nsSVGDefsFrame directly? Or perhaps frames need some renaming and
> restructuring anyway.

I inherited from nsSVGGFrame to get the paint and hit test behavior for free.
Yes, the frame code could probably do with a generic SVG container and leaf
frame that the others inherit from.

Comment 15

[tor]

Comment on attachment 163252 [details] [diff] [review]
changes to shared mozilla files

If you could also glance at nsSVGUseFrame in the other patch in
the bug to back up jwatt, that would be great.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 165959 [details] [diff] [review]
changes/new svg-only files - updated per jwatt's comments

Issues from last review
-----------------------

Can you line up the argument names in the declaration of Init please.
Should Init(nsPresContext*  aPresContext,...) check the rv from Init()?
In nsSVGUseFrame::CreateAnonymousContent can you line up the arguments please. 



nsSVGUseElement::WillModifySVGObservable()
------------------------------------------

Given the change to LookupHref suggested below you can change:
    if (NS_SUCCEEDED(LookupHref(getter_AddRefs(element))) && element) {
to:
    LookupHref(getter_AddRefs(element));
    if (element) {


nsSVGUseElement::CreateAnonymousContent()
-----------------------------------------

Change:
  LookupHref(getter_AddRefs(element));
  if (!element)
    return NS_ERROR_FAILURE;
to:
  nsresult rv = LookupHref(getter_AddRefs(element));
  if (!element)
    return rv;

After:
  // make sure target is valid type for <use>
can you add a comment like:
  // it would be nice if we had an nsSVGGraphicsElement base to QI to


LookupHref()
------------

Set *aResult = nsnull at the beginning.

If we are only going to support local URI references for now then we shouldn't
just ignore anything that comes before the '#' character. The user could be
trying to reference an element in another document, and if an element with the
same id exists in the current document then they will get that one instead. It
seems better to only allow local URI references by checking that the first
character in the href string is '#'. Add something like the following:

  else if (pos > 0) {  // remove this after we support absolute URI references
    NS_ASSERTION(pos > 0, "URI Spec not a local URI reference");
    return NS_ERROR_FAILURE;
  }

Change:
  GetOwnerDocument(getter_AddRefs(document)); add in:
to:
  rv = GetOwnerDocument(getter_AddRefs(document));
  if (!document)
    return rv;

Also:
  nsIDOMSVGElement *svgElement;
  CallQueryInterface(element, &svgElement);
  *aResult = svgElement;
to:
  CallQueryInterface(element, aResult);

Comment 17

[tor]

Attachment: changes/new svg-only files - updated per jwatt's comments

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 166732 [details] [diff] [review]
changes/new svg-only files - updated per jwatt's comments

That looks good to me. BTW, I was asking if  Init(nsPresContext* 
aPresContext,...) should check the rv from Init(), not saying that it
shouldn't. I don't know either way. Maybe checking it as you had is what we
should be doing.

Comment 19

[tor]

Checked in.

Comment 20

[Thomas Passin]

Thanks for your work, guys!

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: make LookupHref warn, not assert

Tim, can you check in this patch if it's okay with you. Because of bug 272416
I'm getting an annoying number of asserts on pages that have a lot of <use>
elements (one for each). Besides that I don't think we should be asserting
here. In fact I don't even think we necessarily want to warn - failure at these
points in the code is down to the author of the SVG document, not us.

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: make LookupHref warn, not assert v2

Further to our discussion on IRC, how about this. I've also just noticed that
in its current state we will never see the "URI Spec not a local URI reference"
assertion.

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: make LookupHref warn, not assert v3

Err, and without the "!".

Comment 24

[tor]

Assertion downgrade checked in.