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.

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.

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.

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.

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.

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.

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

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

Checked in.

Thanks for your work, guys!

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.

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.

Err, and without the "!".

Assertion downgrade checked in.