Last Comment Bug 272288 - Allow SVG source for <svg:image>
: Allow SVG source for <svg:image>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: P2 normal with 34 votes (vote)
: mozilla2.0b9
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://www.w3.org/Graphics/SVG/Test/2...
: 409988 (view as bug list)
Depends on: 216568 276431 598798 619516
Blocks: 455986 svg11tests 619835 805332 435299
  Show dependency treegraph
 
Reported: 2004-11-29 12:52 PST by tor
Modified: 2012-10-24 21:23 PDT (History)
47 users (show)
roc: blocking1.9.2-
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
this image is linked in xref-b.svg (1.11 KB, image/svg+xml)
2009-08-18 09:41 PDT, Martin Hejral
no flags Details
test case (1.63 KB, image/svg+xml)
2009-08-18 09:43 PDT, Martin Hejral
no flags Details
rendering in Safari 4 - last, it's time for the Firefox (69.89 KB, image/png)
2009-08-18 09:49 PDT, Martin Hejral
no flags Details
Patch 1 v1: Use nsLayoutUtils::DrawSingleImage (3.44 KB, patch)
2010-09-09 17:08 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Patch 2: Fix nsSVGImageFrame::GetImageTransform (3.38 KB, patch)
2010-09-09 17:21 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Patch 1: Fix nsSVGImageFrame to handle SVG images (20.39 KB, patch)
2010-12-06 17:17 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 2: Add accessors for nsSVGPreserveAspectRatio anim value (6.18 KB, patch)
2010-12-06 17:29 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Patch 2: Add accessors for nsSVGPreserveAspectRatio anim value (6.21 KB, patch)
2010-12-06 17:33 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 3: temporarily override helper document's preserveAspectRatio when painting <image> (10.85 KB, patch)
2010-12-06 17:58 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 4: Mark <image> as supported feature & update mochitest (2.80 KB, patch)
2010-12-09 22:38 PST, Daniel Holbert [:dholbert]
longsonr: review+
Details | Diff | Splinter Review
Patch 5: Delay notification for pAR tweaks during painting (11.25 KB, patch)
2010-12-09 22:54 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Patch 6: Only "defer" when helper-doc has its own pAR (2.09 KB, patch)
2010-12-09 23:08 PST, Daniel Holbert [:dholbert]
longsonr: review+
Details | Diff | Splinter Review
Patch 2 v2: Use PreserveAspectRatioPacker to convert pAR to/from PRUint64 (5.80 KB, patch)
2010-12-14 16:50 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 3 v2: Make (Un)PackPreserveAspectRatio public static methods (3.84 KB, patch)
2010-12-14 16:54 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 4 v2: Add "mIsSpecifiedVal" flag to pAR (2.48 KB, patch)
2010-12-14 17:07 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 5 v2: Store override pAR value in property table (27.73 KB, patch)
2010-12-14 17:23 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Patch 3: Add missing EvaluateAnimation() call (888 bytes, patch)
2010-12-14 17:35 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
Patch 2 v3: Store override pAR value in property table (23.30 KB, patch)
2010-12-16 12:21 PST, Daniel Holbert [:dholbert]
roc: review+
roc: approval2.0+
Details | Diff | Splinter Review
Patch 5: reftest for images w/ simultaneous SVG+HTML observers & reftests for 'defer' (18.91 KB, patch)
2010-12-16 17:04 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description tor 2004-11-29 12:52:18 PST
 
Comment 1 Boris Zbarsky [:bz] 2005-01-05 20:53:45 PST
Is there a reason this should be specific to <svg:image>?  We should be able to
allow this for <img> nodes too, no?
Comment 2 tor 2005-01-05 21:08:20 PST
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')."
Comment 3 Boris Zbarsky [:bz] 2005-01-05 21:10:37 PST
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).
Comment 4 Jesse Ruderman 2005-09-14 18:28:38 PDT
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.
Comment 5 Boris Zbarsky [:bz] 2005-09-14 23:19:37 PDT
There has been talk of doing non-scripting SVG only for such cases (that is,
allowing declarative animation, but not scripted stuff).
Comment 6 Christian Balkenius 2005-09-18 03:59:50 PDT
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>.
Comment 7 Bruce Rindahl 2005-11-27 15:25:12 PST
(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.
Comment 8 Anne (:annevk) 2005-11-28 00:39:17 PST
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.
Comment 9 Jeff Schiller 2005-11-28 05:49:44 PST
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.  
Comment 10 Anne (:annevk) 2005-11-29 02:32:46 PST
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.
Comment 11 Robert Longson 2007-12-27 13:38:34 PST
*** Bug 409988 has been marked as a duplicate of this bug. ***
Comment 12 John Ellson 2008-01-02 11:37:00 PST
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
Comment 13 jonathan chetwynd 2008-02-18 01:06:03 PST
parity Opera
parity Safari
http://files.myopera.com/MacDev_ed/sarpsborg2007/external-use.svg

does this bug need a testcase or description?
Comment 14 Tomasz Kaźmierczak 2008-02-27 16:24:52 PST
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).
Comment 15 Marcus Andersson 2008-05-23 00:44:10 PDT
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

