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

RESOLVED FIXED

Status

()

Core
SVG
--
major
RESOLVED FIXED
14 years ago
a year ago

People

(Reporter: Thomas Passin, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: SVG: "use" element inoperative → Implement "use" element
Mass reassign of SVG bugs that aren't currently being worked on by Alex to
general@svg.bugs. 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. 
Assignee: alex → general
(Assignee)

Comment 3

13 years ago
Created attachment 161774 [details] [diff] [review]
use/symbol work-in-progress snaphot
(Assignee)

Updated

13 years ago
OS: Windows 2000 → All
Hardware: PC → All
Summary: Implement "use" element → Implement <svg:use> and <svg:symbol> elements
(Assignee)

Comment 4

13 years ago
Created attachment 162588 [details] [diff] [review]
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.
Attachment #161774 - Attachment is obsolete: true
(Assignee)

Comment 5

13 years ago
Created attachment 163072 [details] [diff] [review]
use/symbol anonymous
Assignee: general → tor
Attachment #162588 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Blocks: 265895
(Assignee)

Updated

13 years ago
Blocks: 265894
(Assignee)

Updated

13 years ago
Attachment #163072 - Attachment is obsolete: true
(Assignee)

Comment 6

13 years ago
Created attachment 163252 [details] [diff] [review]
changes to shared mozilla files
(Assignee)

Comment 7

13 years ago
Created attachment 163253 [details] [diff] [review]
changes/new svg-only files
(Assignee)

Updated

13 years ago
Attachment #163252 - Flags: review?(jonathan.watt)
(Assignee)

Updated

13 years ago
Attachment #163253 - Flags: review?(jonathan.watt)

Comment 8

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

13 years ago
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 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.
Attachment #163252 - Flags: review?(jonathan.watt) → review+

Comment 11

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

Updated

13 years ago
Blocks: 269482
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.
Attachment #163253 - Flags: review?(jonathan.watt) → review-
(Assignee)

Comment 13

13 years ago
Created attachment 165959 [details] [diff] [review]
changes/new svg-only files - updated per jwatt's comments
Attachment #163253 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Updated

13 years ago
Attachment #165959 - Flags: review?(jonathan.watt)
(Assignee)

Comment 15

13 years ago
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.
Attachment #163252 - Flags: superreview?(roc)
Attachment #163252 - Flags: superreview?(roc) → superreview+
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);
Attachment #165959 - Flags: review?(jonathan.watt) → review-
(Assignee)

Comment 17

13 years ago
Created attachment 166732 [details] [diff] [review]
changes/new svg-only files - updated per jwatt's comments
Attachment #165959 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #166732 - Flags: review?(jonathan.watt)
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.
Attachment #166732 - Flags: review?(jonathan.watt) → review+
(Assignee)

Comment 19

13 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 20

13 years ago
Thanks for your work, guys!
Created attachment 167490 [details] [diff] [review]
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.
Created attachment 167594 [details] [diff] [review]
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.
Attachment #167490 - Attachment is obsolete: true
Created attachment 167595 [details] [diff] [review]
make LookupHref warn, not assert v3

Err, and without the "!".
Attachment #167594 - Attachment is obsolete: true
(Assignee)

Comment 24

13 years ago
Assertion downgrade checked in.
Depends on: 1223645, 1268431
No longer blocks: 265894
Depends on: 265894
You need to log in before you can comment on or make changes to this bug.