We need to implement the <view> element. The W3C SVG 1.1 Full testsuite includes tests for this: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-linking-uri-01-b.html http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-linking-uri-02-b.html

Implements most of the fragment identifier part of the SVG specification. bug 391966 is mostly the same as this bug I think, this patch covers much of that issue too anyway. bug 512514 is also fixed in this patch. Still not implemented (could be follow ups perhaps) XPointer - does gecko actually support XPointer any more (https://developer.mozilla.org/en/XML_in_Mozilla#Other_Supported_XML_W3C_Recommendations) highlighting things referenced by viewTarget

(In reply to Robert Longson from comment #3) > bug 512514 is also fixed in this patch. Hi Robert, Thanks very much for looking into this, especially the animation part. I haven't looked at the patch properly yet but on a quick glance it looks like when the target is an animation element this patch just kick-starts that animation. According to SMIL Animation, the behaviour is actually to seek the document. See: http://www.w3.org/TR/smil-animation/#HyperlinkSemantics I'm happy to help with that part of the code if you like. All the work for seeking the document has been implemented (using the behaviour defined in that section) so it should be straightforward.

Yes please, I would like help :-) Could do with some tests too if you have time. I've got a wrapper xhtml file that triggers things currently we could do the same in mochitests to I imagine.

No xpointer per bug 32832

I've had a go at implementing the animation part of this. A few things I've touched along the way: * I got a crash inside SVGFragmentIdentifier::HandleEvent where mSVGSVGElement was null so I added a null-check there. It's probably worth going through the rest of the file to see if there are other situations where that might occur. * I mad the SVGFragmentIdentifier get registered even if it's not an SVG doc since the animation hyperlinking still applies for SVG fragments inside HTML docs. A couple of comments on the underlying patch: * Is there any other way to do this apart from listening for hashchange events? The problem with hashchange events is that: - when script registers for the same event we won't have finished updating the document yet--e.g. in the case of animation, we don't have done a document seek yet - hashchange events seem to be skipped when the hash doesn't change (at least they are skipped when setting window.location.hash). If that's the case, I think it's not right. If you follow a link to a named anchor in an HTML document, scroll a bit, then click the link again, it re-seeks, it doesn't ignore the seemingly redundant change. I think the same should happen here, certainly for animation, probably for views too. i.e. even if the location in the location bar doesn't change, if you click a link to the same location, it should re-update. * There are a number of lines over 80 characters. Thanks again Robert for looking into this!

(In reply to Brian Birtles (:birtles) from comment #7) > * I mad the SVGFragmentIdentifier get registered even if it's not an SVG doc > since the animation hyperlinking still applies for SVG fragments inside HTML > docs. We need to restrict it to the root node though as we don't want fragment hyperlinking on an innner svg node. Have you got a testcase for an svg fragment in an html doc? Looks like we at least want !IsInner() I'll change it to that.

(In reply to Robert Longson from comment #8) > We need to restrict it to the root node though as we don't want fragment > hyperlinking on an innner svg node. Have you got a testcase for an svg > fragment in an html doc? Looks like we at least want !IsInner() I'll change > it to that. The mochitests in that patch operate on an svg fragment in an html doc. I haven't got a test to ensure that svgView is ignored in that case. I just passed the result of IsRoot() into the fragment identifier (since it's private so you can call it from within the fragment identifier). Not sure how that differs from !IsInner(). Presumably it's safe to pass that state in since these things get created/destroyed on bind/unbind and hence won't change during the active life of a fragment identifier?

<svg> <- root = true, inner = false <foreignObject> <html> <svg> <= root = false, inner = false <svg/> <= root = false, inner = true </svg> </html> </foreignObject> </svg>

Oops, the <html> tag shouldn't be there.

I would have thought you'd want IsRoot() in that case, not !IsInner() ? That's what the code I modified used to use, and from skimming the SVG spec it seems to refer to SVG documents not SVG document fragments ? ?

We don't want to treat svgView(whatever) as an id that we'd do a hyperlink animation on I think. I'll fix that logic.

(In reply to Brian Birtles (:birtles) from comment #7) > A couple of comments on the underlying patch: > > * Is there any other way to do this apart from listening for hashchange > events? The problem with hashchange events is that: > - when script registers for the same event we won't have finished updating > the document yet--e.g. in the case of animation, we don't have done a > document seek yet Perhaps we could store a flag when we get the hashchange and then check it on document seek? > - hashchange events seem to be skipped when the hash doesn't change (at > least they are skipped when setting window.location.hash). If that's the > case, I think it's not right. If you follow a link to a named anchor in an > HTML document, scroll a bit, then click the link again, it re-seeks, it > doesn't ignore the seemingly redundant change. I think the same should > happen here, certainly for animation, probably for views too. i.e. even if > the location in the location bar doesn't change, if you click a link to the > same location, it should re-update. I don't know how the There are some other events e.g. pageshow or popstateevent I could try registering for. Looks like popstateevent may be what we want. I don't think html works with these events though. I wonder how it does work?

popstate isn't fired either unless the hash changes. Scrolling seems to take place in nsDocShell::InternalLoad This is where I think it sets the scroll position http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8390 And this same code then later sends the events (or not) that we're tapping into http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8532

bz suggested adding code to PresShell::GoToAnchor to directly handle the change rather than relying on any event sending. I'll try that.

(In reply to Robert Longson from comment #19) > bz suggested adding code to PresShell::GoToAnchor to directly handle the > change rather than relying on any event sending. I'll try that. That sounds promising. Thanks very much for following that up!

Fix some comments that got jumbled.

Comment on attachment 616798 [details] [diff] [review] Part 1 - animation infrastructure by Brian, v1b >diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h >+ * @return the time to seek the documen to in milliseconds or an unresolved >+ * time if there is no resolved interval. >+ */ >+ nsSMILTimeValue GetHyperlinkTime() const; s/documen/document/ Otherwise, looks good!

Comment on attachment 616529 [details] [diff] [review] Part 2 - fragment identifier support by Robert Drive-by whitespace nits from jst-review simulacrum on patch 2 (since I was running it on patches 1 & 3 anyway): >+nsSVGViewBoxRect >+nsSVGSVGElement::GetViewBoxRectWithOverride( [...] >+ return nsSVGViewBoxRect(0, 0, end-of-line whitespace there-------^ >+nsSVGSVGElement::GetPreserveAspectRatioWithOverride() const [...] >+ nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE, and there-------------------------------------------------------------^ >diff --git a/layout/reftests/svg/fragmentIdentifier-01.xhtml b/layout/reftests/svg/fragmentIdentifier-01.xhtml >@@ -0,0 +1,11 @@ >+<html xmlns="http://www.w3.org/1999/xhtml"> >+ <head> >+ <title>Testcases for SVG fragment identifiers</title> and this testcase has tab characters which we should probably swap for spaces.

Comment on attachment 616530 [details] [diff] [review] Part 3 - animation hyperlinking tests by Brian Looks great! Just a few nits: >+++ b/content/smil/test/test_smilHyperlinking.xhtml >+<![CDATA[ >+/** Test for SMIL keySplines **/ s/keySplines/Hyperlinking/ :) >+function continueTest() >+{ [...] >+ window.location.hash = ""; Extreme nit: everywhere else in this file, you use '' (not "") for the empty string. Might as well make this line consistent w/ that. >+// Traversing a hyperlink, condition 1: >+// ^ End-of-line whitespace after the lone "//" there. (This also applies to the lines after "condition 2" and "condition 3" >+// "If the target element is active, seek the document time back to the >+// (current) begin time of the element. If there are multiple begin times, use >+// the begin time that corresponds to the current "begin instance"." nit: redundant quote character at the end of the comment there. >+function forwardToInterval1() { >+ is(gSvg.getCurrentTime(), 2, >+ "Unexpected time after activating link to animation scheduled to start " + >+ "the future"); s/the future/in the future/ >+// Traversing a hyperlink, condition 2: >+// >+// "Else if the target element begin time is resolved (i.e., there is any >+// resolved time in the list of begin times, or if the begin time was forced by >+// an earlier hyperlink or a beginElement() method call It'd probably be good to add a test to be sure we honor "gAnim.beginElement()" calls for this category, too, since that's explicitly mentioned out in the spec text there. e.g. we could call gAnim.setAttribute("begin", "3"), and then seek to t=1 and call beginElement(), and then seek elsewhere and trigger a hashchange. This should take us to t=1 (not 3), I believe. >+ window.setTimeout(fireLinkPart2, 0, callback); [...] >+ window.setTimeout(callback, 0); I believe SimpleTest.executeSoon() is preferred over setTimeout(0) in mochitests (as part of setTimeout-avoidance in general) -- unless that's changed, these lines should probably use be using executeSoon. (You won't be able to pass the bonus 'callback' arg in the former line anymore, but you can just do: SimpleTest.executeSoon(function() { fireLinkPart2(callback); }); ) r=me with that

(In reply to Daniel Holbert [:dholbert] from comment #22) > s/documen/document/ Fixed. Thanks Daniel!

Address review feedback. Thanks Daniel! I find the flow of this test really hard to follow. There might be a better way to manage the async parts using generators and yield (although that's not cross-browser yet). However, I'm going to wait to see how Robert goes with the approach in comment 19. We might be able to get rid of the async parts altogether. In any case, I'll probably update this patch before landing.

This is much simpler now we trigger things directly. I've also addressed the drive by nits.

Reworked test patch to remove async waiting since part 2 no longer relies on events. Daniel, I've carried forward the r=dholbert but if you get a chance, would you mind having a glance at the changes. Thanks!

Comment on attachment 617368 [details] [diff] [review] Part 3 - animation hyperlinking tests by Brian, v1c; r=dholbert Looks great!

(In reply to Daniel Holbert [:dholbert] from comment #29) > Looks great! Thanks Daniel!

nsSVGSVGElement has SVGFragmentIdentifier as a member which is initialised for root elements only and holds parsed data for the fragment. We check SVGFragmentIdentifier to see whether we have an SVGViewSpec but if we don't we may have the id of a view element. In part 2 right now I process the view element lookup in SVGFragmentIdentifier. An alternative would be to only create a SVGFragmentIdentifier if we have a SVGViewSpec and directly lookup the view element in nsSVGSVGElement each time. All the view element processing would move to nsSVGSVGElement so you'd have nsSVGSVGElement::GetViewElement which would look up the view element and then this could get called by the nsSVGSVGElement::GetXXXWithOverride methods. The first way encapsulates all the fragment identifier parsing in one place (SVGFragmentIdentifier) The second saves memory if we don't have an SVGViewSpec syntax fragment.

It seems that Opera and Batik implement the fragmentIdentifier as setting the initial value of the root svg. Patch 2 currently implements a permanent override so you can't animate the viewBox of an SVG document if you supplied a fragment identifier. I'll update patch 2 to fall into line.

Comment on attachment 620273 [details] [diff] [review] Part 2 - updated to match Opera/Batik Review of attachment 620273 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFragmentIdentifier.h @@ +34,5 @@ > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ > + > +#ifndef MOZILLA_FRAGMENTIDENTIFIER_H__ Call this MOZILLA_SVGFRAGMENTIDENTIFIER_H__ @@ +43,5 @@ > +class nsSVGSVGElement; > + > +namespace mozilla { > + > +class SVGFragmentIdentifier Add a Doxygen comment documenting what this class is used for, possibly with a link to a section of the spec. @@ +46,5 @@ > + > +class SVGFragmentIdentifier > +{ > +private: > + SVGFragmentIdentifier(); You should annotate this with MOZ_DELETE, and add a comment "/// To prevent instances of this class ever being created:", or something like that. ::: content/svg/content/src/nsSVGViewElement.h @@ +34,5 @@ > + > +#ifndef __NS_SVGVIEWELEMENT_H__ > +#define __NS_SVGVIEWELEMENT_H__ > + > +#include "nsSVGElement.h" Please sort the includes in strictly case-insensitive alphabetical order. @@ +53,5 @@ > +{ > + friend class nsSVGSVGElement; > +protected: > + friend nsresult NS_NewSVGViewElement(nsIContent **aResult, > + already_AddRefed<nsINodeInfo> aNodeInfo); Why the "protected" here? Just drop that line? Also, where is NS_NewSVGViewElement defined? It doesn't seem to be in this patch. @@ +75,5 @@ > + > + virtual nsXPCClassInfo* GetClassInfo(); > + > + virtual nsIDOMNode* AsDOMNode() { return this; } > +protected: s/protected/private/

(In reply to Jonathan Watt [:jwatt] from comment #36) > Also, where is NS_NewSVGViewElement defined? It doesn't seem to be in this > patch. NS_IMPL_NS_NEW_SVG_ELEMENT in nsSVGElement.h So I get that for free!

the viewelement constructor is private (was protected) to force construction to go through the friend method rather and stop direct construction via new.

Comment on attachment 621283 [details] [diff] [review] address review comment Review of attachment 621283 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Robert Longson from comment #37) > NS_IMPL_NS_NEW_SVG_ELEMENT in nsSVGElement.h > > So I get that for free! Gah, I hate symbol declaring/defining macros that hide where things are declared/defined. Oh well, something to change in a different bug. ::: content/svg/content/src/SVGFragmentIdentifier.cpp @@ +74,5 @@ > + nsAutoString token(tokenizer.nextToken()); > + > + bracketPos = token.FindChar('('); > + if (bracketPos == -1 || token.Length() - bracketPos <= 2) { > + continue; Something like "viewBox()" should be ignored, but if the token is "viewBox(" then it seems like an error. If you think the best course of action for a parse error is to ignore the given SVGViewAttribute, then please at least note that in a comment. Probably you should also add |token.Last() == ')'| to this check too. ::: content/svg/content/src/SVGFragmentIdentifier.h @@ +54,5 @@ > + // To prevent the class being instantiated > + SVGFragmentIdentifier() MOZ_DELETE; > + > +public: > + static void Parse(nsSVGSVGElement* root, const nsAString& ref); Can you call this ParseForElement. It would also seem better to me if the parameter order was switched, since it's the string that is being parsed, and _then_ set on the element. (Perhaps a rather personal taste thing.) Also, please add a Doxygen comment along the lines of: /** * Parse an SVG fragment identifier targeted at the given element, and set the * applicable attributes on the element. */ ::: content/svg/content/src/nsSVGSVGElement.cpp @@ +969,5 @@ > if (viewportWidth <= 0.0f || viewportHeight <= 0.0f) { > return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular > } > > + nsSVGViewBoxRect viewBoxRect = I'd suggest just leaving this as "viewBox", since a viewBox is a rect, and you'd touch fewer lines below. ::: content/svg/content/src/nsSVGSVGElement.h @@ +186,5 @@ > > nsSMILTimeContainer* GetTimedDocumentRoot(); > > + // Process the fragment identifier, if any > + void ProcessFragmentIdentifier(nsIDocument *aDocument, I think this should be a static method on SVGFragmentIdentifier. As well as separating out the whole SVGFragmentIdentifier logic, it allows you to avoid the changes you've made to layout/base/Makefile.in which would be good. @@ +305,5 @@ > // - a (valid or invalid) value for the preserveAspectRatio attribute > // - a SMIL-animated value for the preserveAspectRatio attribute > bool HasPreserveAspectRatio(); > > + nsSVGViewBoxRect GetViewBoxRectWithOverride( This name is misleading, since this method doesn't involve any overriding of an explicit viewBox. GetViewBoxOrSynthesizedViewBox or GetViewBoxWithSynthesis would be a better name. Also please add a comment along the lines of: /** * Returns the explicit viewBox rect, if specified, or else a synthesized * viewBox, if appropriate, or else a viewBox matching the dimensions of the * SVG viewport. */ @@ +307,5 @@ > bool HasPreserveAspectRatio(); > > + nsSVGViewBoxRect GetViewBoxRectWithOverride( > + float aViewportWidth, float aViewportHeight) const; > + SVGPreserveAspectRatio GetPreserveAspectRatioWithOverride() const; Add something like: /** * Returns the explicit or default preserveAspectRatio, unless we're * synthesizing a viewBox, in which case it returns the "none" value. */ @@ +308,5 @@ > > + nsSVGViewBoxRect GetViewBoxRectWithOverride( > + float aViewportWidth, float aViewportHeight) const; > + SVGPreserveAspectRatio GetPreserveAspectRatioWithOverride() const; > + nsSVGViewElement* GetViewElement(const nsAString& aId) const; I think this should be a static method on SVGFragmentIdentifier. ::: content/svg/content/src/nsSVGViewElement.h @@ +52,5 @@ > + public nsIDOMSVGZoomAndPan > +{ > + friend class nsSVGSVGElement; > +private: > + friend nsresult NS_NewSVGViewElement(nsIContent **aResult, You don't need this 'private' here, since that's the default for classes.

(In reply to Jonathan Watt [:jwatt] from comment #39) > Something like "viewBox()" should be ignored, but if the token is "viewBox(" > then it seems like an error. If you think the best course of action for a > parse error is to ignore the given SVGViewAttribute, then please at least > note that in a comment. > > Probably you should also add |token.Last() == ')'| to this check too. I check for that already in IsMatchingParameter. I've added some comments to make things clearer

Comment on attachment 622398 [details] [diff] [review] address review comments Review of attachment 622398 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGAnimatedPreserveAspectRatio.h @@ +121,5 @@ > mIsAnimated = false; > mIsBaseSet = false; > } > > + SVGAnimatedPreserveAspectRatio& operator=(const SVGAnimatedPreserveAspectRatio& aOther) { I'm missing why we need this. At any rate, I'm not keen on setting the animVal members. ::: content/svg/content/src/SVGFragmentIdentifier.cpp @@ +65,5 @@ > + static_cast<nsSVGViewElement*>(element) : nsnull; > +} > + > +void > +SVGFragmentIdentifier::ParseForElement(const nsAString& aRef, nsSVGSVGElement* root) I think we should be passing in a document argument instead of an element argument here (see comments at function declaration). For the purposes of fixing this bug, I'd be quite happy for you to then make this function do nothing if 'viewTarget' is specified, and file a follow-up on supporting 'viewTarget'. However, if 'viewTarget' is specified, then this function really should not touch the root element. So I think you should store the values of the other params until you have finished processing them all and know that 'viewTarget' is not specified. @@ +95,5 @@ > + // Each valid SVGViewAttribute is of the form attribute(<params>) > + // IsMatchingParameter checks that the name and brackets are present in the right places > + > + if (!hasViewBox && > + IsMatchingParameter(token, NS_LITERAL_STRING("viewBox"))) { Rather than accepting the first specified parameter, it would seem to make more sense to me to allow subsequent parameters to override former. What do other implementations do (maybe add a comment noting what other implementations do)? @@ +129,5 @@ > + const nsAString& aAnchorName) > +{ > +#ifdef DEBUG > + nsCOMPtr<nsIDOMSVGDocument> svgDoc = do_QueryInterface(aDocument); > + NS_ABORT_IF_FALSE(svgDoc, "expecting an SVG Document"); If the doc is sent as text/xml or something other than image/svg+xml, this will fail. The abort should be based on whether the root element is an nsSVGSVGElement rather than the type of the doc. @@ +138,5 @@ > + > + const nsSVGViewElement* viewElement = GetViewElement(aDocument, aAnchorName); > + > + if (viewElement) { > + rootElement->mViewBox = viewElement->mViewBox; Rather than rootElement, shouldn't we be using the element specified by the 'view' element's 'viewTarget' attribute? @@ +145,5 @@ > + rootElement->mPreserveAspectRatio = viewElement->mPreserveAspectRatio; > + } else { > + ParseForElement(aAnchorName, rootElement); > + } > + rootElement->InvalidateTransformNotifyFrame(); Can you avoid calling this? Perhaps by using SetAttr in SVGFragmentIdentifier::ParseForElement? My work in bug 734082 depends on keeping InvalidateTransformNotifyFrame/NotifyViewportOrTransformChanged as methods used only for non-attribute changes in order to create/destroy layers at the right times. ::: content/svg/content/src/SVGFragmentIdentifier.h @@ +57,5 @@ > + SVGFragmentIdentifier() MOZ_DELETE; > + > +public: > + /** > + * Process the SVG fragment identifier, if there is one. Probably expand on this comment too since it's the main interface. Perhaps "Process the SVG fragment identifier (if there is one) and set the appropriate attributes on the target element." ? @@ +67,5 @@ > + /** > + * Parse an SVG fragment identifier targeted at the given element, and set the > + * applicable attributes on the element. > + */ > + static void ParseForElement(const nsAString& aRef, nsSVGSVGElement* root); Actually, seeing the code as it is now, this would probably be more appropriately named "ProcessSVGViewSpec", since that's what it does. Also the 'viewTarget' can be specified in the SVGViewSpec, so passing in the root element seems wrong. It seems like the document should be passed in, and if the SVGViewSpec doesn't specify the viewTarget, then get the root element and use that. In light of that, and assuming you change the code to do that, a better comment would be "Parse the given SVGViewSpec and set the applicable attributes on the target 'svg' element". Also aRef is a pretty undescriptive name, so maybe rename it to aSVGViewSpec? ::: content/svg/content/src/nsSVGViewBox.h @@ +73,5 @@ > > + nsSVGViewBox& operator=(const nsSVGViewBox& aOther) { > + mBaseVal = aOther.mBaseVal; > + mAnimVal = aOther.mAnimVal; > + mHasBaseVal = aOther.mHasBaseVal; I don't think we should set mAnimVal here. ::: layout/base/nsPresShell.cpp @@ +2968,5 @@ > } > + > + // If the target is an animation element, activate the animation > + if (content->IsNodeOfType(nsINode::eANIMATION)) { > + static_cast<nsSVGAnimationElement*>(content.get())->ActivateByHyperlink(); If using nsSVGAnimationElement requires you to add content/smil to the include paths for layout/base, then the layout guys will likely prefer you to expose a utility method for this. (At least bz got me to do that recently.) Perhaps add something to nsSVGUtils as a shim if you can't come up with something better. Or maybe handle this in SVGFragmentIdentifier::ProcessFragmentIdentifier?

(In reply to Jonathan Watt [:jwatt] from comment #42) > > I'm missing why we need this. At any rate, I'm not keen on setting the > animVal members. OK I've changed this to call SetBaseValue. > > I think we should be passing in a document argument instead of an element > argument here (see comments at function declaration). > > For the purposes of fixing this bug, I'd be quite happy for you to then make > this function do nothing if 'viewTarget' is specified, and file a follow-up > on supporting 'viewTarget'. However, if 'viewTarget' is specified, then this > function really should not touch the root element. So I think you should > store the values of the other params until you have finished processing them > all and know that 'viewTarget' is not specified. Is far as i can tell viewTarget is only about highlighting specific elements. So I'd get the list and call some kind of "highlight yourself" method on each in theory. Opera doesn't support it and the documentation is pretty limited so it's hard to tell what it should do. I've not changed the code as if we do implement this and need to change things we can always do so at that point. > Rather than accepting the first specified parameter, it would seem to make > more sense to me to allow subsequent parameters to override former. What do > other implementations do (maybe add a comment noting what other > implementations do)? Opera ignores the fragment identifier if it is like this. I've changed the patch to follow suit. > If the doc is sent as text/xml or something other than image/svg+xml, this > will fail. The abort should be based on whether the root element is an > nsSVGSVGElement rather than the type of the doc. Done. > > Rather than rootElement, shouldn't we be using the element specified by the > 'view' element's 'viewTarget' attribute? I don't think so and anyway I'm not supporting viewTarget at all. > > Can you avoid calling this? Perhaps by using SetAttr in > SVGFragmentIdentifier::ParseForElement? My work in bug 734082 depends on > keeping InvalidateTransformNotifyFrame/NotifyViewportOrTransformChanged as > methods used only for non-attribute changes in order to create/destroy > layers at the right times. I've removed the call. But... If I go from a URL of the form xxxx#viewSpec() to just xxx e.g. by using the back button then as there's nothing to set the document is left displaying the viewSpec. Same problem happens if it goes from valid to invalid. How do you want me to fix that? What if I need to trigger a repaint with a recalculation of frames cached canvasTM and there's nothing to set? > Actually, seeing the code as it is now, this would probably be more > appropriately named "ProcessSVGViewSpec", since that's what it does. Also > the 'viewTarget' can be specified in the SVGViewSpec, so passing in the root > element seems wrong. It seems like the document should be passed in, and if > the SVGViewSpec doesn't specify the viewTarget, then get the root element > and use that. > > In light of that, and assuming you change the code to do that, a better > comment would be "Parse the given SVGViewSpec and set the applicable > attributes on the target 'svg' element". > > Also aRef is a pretty undescriptive name, so maybe rename it to aSVGViewSpec? > Done. > > If using nsSVGAnimationElement requires you to add content/smil to the > include paths for layout/base, then the layout guys will likely prefer you > to expose a utility method for this. (At least bz got me to do that > recently.) Perhaps add something to nsSVGUtils as a shim if you can't come > up with something better. Or maybe handle this in > SVGFragmentIdentifier::ProcessFragmentIdentifier? Added to nsSVGUtils as it's not really a fragment identifier.

Comment on attachment 622697 [details] [diff] [review] updated patch Review of attachment 622697 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Robert Longson from comment #43) > If I go from a URL of the form xxxx#viewSpec() to just xxx e.g. by using the > back button then as there's nothing to set the document is left displaying > the viewSpec. Same problem happens if it goes from valid to invalid. > > How do you want me to fix that? What if I need to trigger a repaint with a > recalculation of frames cached canvasTM and there's nothing to set? How about in SVGFragmentIdentifier::ProcessSVGViewSpec you allocate an SVGViewBox and/or SVGPreserveAspectRatio object and store the object(s) using frame properties if the spec causes us to overwrite them. If there is an error in future, then we can restore the original values. I think that's probably good enough. ::: content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp @@ +152,5 @@ > +bool > +SVGPreserveAspectRatio::operator==(const SVGPreserveAspectRatio& aOther) const > +{ > + if (&aOther == this) > + return true; Just in passing, is this optimization worth it? (Do we expect to hit it?) ::: content/svg/content/src/SVGAnimatedPreserveAspectRatio.h @@ +132,5 @@ > + nsSVGElement *aSVGElement); > + nsresult SetBaseAlign(PRUint16 aAlign, nsSVGElement *aSVGElement) { > + if (aAlign < nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE || > + aAlign > nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_XMAXYMAX) > + return NS_ERROR_FAILURE; Can you use curly brackets around the body of this |if|, please? @@ +141,5 @@ > + } > + nsresult SetBaseMeetOrSlice(PRUint16 aMeetOrSlice, nsSVGElement *aSVGElement) { > + if (aMeetOrSlice < nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET || > + aMeetOrSlice > nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE) > + return NS_ERROR_FAILURE; And this one. ::: content/svg/content/src/SVGFragmentIdentifier.cpp @@ +66,5 @@ > +} > + > +void > +SVGFragmentIdentifier::ProcessSVGViewSpec( > + const nsAString &aViewSpec, nsSVGSVGElement *root) IMO this would be easier on the eyes as: SVGFragmentIdentifier::ProcessSVGViewSpec(const nsAString &aViewSpec, nsSVGSVGElement *root) @@ +91,5 @@ > + > + nsAutoString token(tokenizer.nextToken()); > + > + bracketPos = token.FindChar('('); > + if (bracketPos == -1 || token.Length() - bracketPos <= 2 || I think it would be better to change |bracketPos == -1| to |bracketPos < 1| since there should be at least one non-'(' character before the '('. Also, it seems like you can drop the |token.Length() - bracketPos <= 2| since the |token.Last() != ')'| check covers that. @@ +117,5 @@ > + if (zoomAndPanParams) { > + return; > + } > + zoomAndPanParams = &params; > + } I think you should chain the above |if| statements together using |else if|. And I think you should tag on a trailing |else| here that just returns if we get something unexpected or invalid. That would cover 'viewTarget' which as we've noted is massively underspecified, and discourage people from using it and coming to rely on behavior that the WG would like to change. It would also cover unknown things like "foo(bar)" and invalid things like "ao)eu(aoue)". ::: layout/svg/base/src/nsSVGUtils.h @@ +65,5 @@ > class nsPresContext; > class nsRenderingContext; > class nsStyleContext; > class nsStyleCoord; > +class nsSVGAnimationElement; Please remove this - the header doesn't need it as-is. @@ +238,5 @@ > > /* > + * Activate the animation by hyperlink > + */ > + static void ActivateByHyperlink(nsIContent *aContent); Can you make this a proper Doxygen style comment and expand it to provide more of the non-obvious info? Perhaps: /** * Activates the animation element aContent as a result of navigation to the * fragment identifier that identifies aContent. aContent must be an instance * of nsSVGAnimationElement. * * This is just a shim to allow nsSVGAnimationElement::ActivateByHyperlink to * be called from layout/base without adding to that directory's include paths. */

Also I think 'view' elements with a 'viewTarget' attribute should be ignored for now until we can get the WG to sort out what it's actually supposed to do. For what it's worth my previous understanding was that it allowed an element other than the root element to have its 'viewBox' etc. overwritten.

Comment on attachment 623127 [details] [diff] [review] Part 2 - updated again Review of attachment 623127 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/nsSVGViewBox.h @@ +91,2 @@ > void SetBaseValue(float aX, float aY, float aWidth, float aHeight, > + nsSVGElement *aSVGElement) { Can you line this parameter up with the opening parenthesis please? ::: layout/base/nsPresShell.cpp @@ +2829,5 @@ > } > > + nsCOMPtr<nsIDOMHTMLDocument> htmlDoc = do_QueryInterface(mDocument); > + > + if (mDocument->GetRootElement()->IsSVG(nsGkAtoms::svg) && !htmlDoc) { Can you add a comment here noting that you need to do this before the |aAnchorName.IsEmpty()| check, and why? I think probably we shouldn't bother with the |htmlDoc| check here. I think also that if SVGFragmentIdentifier::ProcessFragmentIdentifier handles this "link" change, then we should return here and not proceed with the other handling below. There should only really be one right action here, and doing the "view" handling thing, plus the HTML scrolling thing, for example, seems wrong.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e288dfb36d73 https://hg.mozilla.org/integration/mozilla-inbound/rev/3597432da4d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d51f702f5cbe

http://hg.mozilla.org/mozilla-central/rev/e288dfb36d73 http://hg.mozilla.org/mozilla-central/rev/3597432da4d9 http://hg.mozilla.org/mozilla-central/rev/d51f702f5cbe

Comment on attachment 623127 [details] [diff] [review] Part 2 - updated again Review of attachment 623127 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFragmentIdentifier.cpp @@ +52,5 @@ > +static bool > +IsMatchingParameter(const nsAString &aString, const nsAString &aParameterName) > +{ > + return StringBeginsWith(aString, aParameterName) && > + aString.CharAt(aParameterName.Length()) == '(' && Do explain why this is correct if aString == aParameterName ::: xpcom/ds/Makefile.in @@ +109,5 @@ > nsStringEnumerator.h \ > nsHashPropertyBag.h \ > nsWhitespaceTokenizer.h \ > nsCharSeparatedTokenizer.h \ > + CharTokenizer.h \ Revert this. CharTokenizer.h is included as "mozilla/CharTokenizer.h".

Do raise a follow up and I'll fix things there, I'm not going to back all this out.

I've finally documented the support at https://developer.mozilla.org/en-US/docs/Web/SVG/Element/view. Sebastian

Thanks, sebo!