Comment 16 Martin Hejral 2008-09-28 23:41:27 PDT
(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...
Comment 17 Tomasz Kaźmierczak 2008-11-11 11:27:27 PST
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>.
Comment 18 Tomasz Kaźmierczak 2009-08-15 15:21:32 PDT
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.
Comment 19 Martin Hejral 2009-08-18 09:41:49 PDT
Created attachment 395078 [details]
this image is linked in xref-b.svg
Comment 20 Martin Hejral 2009-08-18 09:43:01 PDT
Created attachment 395079 [details]
test case
Comment 21 Martin Hejral 2009-08-18 09:49:44 PDT
Created attachment 395080 [details]
rendering in Safari 4 - last, it's time for the Firefox

XREF testcase works good with Opera 9, Safari 4 and ASV 3
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-09-22 20:38:44 PDT
Not making it.
Comment 23 worms 2009-10-19 21:25:26 PDT
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!
Comment 24 Jonathan Watt [:jwatt] 2009-10-23 09:31:48 PDT
See bug 276431 comment 65.
Comment 25 Daniel Holbert [:dholbert] 2010-09-09 09:25:37 PDT
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 )
Comment 26 Daniel Holbert [:dholbert] 2010-09-09 09:26:52 PDT
er, sorry, I meant "bug 276431" &  "bug 276431 comment 159" in previous comment
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-09 15:42:52 PDT
(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.
Comment 28 Daniel Holbert [:dholbert] 2010-09-09 16:44:38 PDT
Yeah, that sounds better.  Patches coming up soon.
Comment 29 Daniel Holbert [:dholbert] 2010-09-09 17:08:50 PDT
Created attachment 473830 [details] [diff] [review]
Patch 1 v1: Use nsLayoutUtils::DrawSingleImage

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.
Comment 30 Daniel Holbert [:dholbert] 2010-09-09 17:21:44 PDT
Created attachment 473832 [details] [diff] [review]
Patch 2: Fix nsSVGImageFrame::GetImageTransform

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.
Comment 31 Robert Longson 2010-09-09 23:13:24 PDT
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.
Comment 32 Robert Longson 2010-09-09 23:14:16 PDT
Also do you need to do something for feImage in nsSVGFilters.cpp or does that already work?
Comment 33 Robert Longson 2010-09-10 14:01:28 PDT
What happened to opacity support too?
Comment 34 Daniel Holbert [:dholbert] 2010-09-10 14:19:23 PDT
(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.
Comment 35 Daniel Holbert [:dholbert] 2010-09-23 11:10:59 PDT
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.
Comment 36 Daniel Holbert [:dholbert] 2010-09-23 11:32:19 PDT
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)
Comment 37 Robert Longson 2010-09-23 11:43:51 PDT
B rather depends on whether PreserveAspectRatio is defer or not doesn't it?
Comment 38 Daniel Holbert [:dholbert] 2010-09-23 13:19:41 PDT
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.)
Comment 39 Daniel Holbert [:dholbert] 2010-12-06 15:50:41 PST
(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.
Comment 40 Daniel Holbert [:dholbert] 2010-12-06 17:17:37 PST
Created attachment 495708 [details] [diff] [review]
Patch 1: Fix nsSVGImageFrame to handle SVG images

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).
Comment 41 Daniel Holbert [:dholbert] 2010-12-06 17:29:50 PST
Created attachment 495714 [details] [diff] [review]
Patch 2: Add accessors for nsSVGPreserveAspectRatio anim value

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.
Comment 42 Daniel Holbert [:dholbert] 2010-12-06 17:33:23 PST
Created attachment 495717 [details] [diff] [review]
Patch 2: Add accessors for nsSVGPreserveAspectRatio anim value

