Bug 5971 – svg content doesn't work in <image> <img> or <feImage> tags

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

svg content doesn't work in <image> <img> or <feImage> tags

This is a very important bug.  Basically WebCoreImageRenderer needs to be taught how to load an 
arbitrary xml document, turn off certain features (javascript?) and then render it to a pixmap.  It also 
needs to be smart enough to listen to document changes and re-render the pixmap, invalidating all clients 
of that image.

Although the spec does not say so, this should be implemented for any arbitrary document (xhtml too), 
since at least <foreignObject> tags will have to work in this context.  Basically there should be some sort 
of drawRenderObjectToPixmap() call.  <pattern> could also take advantage of such a call.  As could 
<cursor> and many other places in the SVG code.

[Eric Seidel (no email)]

In my view this will be the primary use of SVG throughout the web.  We cannot ship SVG w/o this feature.  Adding SVGHitList keyword and bumping to P2.

In order to fix this would require pulling WebCoreImageRenderer down into WebCore from WebKit, and then wiring in support for SVG images into the renderer and possibly adding a CachedDocument class, or some other way of holding the cached SVG document data.

This will also require disabling JavaScript for that particular SVG, as well as possibly disabling SVG animations and interaction depending on the context.

[Dave Hyatt]

Agreed that this is a killer feature.

[Dave Hyatt]

I don't know that you'd have to disable Javascript.  I talked with Hixie about this and some principles we agreed on were:

(1) No interactivity, i.e., you can't get to the subdocument from the parent, and you can't hit test on anything.  It's just a single atomic image as far as the page is concerned.
(2) Animation should be fine, and there's no reason to preclude JS from being the way that animation might be done.
(3) The document has to be sandboxed.  No connection would exist between this document and the rest of the loaded pages.  It couldn't target other frames for example or get to any other frames/pages/documents.

Adding ian to cc list.

[Nicholas Shanks]

Until this is implemented can you please add image/svg+xml;q=0 to the accept list sent when retrieving source documents of <img> tags and the others. Safari prefers the SVG images over PNG at the moment as they're sometimes smaller and will likely have higher qs values too.

[Eric Seidel (no email)]

Created attachment 10034 [details]
My first hack-and-slash architecture exploration (for hyatt and others to look at)

I wonder if I even dare try and list all the problems with this patch.

The purpose is to let hyatt and others look at this, and promote intelligent discussion about what sort of architecture re-workings will be necessary to make SVG content work where bitmap images work today (<img>, background-image, etc.).

To do this, I made 'Image' an abstract class, breaking logic out into BitmapImage and PDFDocumentImage.  I also added a 'SVGImage' subclass of Image.

However, there are issues:

Right now RenderImage is not wired up to allow the Image object to effect the size calculation.  This will have to change for this to work.  (Right now for percentage-sized <svg>s the code just assumes a 1000x1000 viewport and sizes the SVG canvas accordingly.)

Also I'm wondering if RenderImage is the right Renderer to use here at all.  I'm having to fake a bunch of stuff that RenderFrame normally would do.  Perhaps <img>, etc should just use a RenderFrame when the Image is an SVG and set a RenderFrame::pretendToBeAnImage flag.  I'm not sure how background-image works, we may end up needing a hacked up SVGImage anyway.

If we go with the SVGImage route, we'll have to teach WebCore how to make Page/FrameView/Frame stacks w/o needing to use WebKit to store the view size.  (The FrameView thinks its content is 0x0.  I believe that is why the background color is not correctly drawn.)

Comments are encouraged.

[Eric Seidel (no email)]

Created attachment 10035 [details]
image showing my hack-and-slash demo patch in action

[Eric Seidel (no email)]

Created attachment 10719 [details]
An updated version of my original hack-and-slash patch

This patch actually compiles (and sorta works!) on TOT.  I've added it to the bug to give hyatt and I another launching point for discussions.  I still don't like the frameAtIndex() architecture, nor do I like how many virtual methods I had to add to Image/BitmapImage to make this all work.

