Closed Bug 272288 Opened 20 years ago Closed 13 years ago

Allow SVG source for <svg:image>

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- -

People

(Reporter: tor, Assigned: dholbert)

References

(Blocks 3 open bugs, )

Details

Attachments

(8 files, 11 obsolete files)

1.11 KB, image/svg+xml
Details
1.63 KB, image/svg+xml
Details
69.89 KB, image/png
Details
20.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.80 KB, patch
longsonr
: review+
Details | Diff | Splinter Review
888 bytes, patch
roc
: review+
Details | Diff | Splinter Review
23.30 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
 
Is there a reason this should be specific to <svg:image>?  We should be able to
allow this for <img> nodes too, no?
There's another bug about using SVG for <img> and css properties, bug 276431.

One thing I'm not sure about which would greatly impact how both of these
problems is addressed is whether the svg should be "live" or a snapshot.
The SVG 1.1 spec seems a bit ambiguous for svg:image - the statement
"The result of processing an 'image' is always a four-channel RGBA result."
could be interpreted both ways.

Likewise the spec language for <img>/css-properties is not explicit - "If
the user agent includes an HTML or XHTML viewing capability or can apply
CSS/XSL styling properties to XML documents, then a Conforming SVG Viewer
must support resources of MIME type "image/svg+xml" wherever raster image
external resources can be used, such as in the HTML or XHTML 'img' element
and in CSS/XSL properties that can refer to raster image resources (e.g.,
'background-image')."
Well... if we can make SVG support the image apis imagelib wants out of images,
we can get svg images for free anywhere we use images now, no?  I'm not sure how
hard this would be to do, though (in a performant way).
Depends on: 276431
I'm worried that allowing HTML <img> to reference SVGs could lead to security
holes because SVGs can contain scripts.  Many message board allow users to
include <img> tags in their posts referencing external images, and depending on
how this is implemented, scripts in SVGs referenced in that way might run.
There has been talk of doing non-scripting SVG only for such cases (that is,
allowing declarative animation, but not scripted stuff).
It is essential for SVG based applications that <svg:image> can include SVG
source, but it is also necessary that scripts works across the included SVGs and
the outer SVG just as it now works between HTML/XML and an included SVG in an
<object>.
(In reply to comment #6)
> It is essential for SVG based applications that <svg:image> can include SVG
> source, but it is also necessary that scripts works across the included SVGs and
> the outer SVG just as it now works between HTML/XML and an included SVG in an
> <object>.
I can do this now by loading the file via XMLHttpRequest() and inserting into the DOM.  This may be the simplest way to implement this since the overhead required to create a RGBA image would be more than just loading the file into the DOM.
That would not be correct. You do not want to modify the DOM and effectively merge the two.

What I guess is needed is a definition of "SVG as image". For this as well as for html:img, 'background-image' and 'list-style-image' and any other properties/elements that can load images.
tor, have you posed the question to the SVG WG for svg:image?

For html:img (Bug 276431) and 'background-image' (Bug 231179), from the perspective of the specs, things are not fully clarified either.  Anne, I believe the CDF Working Group is working out some of these issues (event propagation) but you are largely focusing on html:object and svg:foreignObject, correct?

For the security issues, I would say the browser should support the SVG format.  I feel it's the responsibility of the message board software not the browser to either not support SVG uploads or properly scrub any SVG images submitted for scripts.  
There are some bits in the WICD profile but they are underdefined at the moment. I'm trying to get that improved before last call and hopefully get some feedback from implementors on it.
OK, I agree that Bug 409988 is a duplicate, but has there been any progress on this bug in the 4 years since it was first reported?   

This feature could be very useful for graphviz using applications like Doxygen or SchemaSpy
parity Opera
parity Safari
http://files.myopera.com/MacDev_ed/sarpsborg2007/external-use.svg

does this bug need a testcase or description?
This feature will be very usefull, first of all, for web sites that are XHTML plus SVG (or other combinations of SVG and web languages). I, for example, would like to use SVG as an <svg:image> in some website, but I can't, as this is not supported by Firefox (the browser with the biggest "market share" from all web browsers that natively support SVG).

So please, implement it.

(it is very difficult to avoid using proprietary web technologies, when the free and open alternatives aren't well supported by the most important user agents, and as you may already know, Opera 9.5 beta supports the feature in question - now it's time for Firefox).
I have written a generic svg application to enable pan and zoom in any image. It uses <svg:image> to load the image to work on. I wrote it primarily for other svg images, and was surprised when Firefox did not support this. It works fine with png images and as mentioned above, Opera 9.5 can handle svg images as well.

Test with: http://kornet1.se/svg/zoomframe.html

Blocks: 455986
(In reply to comment #14)

> ...Opera 9.5 beta supports the feature in
> question - now it's time for Firefox).

BTW: SVG is also very good supported in Safari 3 today... and next example works too...
As for Opera, it supports (at least SMIL) animation in SVG images included using <svg:image>. But this is only true when the topmost SVG image is embeded into HTML using <object> (or if it's just inline SVG), and not when using <img>.
Flags: blocking1.9.2+
Priority: -- → P2
Assignee: general → nobody
QA Contact: ian → general
Assignee: nobody → roc
Target Milestone: --- → mozilla1.9.2a1
Correction of my previous comment: Opera (9.5, 9.6) supports SMIL animation in inner SVG images also if the outer SVG image is embedded into HTML using <img> tag.
Attached image test case
REF testcase works good with Opera 9, Safari 4 and ASV 3
Not making it.
Flags: blocking1.9.2+ → blocking1.9.2-
blocking2.0: --- → ?
Would it be possible to know, roughly, what prevented this bug, together with Bug 231179 and Bug 276431, to make it in 1.9.2, as we were expecting it so much, and when would its new estimated time of implementation be, if it's already known?
 
Thank you!
Woot, so Bug 231179 has landed, getting us most of the way there for this bug. 

Per Bug 231179 comment 159, <svg:image> doesn't quite work yet, though -- it still requires an implementation of imgIContainer::CopyFrame for VectorImage. (and some tweaks in the existing <svg:image> code to fix assumptions about images always having pixel-valued heights/widths).

Longsonr reminds me that we should set <svg:image> as a supported feature once this is fixed, too. ( http://www.w3.org/TR/SVG/feature.html )
er, sorry, I meant "bug 276431" &  "bug 276431 comment 159" in previous comment
(In reply to comment #25)
> Per Bug 231179 comment 159, <svg:image> doesn't quite work yet, though -- it
> still requires an implementation of imgIContainer::CopyFrame for VectorImage.
> (and some tweaks in the existing <svg:image> code to fix assumptions about
> images always having pixel-valued heights/widths).

I think we should just change nsSVGImageFrame to call Draw() instead.
Yeah, that sounds better.  Patches coming up soon.
This patch makes nsSVGImageFrame::PaintSVG use nsLayoutUtils::DrawSingleImage,
instead of imgIContainer::GetFrame.  I haven't tested it thoroughly, but it
works on simple testcases with both PNG and SVG images.  I'm running it through
TryServer to see if it breaks anything.
This second patch isn't needed for painting, but it is needed for hit-testing. 
It fixes up GetImageTransform, whose only client is nsSVGImageFrame::GetFrameForPoint. (Patch 1 removes the only other client.)

Without this patch, nsSVGImageFrame::GetImageTransform assumes that our image has a pixel-valued width & height, but that's of course not always true for SVG images.

Conveniently, nsSVGImageFrame has a fixed size, and its VectorImage will adapt to fit that size. So we can just use the nsSVGImageFrame's own size as the image-size, for Vector-type images.
Regarding comment 29 you seem to have removed the clip code so does and image with a clip attribute set to something other than auto still work? There are no image clip reftests but there rect clip reftests. See bug 481614. Note that as we do CSS2.1 clipping rather than CSS 2.0 clipping as the SVG spec mandates we still don't pass the SVG testsuite test mentioned in that bug as we clip the image to nothing.
Also do you need to do something for feImage in nsSVGFilters.cpp or does that already work?
What happened to opacity support too?
(In reply to comment #33)
> What happened to opacity support too?

Yup, I have opacity working locally.

I'm looking into getting transforms on the image working right now -- those are missing from the first-pass-patch, too.  (Transforms will actually keep us from using the DrawSingleImage wrapper for "Draw", I think, since you can't pass a transform through that.  Oh well.)  Then I'll look into clipping & feImage.
Depends on: 598798
Comment on attachment 473830 [details] [diff] [review]
Patch 1 v1: Use nsLayoutUtils::DrawSingleImage

(In reply to comment #34)
> (Transforms will actually keep us from
> using the DrawSingleImage wrapper for "Draw", I think, since you can't pass a
> transform through that.  Oh well.)

Sorry, ignore that part of my last comment -- the transform can just go in the gfxContext, so that's not an issue.

I've now split off patch 1 into a helper-bug: bug 598798.
Attachment #473830 - Attachment is obsolete: true
So with bug 598798, this is now mostly trivial, with two non-trivial exceptions from http://www.w3.org/TR/SVG11/struct.html#ImageElement :

 (A) If the <image> element happens to set "overflow:visible", then we aren't supposed to clip the image at its viewport.  That is to say: the image's "viewport" is the <image> element, but we're supposed to paint anything outside the viewport that'd still show up on-screen, too.

 (B) We also need to tell the image-helper-document's root <svg> element to disregard its preserveAspectRatio, clip, and 'overflow' properties, since the <image> element will apply its own versions of those properties.

I think (A) is something that can happen in another bug, and if that doesn't make this release, that's not a big deal, since it'd just be a case of us clipping at an <image> boundary when we're not supposed to. (I also doubt <image overflow="visible"> is a frequent use-case...)

(B) is something that we need to get right in this bug, though (at least for preserveAspectRatio), since it affects the scale & positioning of SVG content within an <image> element.  It's not immediately obvious what the best way to accomplish it is, though, since the helper SVG document is obscured behind the imgIContainer interface.

Perhaps we could use a flag in the imgIContainer::Draw "aFlags" parameter, like FLAG_IN_SVG_IMAGE, to communicate "this image is currently being drawn for an <svg:image> element (so ignore your preserveAspectRatio/clip/overflow properties for this Draw() operation)
B rather depends on whether PreserveAspectRatio is defer or not doesn't it?
Ah, indeed -- if we have
 <image preserveAspectRatio="defer" xlink:href="foo.svg">
then we need to honor foo.svg's preserveAspectRatio after all, per
http://www.w3.org/TR/SVG11/coords.html#PreserveAspectRatioAttribute

(I wish that were also mentioned in the section of spec I'd linked to in comment 36...  That section's prose says "When an ‘image’ element references an SVG image the ‘preserveAspectRatio’ attribute as well as the clip and overflow properties on the root element in the referenced SVG image are ignored".  It makes no mention of the "defer" possibility.)
Target Milestone: mozilla1.9.2a1 → ---
(In reply to comment #38)
> (I wish that were also mentioned in the section of spec I'd linked to in
> comment 36...

(I brought this up on www-svg, FWIW, and it's been fixed in the editor's draft:
http://lists.w3.org/Archives/Public/www-svg/2010Oct/0047.html
http://lists.w3.org/Archives/Public/www-svg/2010Nov/0095.html
)

Patches coming here soon -- sorry for the lag time on this.
Attachment #473832 - Attachment is obsolete: true
patch 1: Fix nsSVGImageFrame to handle SVG images

This patch tweaks nsSVGImageFrame to support SVG-backed <image> elements.  The only parts that are different for vector vs raster images are:
 (i)  the image transform (I've tweaked that part of PaintSVG and GetFrameForPoint)
 (ii) the nsLayoutUtils method that we call painting (DrawSingleImage instead of DrawSingleUnscaledImage -- the latter assumes that imgIContainer::GetWidth/GetHeight always work, so we can't rely on it for SVG-backed-images)

This patch does not handle imposing our <image> element's preserveAspectRatio value on the helper image.  I'll add that in upcoming patches.

NOTE: This patch also adds a sanity-check to nsSVGUtils::GetViewBoxTransform (to check a condition that this patch ran afoul of in a previous life).  And as long as I'm already touching it that file, I'm removing its no-longer-used static global variable "gThebesComputationalSurface" (whose last use was removed in bug 585817).
Attachment #495708 - Flags: review?(roc)
This patch exposes methods to query, set, & clear a nsSVGPreserveAspectRatio's animated value.

This will let us (in the next patch) back up the existing animated value (if any) on the image's root <svg> node, and (temporarily) replace it with a new animated value.
Attachment #495714 - Flags: review?(roc)
(Reposting patch with an additional comment; forgot to qrefresh it in before.)
Attachment #495714 - Attachment is obsolete: true
Attachment #495717 - Flags: review?(roc)
Attachment #495714 - Flags: review?(roc)
Attachment #495708 - Attachment description: patch 1: Fix nsSVGImageFrame to handle SVG images → Patch 1: Fix nsSVGImageFrame to handle SVG images
Comment on attachment 495717 [details] [diff] [review]
Patch 2: Add accessors for nsSVGPreserveAspectRatio anim value

+  aPar.SetDefer(((aPackedValue & 0xff0000) >> 16) ? PR_TRUE : PR_FALSE);

((aPackedValue & 0xff0000) >> 16) != 0
Attachment #495717 - Flags: review?(roc) → review+
This patch creates & uses a helper class "PAROverrider. When instantiated, this class overrides the animated value of "preserveAspectRatio" on the passed-in nsSVGSVGElement (unless it's unnecessary -- if value is 'defer' or if there's no viewBox).

The original value (if any) is restored when PAROverrider goes out of scope.

I'm working on a few more intensive tests for this, e.g. with an animated pAR value in the image's helper-document (which should be overridden for <svg:image>) and with the image simultaneously appearing in other contexts (which should show the image without its un-overridden pAR). Those aren't quite ready -- I'll post them when they are -- but they work well enough so far that I think the code & existing tests are ready for review.
Attachment #495725 - Flags: review?(roc)
(In reply to comment #43)
> +  aPar.SetDefer(((aPackedValue & 0xff0000) >> 16) ? PR_TRUE : PR_FALSE);
> 
> ((aPackedValue & 0xff0000) >> 16) != 0

Fixed in patch-queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/4ba788aa9454
set <svg:image> as a supported feature. ( http://www.w3.org/TR/SVG/feature.html )

I'd prefer PAROverrider be called PreserveAspectRatioOverrider.
(In reply to comment #46)
> set <svg:image> as a supported feature.
Fixed in patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/924c501c9eec

> I'd prefer PAROverrider be called PreserveAspectRatioOverrider.
Fixed in patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/fcdb6d7bdc98
There's a mochitest for feature strings test_isSupported.xhtml I hope you've updated that.
I've got just a few more patches to attach here.

First, I split out the <image>-as-a-supported-feature tweak (noted in comment 46/47) into its own patch, and added the mochitest tweak from comment 48.  (I'm making this its own patch since it's not really logically linked to any particular one of the other patches here.)
Attachment #496755 - Flags: review?(longsonr)
This patch addresses an issue with patch 3 -- it turns out that patch 3's temporary-preserveAspectRatio-overriding actually triggers invalidations to everyone who's using the image, saying "image has changed, repaint it!", which puts us in an endless cycle of (asynchronous) repaints.

I'd actually originally been expecting that to be a problem, but I didn't hit those invalidations at first, because I was using a --disable-libxul build, and so invalidations to image-consumers weren't happening for me due to bug 617283.  When I tried patch 3 in a libxul build, though, I could immediately see the CPU effects of the invalidations & self-perpetuating repaints.

This patch fixes that problem by delaying the "DidAnimatePreserveAspectRatio()" call (which triggers invalidation) to not fire until **during** imgIContainer::Draw, at which point we can safely intercept (and drop) any invalidations via the (already-existing) flag SVGDocumentWrapper::mIgnoreInvalidation.
Attachment #496759 - Flags: review?(roc)
Attachment #496759 - Attachment description: Patch 5: Defer notification for pAR tweaks during painting → Patch 5: Delay notification for pAR tweaks during painting
This patch is another followup to patch 3.

My initial "PreserveAspectRatioOverrider" (aka PAROverrider) implementation in patch 3 made the simple (and mistaken) assumption that "defer" unconditionally means we should not override.

In fact, if our helper-document doesn't have any preserveAttributeRatio value specified on its root node, then we *are* supposed to override, even if we've got a "defer" type value.  Not that "defer" is always bundled with a real preserveAspectRatio value (e.g. "defer xMinYMin slice"), and that bundled value is what we'd apply in this case.  ("defer" by itself isn't a valid value).

Reference: http://www.w3.org/TR/SVG11/coords.html#PreserveAspectRatioAttribute
Attachment #496764 - Flags: review?(longsonr)
Attachment #496755 - Flags: review?(longsonr) → review+
(In reply to comment #51)
> Not that "defer" is always bundled with a real preserveAspectRatio value

(sorry - s/Not/Note/)
Comment on attachment 496759 [details] [diff] [review]
Patch 5: Delay notification for pAR tweaks during painting

>+
>+  if (aDeferNotification) {
>+    mHasDeferredAnimNotification = PR_TRUE;
>+  } else {
>+    aSVGElement->DidAnimatePreserveAspectRatio();
>+    mHasDeferredAnimNotification = PR_FALSE;
>+  }

mHasDeferredAnimNotification = aDeferNotification;


> }
>+    if (aDeferNotification) {
>+      mHasDeferredAnimNotification = PR_TRUE;
>+    } else {
>+      aSVGElement->DidAnimatePreserveAspectRatio();
>+      mHasDeferredAnimNotification = PR_FALSE;

here too.
Hmm, this deferred setup is getting a bit ugly.

What if svg:image stored its value of preserveAspectRatio temporarily as an extra property of the image document root element (nsINode::SetProperty)? Then in VectorImage::Draw, read that property and the current animated value of preserveAspectRatio on the image document root element, decide what the effective preserveAspectRatio should be, and temporarily override the animated value with the new computed value if necessary.
Comment on attachment 496764 [details] [diff] [review]
Patch 6: Only "defer" when helper-doc has its own pAR

>+  // If aOverrideValue contains 'defer', we should defer (not override), unless
>+  // root elem lacks a (specified or animated) preserveAspectRatio value.
>+  if (aOverrideValue.GetAnimValue().GetDefer() &&
>+      (mRootElem->HasAttr(kNameSpaceID_None,
>+                          nsGkAtoms::preserveAspectRatio) ||
>+       mModifiedPAR->IsAnimated())) {

I'm tremendously suspicious of any HasAttr call even if the logic is right as it is in this case.
Move the HasAttr into an IsValid member on nsSVGPreserveAspectRatio that would return true on HasAttr || IsAnimated. (Same as nsSVGViewBox IsValid)
that will hide it a bit.

r=longsonr with this.
Attachment #496764 - Flags: review?(longsonr) → review+
(In reply to comment #54)
> What if svg:image stored its value of preserveAspectRatio temporarily as an
> extra property of the image document root element

Agreed -- that sounds cleaner. Working on a patch to do that; will post when ready (later today, I think).

(In reply to comment #55)
> Move the HasAttr into an IsValid member on nsSVGPreserveAspectRatio that would
> return true on HasAttr || IsAnimated. (Same as nsSVGViewBox IsValid)

Sounds good, thanks.
Is there any code in these patches to check for loops? What if I make an SVG file that tries to use itself as the image? If you haven't got that you may need an AutoImageReferencer like an AutoMarkerReferencer
We do check for loops inside VectorImage::Draw.
(In reply to comment #57)
> What if I make an SVG
> file that tries to use itself as the image?

When you use it as an image, you'll get no recursive instances showing up.

When you view it as a standalone SVG document (or via <embed>, etc), you'll get 1 recursive instance (the image), but no recursion beyond that.

Patch 1 includes tests for that situation ("svg-image-recursive-*"), w/ descriptions/explanations of expected output.
In a later patch, I'm going to add a new PRPackedBool member to PreserveAspectRatio.  Rather than adding new bit-shifting / packing code for this additional member, I thought it'd be safer & more readable to just use a union for converting back & forth between packed & expanded form.

So, this patch adds a helper-class "PreserveAspectRatioPacker" to help with that, and it gets rid of the existing bit-packing code with the help of this class.

(Obsoleting most of the old patches here, which were all supporting pieces of the old strategy that I'm abandoning per comment 54 / comment 56)
Attachment #495717 - Attachment is obsolete: true
Attachment #495725 - Attachment is obsolete: true
Attachment #496759 - Attachment is obsolete: true
Attachment #496764 - Attachment is obsolete: true
Attachment #496759 - Flags: review?(roc)
Attachment #497685 - Flags: review?(roc)
Attachment #496755 - Attachment description: Patch 4: Mark <image> as supported feature & update mochitest → Patch N: Mark <image> as supported feature & update mochitest
This patch builds slightly on the last one by making PackPreserveAspectRatio / UnpackPreserveAspectRatio public static methods.  (They're used by nsSVGUtils & nsSVGSVGElement in 2 patches from now.)
Attachment #497687 - Flags: review?(roc)
Attachment #497687 - Attachment description: Patch 3: Make (Un)PackPreserveAspectRatio public static methods → Patch 3 v2: Make (Un)PackPreserveAspectRatio public static methods
(In reply to comment #60)
> In a later patch, I'm going to add a new PRPackedBool member to
> PreserveAspectRatio.

Here's that new member variable.  It indicates whether a PreserveAspectRatio was obtained by parsing a string (PR_TRUE) vs. whether it's just the default underlying value (PR_FALSE) (either from being unset or from parse-failure on an invalid value).

This is necessary to get what I think is the most sensible "defer" behavior.

My old impl quoted in Comment 55 just used "HasAttr" to check for a pAR value on the image's internal document, but that strategy would have us defer even to invalid values like preserveAspectRatio="someInvalidString".  I think it's more sensible to only defer to *valid* preserveAspectRatio values -- but to do that, we need to keep track of when we've parsed one of those.

(I checked Opera & Webkit to see what they do here -- but Opera doesn't seem to honor 'defer' on <image> elements at all, and WebKit applies some odd combination of the referring pAR and the internal pAR.  So, AFAICT, neither of them have behavior that we'd want to mimic here. :))
Attachment #497689 - Flags: review?(roc)
This patch stores the preserveAspectRatio value (as a PRUint64) in the property table of the image document's root node, during drawing. Notes:
 - I added a new property-category for this, since ultimately we'll want to do this for a few other properties ('overflow', 'clip'), and having a separate category lets us just use the property-name as the key, which is pretty convenient.
 - We decide whether to override (returning early for 'defer' & no-viewBox cases) in the setter, nsSVGSVGElement::SetImageOverridePreserveAspectRatio
 - We act on the override value in nsSVGSVGElement::GetViewBoxTransform.
 - We still need to delay notifications until we're in imgIContainer::Draw (as noted in comment 50) to prevent notification ping-pong.  This patch uses a flag "mNeedsPreserveAspectRatioFlush" to do that. (Pretty similar to what the obsolete patch in comment 50 did.)
This one-liner addresses an issue that specifically crops up with <svg:image>, where this happens:
 - Our image is loading...
 - Image::IncrementAnimationConsumers() --> Image::EvaluateAnimation() called
 - EvaluateAnimation's call to VectorImage::ShouldAnimate() fails, since we're still loading -- so we don't StartAnimation yet. (reasonable)
 - We finish loading.
 - We don't get any more calls to EvaluateAnimation, so we never start animating.

(This bug affected the old series of patches here, too -- it was a sporadic issue that happened frequently on tryserver but infrequently locally.  This should fix it.)
(This issue only seems to crop up with <svg:image> -- I think that's because other image-consumers wait until the image loads before they call IncrementAnimationConsumers().  Or maybe they call something else that triggers another EvaluateAnimation() after the image has loaded.  Either way, image-load-time is a reasonable time to re-evaluate whether we should start animating.)
Attachment #497693 - Flags: review?(roc)
Attachment #497696 - Flags: review?(roc)
Attachment #496755 - Attachment description: Patch N: Mark <image> as supported feature & update mochitest → Patch 7: Mark <image> as supported feature & update mochitest
Comment on attachment 497685 [details] [diff] [review]
Patch 2 v2: Use PreserveAspectRatioPacker to convert pAR to/from PRUint64

-nsSVGPreserveAspectRatio::SetAnimValue(PRUint64 aPackedValue, nsSVGElement *aSVGElement)
+nsSVGPreserveAspectRatio::SetAnimValue(const PRUint64& aPackedValue,

Don't make this change.
Attachment #497685 - Flags: review?(roc) → review+
+// Temporary storage for overriden attributes on an <svg> root node. The
+// override values are provided by the referencing <svg:image> element.
+#define SVG_IMAGE_ATTR_OVERRIDEVAL 4

You can use the global category instead.

+  PRUint64* overridePARPtr = GetImageOverridePreserveAspectRatio();
+  if (overridePARPtr) {
+    return nsSVGUtils::GetViewBoxTransform(this,
+                                           viewportWidth, viewportHeight,
+                                           viewBox.x, viewBox.y,
+                                           viewBox.width, viewBox.height,
+                                           *overridePARPtr);
+  } else {
+    return nsSVGUtils::GetViewBoxTransform(this,
+                                           viewportWidth, viewportHeight,
+                                           viewBox.x, viewBox.y,
+                                           viewBox.width, viewBox.height,
+                                           mPreserveAspectRatio);

Better to have just one call to GetViewBoxTransform and make the last parameter
  overridePARPtr ? *overridePARPtr : mPreserveAspectRatio

+  if (aPAR.GetAnimValue().GetDefer() &&
+      mPreserveAspectRatio.GetAnimValue().IsSpecifiedVal()) {
+    return; // Referring element defers to my own preserveAspectRatio value.

Do we need to check IsSpecifiedVal here? Can't we just always return early if the image says 'defer'?

+  nsresult rv = SetProperty(SVG_IMAGE_ATTR_OVERRIDEVAL,
+                            nsGkAtoms::preserveAspectRatio,
+                            packedValPtr,
+                            ReleasePRUint64PropertyValue);

So you can just set the three-arg SetProperty here. You might want to pick a new atom value though.
Depends on: 619516
(Attaching updated version of patch 5)

(In reply to comment #66)
> You can use the global category instead.

Done, using new atom "nsGkAtoms::overridePreserveAspectRatio" as the hash key.
 
> +  PRUint64* overridePARPtr = GetImageOverridePreserveAspectRatio();
> +  if (overridePARPtr) {
> +    return nsSVGUtils::GetViewBoxTransform(this,

> Better to have just one call to GetViewBoxTransform and make the last parameter
>   overridePARPtr ? *overridePARPtr : mPreserveAspectRatio

I previously couldn't do this cleanly (since GetViewBoxTransform takes a nsSVGPreserveAspectRatio, which is a 2-valued "bundle", and we only have 1 override value to apply), but I filed Bug 619516 to clean up that situation.  So, this bug's patches now layer on top of that bug's patches.

Bug 619516 also makes the PRUint64-packing here unnecessary, too (by promoting nsSVGPreserveAspectRatio::PreserveAspectRatio to a normal class that can be forward-declared and used in public APIs, rather than being an inner helper-class).  So, Patch 2 & 3 (using PRUint64 packing) are now obsolete for this bug's purposes -- sorry for the wasted review-time on those.  (but hooray for the resulting cleaner code! :))

> +  if (aPAR.GetAnimValue().GetDefer() &&
> +      mPreserveAspectRatio.GetAnimValue().IsSpecifiedVal()) {
> +    return; // Referring element defers to my own preserveAspectRatio value.
> 
> Do we need to check IsSpecifiedVal here? Can't we just always return early if
> the image says 'defer'?

No, "defer" isn't always an early-return -- see Comment 51 & chunk of spec quoted below. (Basically, "defer" only defers when there's a pAR value to defer to.)

I've tweaked this slightly to no longer use "IsSpecifiedValue", though.  I think I was wrong spec-wise to be caring about *valid* preserveAspectRatio values.  The spec just says, for defer...
   the value of the ‘preserveAspectRatio’ attribute on the referenced
   content if present should be used
...so it's just whether a value (any value) is present.  Validitity doesn't seem to matter.  (The sentence after that one implies this as well.)
http://www.w3.org/TR/SVG11/coords.html#PreserveAspectRatioAttribute

So, this patch goes back to using HasAttr (matching the partial-patch in Comment 51), using a helper method ("HasPreserveAspectRatio()") as longsonr suggested in his review comments for that partial-patch.  (though he'd suggested putting it elsewhere because at that point the check was happening elsewhere)

So, this means that patch 4 (adding mIsSpecifiedVal flag to pAR for valid parsed pAR values) is obsolete, too.  Again, sorry for the wasted review-time, and hooray for less added code. :)
Attachment #497685 - Attachment is obsolete: true
Attachment #497687 - Attachment is obsolete: true
Attachment #497689 - Attachment is obsolete: true
Attachment #497693 - Attachment is obsolete: true
Attachment #498186 - Flags: review?(roc)
Attachment #497693 - Flags: review?(roc)
roc: You'd also mentioned in IRC possibly caching the last-used override preserveAspectRatio, e.g. on the VectorImage or SVGDocumentWrapper, so that we don't have to flush a viewport change on every single draw in the single-<image>-client case.

If it's ok with you, I'd like to do that as a small followup bug, since it's an optimization & shouldn't affect behavior, and this bug's already suffered more than its fair share of patch-spam. :)  (And also on the off chance that the optimization causes regressions or breakage, I'd like to be able to back out the optimization without backing out the <image> functionality entirely).
Comment on attachment 498186 [details] [diff] [review]
Patch 2 v3: Store override pAR value in property table

+      nsSVGSVGElement* rootElem =
+        static_cast<nsSVGSVGElement*>(imgRootFrame->GetContent());
+      NS_ABORT_IF_FALSE(rootElem, "root layout frame with no content node?");

I think we should probably check the element type here and only do the following code if it's really an <svg>.

Maybe that's not strictly necessary but it'll be simple enough and crash- and future-proof.
Attachment #498186 - Flags: review?(roc)
Attachment #498186 - Flags: review+
Attachment #498186 - Flags: approval2.0+
Sounds good -- fixed that in my patch queue. Thanks!
Blocks: 619835
Filed Bug 619835 on the optimization mentioned in comment #68 here.
Comment on attachment 498186 [details] [diff] [review]
Patch 2 v3: Store override pAR value in property table

(Re-renumbering patches so they make more sense, given the obsoleted intermediate patches in comment 67.)
Attachment #498186 - Attachment description: Patch 5 v3: Store override pAR value in property table → Patch 2 v3: Store override pAR value in property table
Attachment #497696 - Attachment description: Patch 6: Add missing EvaluateAnimation() call → Patch 3: Add missing EvaluateAnimation() call
Attachment #496755 - Attachment description: Patch 7: Mark <image> as supported feature & update mochitest → Patch 4: Mark <image> as supported feature & update mochitest
This (last!) patch adds two more sets of reftests:

* layout/reftests/svg/as-image/img-and-image-1.html & friends:
  - HTML file which references three SVG images, the third of which references the other two.

* layout/reftests/svg/image/image-preserveAspectRatio-03.svg, 04.svg
  - 03 tests <image preserveAspectRatio> **with** an animated preserveAspectRatio in the referenced element
  - 03 & 04 both test preserveAspectRatio='defer...' as a 'bonus' element in the grid.  In 03, there's an underlying pAR value (in the image) that we can defer to, and in 04 there isn't (so we don't defer).
Attachment #498263 - Flags: review?(roc)
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
I made one more tweak to patch 2 before landing, btw -- I made nsSVGSVGElement::FlushPreserveAspectRatioOverride into a virtual method, to fix a non-libxul build issue. (since that function is defined in gklayout and gets called in imagelib)
Keywords: dev-doc-needed
Whiteboard: [https://developer.mozilla.org/En/SVG_in_Firefox needs updating]
Updated https://developer.mozilla.org/En/SVG_in_Firefox to indicate that SVG-backed <image> elements are supported as of Gecko 2.0.
Keywords: dev-doc-needed
Whiteboard: [https://developer.mozilla.org/En/SVG_in_Firefox needs updating]
Hi Daniel,
I got the href/svg/images working using the latest FF nightly build (ff4b9pre). I suppose it includes your patch. Thank you very much.
I am not custom with Firefox bug-management / version-release and I have some doubts... I would appreciate (much, as our whole projects depends on this) some notes about what will happen now with your patch. Is it for sure that it will be also included in the next FF4b9 release? How about in the first FF4.0 release? Is there a votation to encourage to include it?
Thank you again.
(In reply to comment #77)
> Is it for sure that it will
> be also included in the next FF4b9 release?

Yes.

> How about in the first FF4.0
> release?

99.99% yes.

Basically: once stuff gets checked in to mozilla-central (as this did in Comment 74), it stays there and will make it into all future releases off of mozilla-central (which in this case includes all future 4.0 releases, including 4.0b9).  (That is -- unless the patch causes serious regressions/problems that can't be fixed in followup bugs, which is unlikely in this case.)

(At some point Gecko 2.0/Firefox 4.0 will branch and get its own separate repository in which changes will need to land to get into 4.0 builds, but that hasn't happened yet.)
I am using FF 7.0.1 0n Linux Ubuntu 11.10,  This does not Work!  I will attach a test case that is is broken.  The get the test case to work you will have to add your own images and svg image.  I have tested it using Chrome and it works. But i FF it does not.

The basic test case is html file with <embed> svg.  Then  <image xlink:href="other.svg">
Sorry could not find how to upload file

***************

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html  style="height:100%:width:100%">
<head>
<title>ffsvginsvg</title>
<meta name="generator" content="Bluefish">
<meta name="author" content="">
<meta name="date" content="2011-10-12T15:07:47+0800" >
<meta name="copyright" content="">
<meta name="keywords" content="">
<meta name="description" content="">
<meta name="ROBOTS" content="NOINDEX, NOFOLLOW">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<meta http-equiv="content-type" content="application/xhtml+xml; charset=UTF-8">
<meta http-equiv="content-style-type" content="text/css">
<meta http-equiv="expires" content="0">
<link href="ff.css" rel="stylesheet" type="text/css">
</head>
<body>
	<div id="main" style="width:100%;height:680px;background-color:#888;overflow:auto;">
		<embed  src="ffsvginsvg.svg" type="image/svg+xml" width="2000px" height="800px"/>
	</div>
</body>

**********************************************
<svg
	xmlns="http://www.w3.org/2000/svg"
	xmlns:svg="http://www.w3.org/2000/svg"
	xmlns:xlink='http://www.w3.org/1999/xlink'
	xmlns:html="http://www.w3.org/1999/xhtml"
	version="1.1"
	id="main_svg"
	fill="#111">
	<defs>
	</defs>
	<g>
		<rect id="background" width="1000" height="800" fill="#111"/>
		<g id="ships" width="100%" height="100%" fill="#111">
			<image id="ship1" xlink:href="images/spacecraft_2.svg" width="100" height="70" x="100" y="350"/>
			<image id="ship2" xlink:href="images/spaceship-1.png" width="100" height="70" x="100" y="400"/>
		</g>
		<image id="planet" xlink:href="images/planets13.gif" width="70" height="70" x="900" y="375"/>
	</g>
</svg>
(In reply to savages from comment #79)
> The basic test case is html file with <embed> svg.  Then  <image
> xlink:href="other.svg">

In case you meant <img> instead of <embed>, then this behavior is intentional. See bug 628747 comment 0 for details. For SVG in an image context (<img>/background/etc), we don't allow SVG to load *any* external resources.  (though you can embed external resources inline as a data URI)

If the above doesn't address your concern, please file a new bug and we can take it up there.  This bug here is about <svg:image xlink:href="foo.svg"> working _at all_ in SVG, and specific edge-cases like what you describe should be handled as separate bugs.
@savages: FWIW, I tested the testcase you provided (filling in my own SVG and PNG images, linked from the SVG content) and it worked just fine for me, in both Firefox 7.0.1 and trunk.

So I won't file a followup bug myself, since I can't reproduce, but if you're still hitting this, please do please file a bug here:
  https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=SVG
and we can take it from there.
You need to log in before you can comment on or make changes to this bug.