(Reposting patch with an additional comment; forgot to qrefresh it in before.)
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-06 17:46:21 PST
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
Comment 44 Daniel Holbert [:dholbert] 2010-12-06 17:58:40 PST
Created attachment 495725 [details] [diff] [review]
Patch 3: temporarily override helper document's preserveAspectRatio when painting <image>

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.
Comment 45 Daniel Holbert [:dholbert] 2010-12-06 18:18:44 PST
(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
Comment 46 Robert Longson 2010-12-07 01:50:33 PST
set <svg:image> as a supported feature. ( http://www.w3.org/TR/SVG/feature.html )

I'd prefer PAROverrider be called PreserveAspectRatioOverrider.
Comment 47 Daniel Holbert [:dholbert] 2010-12-07 12:18:42 PST
(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
Comment 48 Robert Longson 2010-12-07 12:24:10 PST
There's a mochitest for feature strings test_isSupported.xhtml I hope you've updated that.
Comment 49 Daniel Holbert [:dholbert] 2010-12-09 22:38:28 PST
Created attachment 496755 [details] [diff] [review]
Patch 4: Mark <image> as supported feature & update mochitest

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.)
Comment 50 Daniel Holbert [:dholbert] 2010-12-09 22:54:22 PST
Created attachment 496759 [details] [diff] [review]
Patch 5: Delay notification for pAR tweaks during painting

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.
Comment 51 Daniel Holbert [:dholbert] 2010-12-09 23:08:17 PST
Created attachment 496764 [details] [diff] [review]
Patch 6: Only "defer" when helper-doc has its own pAR

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
Comment 52 Daniel Holbert [:dholbert] 2010-12-09 23:46:29 PST
(In reply to comment #51)
> Not that "defer" is always bundled with a real preserveAspectRatio value

(sorry - s/Not/Note/)
Comment 53 Robert Longson 2010-12-10 01:53:48 PST
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.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-10 01:58:48 PST
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 55 Robert Longson 2010-12-10 02:27:07 PST
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.
Comment 56 Daniel Holbert [:dholbert] 2010-12-13 14:28:48 PST
(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.
Comment 57 Robert Longson 2010-12-13 16:31:16 PST
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
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-13 16:38:41 PST
We do check for loops inside VectorImage::Draw.
Comment 59 Daniel Holbert [:dholbert] 2010-12-14 16:01:53 PST
(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.
Comment 60 Daniel Holbert [:dholbert] 2010-12-14 16:50:33 PST
Created attachment 497685 [details] [diff] [review]
Patch 2 v2: Use PreserveAspectRatioPacker to convert pAR to/from PRUint64

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)
Comment 61 Daniel Holbert [:dholbert] 2010-12-14 16:54:35 PST
Created attachment 497687 [details] [diff] [review]
Patch 3 v2: Make (Un)PackPreserveAspectRatio public static methods

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.)
Comment 62 Daniel Holbert [:dholbert] 2010-12-14 17:07:59 PST
Created attachment 497689 [details] [diff] [review]
Patch 4 v2: Add "mIsSpecifiedVal" flag to pAR

(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. :))
Comment 63 Daniel Holbert [:dholbert] 2010-12-14 17:23:03 PST
Created attachment 497693 [details] [diff] [review]
Patch 5 v2: Store override pAR value in property table

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.)
Comment 64 Daniel Holbert [:dholbert] 2010-12-14 17:35:42 PST
Created attachment 497696 [details] [diff] [review]
Patch 3: Add missing EvaluateAnimation() call

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.)
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-14 22:37:07 PST
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.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-14 22:48:00 PST
+// 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.
Comment 67 Daniel Holbert [:dholbert] 2010-12-16 12:21:08 PST
Created attachment 498186 [details] [diff] [review]
Patch 2 v3: Store override pAR value in property table

(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. :)
Comment 68 Daniel Holbert [:dholbert] 2010-12-16 12:25:45 PST
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 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-16 12:35:59 PST
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.
Comment 70 Daniel Holbert [:dholbert] 2010-12-16 16:22:10 PST
Sounds good -- fixed that in my patch queue. Thanks!
Comment 71 Daniel Holbert [:dholbert] 2010-12-16 16:25:45 PST
Filed Bug 619835 on the optimization mentioned in comment #68 here.
Comment 72 Daniel Holbert [:dholbert] 2010-12-16 16:50:44 PST
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.)
Comment 73 Daniel Holbert [:dholbert] 2010-12-16 17:04:47 PST
Created attachment 498263 [details] [diff] [review]
Patch 5: reftest for images w/ simultaneous SVG+HTML observers & reftests for 'defer'

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).
Comment 75 Daniel Holbert [:dholbert] 2010-12-19 17:15:26 PST
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)
Comment 76 Daniel Holbert [:dholbert] 2010-12-27 10:58:47 PST
Updated https://developer.mozilla.org/En/SVG_in_Firefox to indicate that SVG-backed <image> elements are supported as of Gecko 2.0.
Comment 77 boadadf 2011-01-11 07:33:29 PST
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.
Comment 78 Daniel Holbert [:dholbert] 2011-01-11 08:52:00 PST
(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.)
Comment 79 savages 2011-10-26 21:17:45 PDT
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">
Comment 80 savages 2011-10-26 21:21:57 PDT
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>
Comment 81 Daniel Holbert [:dholbert] 2011-10-27 08:42:12 PDT
(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.
Comment 82 Daniel Holbert [:dholbert] 2011-10-27 11:40:05 PDT
@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.

Note You need to log in before you can comment on or make changes to this bug.