SVGImages aren't sized correctly (hyatt and I talked about ways to do this before, we need to talk again), and I'm not even sure if PDF images still work (I didn't test).  Patterned images for both PDF and SVG do not work (mostly due to the frameAtIndex() architectural problems).

Please don't re-write all of Image out from under me again. ;(

[Eric Seidel (no email)]

Adding hyatt so he can see my new patch.

[Eric Seidel (no email)]

Created attachment 10720 [details]
Updated version of my original hack-and-slash patch (now with all the files)

Hyatt:
One thing I should note, I removed these lines of code:
 void Image::invalidatePlatformData()
 {
-    if (m_frames.size() != 1)
-        return;
-
which seemed completely bogus (at best, a cause of leaks).  Perhaps you can explain to me why they were needed/wanted?

[Dave Hyatt]

While image frames after the first load, there's no need to invalidate the NSImage or the TIFFRep of frame 0.  So when invalidateData is being called while loading, the code is correct. 

However, you're right that the code ends up being incorrect when invoked from the Image destructor.  I think you're right that the simplest fix is to just yank the guard.

[Eric Seidel (no email)]

Created attachment 12075 [details]
Another functional SVGImage patch

Woh.  This one has some serious hacks in it.  No, we could never land this.  But, it does demonstrate some architectural issues that will need to be considered if we're ever going to get SVGImage to work for real.  SVGImage needs to be passed a Page object.  SVGImages are created from a CachedImage, which although it is passed a docLoader in its constructor, does not hold onto that pointer, and thus has no way to tell SVGImage what the current page is.  I'm not even sure if we would want SVGImages to use the current page.  Using their own custom page object (like this patch does) is really really really ugly, and likely won't work long-term.

At least hyatt and othermaciej should take a peak at this patch.

[Eric Seidel (no email)]

Created attachment 12076 [details]
An updated hack, with better memory management (just code see old patch for tests)

So I looked a little bit at ways to avoid having SVGImage create its own Page.  Unfortunately I don't see any way to make that work inside SVGImage.

CachedImage objects are not tied to a page or a frame.  Image objects, are thus not tied to either.  Even if they were, passing the Page into the image and creating a frame for it would change the mainFrame on the page, unless we were also to pass an ownerElement to the Frame constructor.  We have no such element inside SVGImage unless we also manage to pass down the HTMLImageElement (or SVGImageElement) down, but that doesn't necessarily work for background images or filters, so that's a no go.

Another thought would be to teach the caching system to abort any CachedImage load when it sees it trying to load image/svg+xml, and instead do a requestObject load like embed/object do, or somehow "half load" the image inside SVGImage, and then have something outside (like HTMLImageElement) know how to finish the load.  That also doesn't sound optimal, given the fact that images should be usable from backgrounds, filters, etc without lots of extra code.

Another possibility is to ignore the Frame/FrameView/Page setup entirely and just directly create an SVGDocument, decode the data stream with our own Decoder, and create our own parser, etc.  The problem I forsee with that is that things in Document (and the DOM tree) likely expect to have a Frame.    I'm not certain of that requirement however.  It would be a substantial amount of work to find out how required Frame objects are though.  (Unless hyatt, darin, mjs care to comment on this possibility.)

So that leads us back to creating or own Page/Frame/FrameView stack.  The problem here has become much harder since I initial started on this bug.   WebCore is now very very tied into WebKit.  Page and Frame classes now depend on being passed client objects and crash when they don't exist.  So to make this patch work, I created (really ugly!) dummy objects, which are completely empty.  This solution could work, except that it's likely to be very brittle.  I can forsee a solution like this breaking the build constantly any time someone wished to change a client.

I'm interested to hear your thoughts.  The attached patch is actually quite stable (as far as I've seen) and doesn't leak (any more than the dummy objects which are allocated once).  The code is a bit ugly, with all the dummy client classes embedded inside SVGImage.cpp.

If this hack wasn't so brittle, I would actually seriously propose it for landing.

I'm very hesitant to start rounding out the corners of this patch by solving things like tiling, size negotiation, etc.  Those should all be pretty easy and quick once a simple architecturally-sound skeleton is landed.  Until then, I think solving the smaller issues is just wasting my time as this patch will just rot until a way can be found to wire this into the loading architecture for real.

I'm interested to hear your comments.

[Eric Seidel (no email)]

Created attachment 12077 [details]
A slightly prettier version

So this is yes, still very much a hack.  But it does work, and looks a lot nicer with the stubs moved out into a separate file.  I'm going to mark this for review and hopefully receive some insightful suggestions as to a better way to do this. :)

[Eric Seidel (no email)]

Another possibly less hacky way to do this would be to add a call in WebKit (and a static method on Page) which wired up a "dummy" Page->Frame->FrameView system for the SVGImage to use.

[Dave Hyatt]

The empty client thing actually doesn't bother me so much.  Brady, Eric and I talked on IRC about the idea of having the empty clients be a more general concept, e.g., so that you can have a more lightweight Frame/Page/FrameView stack that isn't concerned with the client functionality.

I am comfortable with this idea, since I think other features will need this same capability.  For example, XBL subresource documents could fit into this model.

[Dave Hyatt]

Comment on attachment 12077 [details]
A slightly prettier version

I am giving this a +, even with the empty client hack.  These can always be moved later for a more general purpose solution without too much trouble.

[Eric Seidel (no email)]

Finally.

Lots left to be done here.  <image> and <feImage> need support for fragment urls (should be a couple hours work).  SVGImage does not yet know how to tile (thus it does not work as a background image or border-image).  Image does not have a way for SVGImage to negotiate size with the caller, until that's made available, SVGImages won't scale properly.

Also, the architecture needs to be cleaned up.  All the SVGEmpty*Clients are really ugly.

But it's landed!

[jay]

 is this fixed, but disabled on trunk?

I'd like to use and publicise this feature, particularly if it works for html emails....

if landed, where is it?

cheers MacDome ~:"

[jay]

*** Bug 15314 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

It was implemented, and then got turned off again in an effort to stabilize for leopard. I believe there is a bug about turning it on again.  I'm not even sure it is turned on in feature-branch.

[jay]

Not working in either the trunk or feature-build nightlies for 30/09/07

[Robert Blaut]

(In reply to comment #20)
> It was implemented, and then got turned off again in an effort to stabilize for
> leopard. I believe there is a bug about turning it on again.  I'm not even sure
> it is turned on in feature-branch.
> 

Is it turned on again? This document http://docs.info.apple.com/article.html?artnum=307467 advertises tah SVG works now for img elements and also as CSS images (background-image?) but my quick test fails on this subject:

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cp%20style%3D%22background-image%3A%20url(http%3A%2F%2Ffiles.myopera.com%2Fdstorey%2Fexperiments%2Fimages%2Froundedcorners.svg)%3B%22%3Etest%3C%2Fp%3E%0A%3Cp%3E%20%3Cimg%20src%3D%22http%3A%2F%2Ffiles.myopera.com%2Fdstorey%2Fexperiments%2Fimages%2Froundedcorners.svg%22%3E%3C%2Fp%3E

[Eric Seidel (no email)]

(In reply to comment #22)
> Is it turned on again? This document
> http://docs.info.apple.com/article.html?artnum=307467 advertises tah SVG works
> now for img elements and also as CSS images (background-image?) but my quick
> test fails on this subject:
> 
> http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cp%20style%3D%22background-image%3A%20url(http%3A%2F%2Ffiles.myopera.com%2Fdstorey%2Fexperiments%2Fimages%2Froundedcorners.svg)%3B%22%3Etest%3C%2Fp%3E%0A%3Cp%3E%20%3Cimg%20src%3D%22http%3A%2F%2Ffiles.myopera.com%2Fdstorey%2Fexperiments%2Fimages%2Froundedcorners.svg%22%3E%3C%2Fp%3E
> 

Yes SVG images work as backgrounds, but *only* when they have an explicit width and height set on the <svg> element.

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cp%20style%3D%22background-image%3A%20url(http%3A%2F%2Fcroczilla.com%2Fsvg%2Fsamples%2Fchem1%2Fchem1.svg)%3B%22%3Etest%3C%2Fp%3E%0A%3Cp%3E%20%3Cimg%20width%3D%22100px%22%20height%3D%22100px%22%20src%3D%22http%3A%2F%2Fcroczilla.com%2Fsvg%2Fsamples%2Fchem1%2Fchem1.svg%22%3E%3C%2Fp%3E

works.  Note, that you're also hitting: http://bugs.webkit.org/show_bug.cgi?id=17552