Bug 33630 - SVG not rendered as background-image from data URI
Summary: SVG not rendered as background-image from data URI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-13 15:48 PST by Sidney San Martín
Modified: 2010-04-02 10:51 PDT (History)
8 users (show)

See Also:


Attachments
Demo of SVG data URI in CSS (807 bytes, text/html)
2010-01-13 15:48 PST, Sidney San Martín
no flags Details
Patch v1 (4.12 KB, patch)
2010-01-29 13:10 PST, Yael
no flags Details | Formatted Diff | Diff
Patch v2 (5.60 KB, patch)
2010-02-01 11:27 PST, Yael
eric: review-
Details | Formatted Diff | Diff
Patch v3 (5.55 KB, patch)
2010-02-01 18:20 PST, Yael
no flags Details | Formatted Diff | Diff
Patch v4 (17.19 KB, patch)
2010-02-02 07:20 PST, Yael
no flags Details | Formatted Diff | Diff
Patch v5 (17.38 KB, patch)
2010-02-02 11:56 PST, Yael
no flags Details | Formatted Diff | Diff
Patch v6 (41.28 KB, patch)
2010-02-09 14:46 PST, Yael
zimmermann: review-
Details | Formatted Diff | Diff
Patch v7 (29.82 KB, patch)
2010-02-16 12:46 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sidney San Martín 2010-01-13 15:48:52 PST
Created attachment 46515 [details]
Demo of SVG data URI in CSS

When an SVG is described with a data URI in CSS, it isn't displayed. An identical data URI works in an <img>.
Comment 1 Alexey Proskuryakov 2010-01-14 15:56:25 PST
Confirmed with r53269.
Comment 2 Yael 2010-01-29 06:50:40 PST
The failure is not due to the data URL, it is because we don't properly support SVG as background image, if the size of the SVG is smaller than the space it should fill.
Comment 3 Sidney San Martín 2010-01-29 09:08:39 PST
(In reply to comment #2)
> The failure is not due to the data URL, it is because we don't properly support
> SVG as background image, if the size of the SVG is smaller than the space it
> should fill.

Related to bug 12095?
Comment 4 Yael 2010-01-29 09:45:45 PST
(In reply to comment #3)
> (In reply to comment #2)
> > The failure is not due to the data URL, it is because we don't properly support
> > SVG as background image, if the size of the SVG is smaller than the space it
> > should fill.
> 
> Related to bug 12095?

I don't think so. I think that SVGImage::nativeImageForCurrentFrame() is not implemented correctly. I have a patch for it, but I want to test it some more before uploading it.
Comment 5 Yael 2010-01-29 13:10:32 PST
Created attachment 47731 [details]
Patch v1

I am pretty sure that I did not generate the expected test results properly. I would really appreciate if a reviewer could look at it and help me figure out how to generate the results properly. thanks!
Comment 6 Yael 2010-02-01 11:27:40 PST
Created attachment 47856 [details]
Patch v2

Updated test results.
Comment 7 Eric Seidel (no email) 2010-02-01 12:30:02 PST
Comment on attachment 47856 [details]
Patch v2

FloatRect has a constructor which takes a point and a size.  That would be cleaner than using size.width() size.height().

"r" is not a very good variable name.

Are you sure that rect() isn't already implemented in Image.h anyway?

Why should draw() be used explicitly instead of renderSubtreeToImage changed?
Comment 8 Yael 2010-02-01 13:08:31 PST
(In reply to comment #7)
> (From update of attachment 47856 [details])
> FloatRect has a constructor which takes a point and a size.  That would be
> cleaner than using size.width() size.height().
> 
> "r" is not a very good variable name.
> 
> Are you sure that rect() isn't already implemented in Image.h anyway?
> 
> Why should draw() be used explicitly instead of renderSubtreeToImage changed?

Thank you for the review, Eric. 
I chose to use draw() instead of modifying renderSubTreeToImage() to be consistent with the behavior when the SVG is bigger than the background it covers. I could probably use renderSubTreeToImage() if instead of passing the RenderView, I would walk the render tree, and find the correct renderer to pass to renderSubTreeToImage().
Comment 9 Yael 2010-02-01 18:20:17 PST
Created attachment 47895 [details]
Patch v3

Updated the patch to use Image::rect().
Comment 10 Yael 2010-02-02 07:20:50 PST
Created attachment 47930 [details]
Patch v4

I hope I added the test result image correctly this time.
Comment 11 Darin Adler 2010-02-02 10:00:25 PST
Comment on attachment 47930 [details]
Patch v4

Why is the test skipped on Mac again? Is that just because you don’t have pixel results for Mac yet?
Comment 12 Yael 2010-02-02 10:27:53 PST
(In reply to comment #11)
> (From update of attachment 47930 [details])
> Why is the test skipped on Mac again? Is that just because you don’t have pixel
> results for Mac yet?
Sorry, but generating the png image and adding it to the patch turned out to be very difficult for me. I do have the results, just don't know how to properly add them to the patch :-(
Comment 13 Yael 2010-02-02 11:56:55 PST
Created attachment 47960 [details]
Patch v5

Fix the conflicts in the various Skipped files.
Comment 14 Yael 2010-02-04 08:17:27 PST
Eric, does my answer in c#8 make sense to you?
thanks!
Comment 15 Eric Seidel (no email) 2010-02-04 11:08:07 PST
The reason why I questioned your use of draw() was that after this change it's not clear to me when code should use draw() and when code should use renderSubTreeToImage.

I'm not sure I even remember what draw() does.  I think it was for markers.  I remember writing renderSubTreeToImage for SVGImage (or maybe it was for patterns?) though.

I find it confusing that we have two functions that do similar things and it's not clear from reading the code when one should be used instead of the other.
Comment 16 Yael 2010-02-04 16:24:17 PST
(In reply to comment #15)
> The reason why I questioned your use of draw() was that after this change it's
> not clear to me when code should use draw() and when code should use
> renderSubTreeToImage.
> 
> I'm not sure I even remember what draw() does.  I think it was for markers.  I
> remember writing renderSubTreeToImage for SVGImage (or maybe it was for
> patterns?) though.
> 
> I find it confusing that we have two functions that do similar things and it's
> not clear from reading the code when one should be used instead of the other.

draw() goes through all the phases of drawing, while renderSubTreeToImage draws only the PaintPhaseForeground phase.
And like you said, renderSubTreeToImage is used for patterns and filter effects.

Ifis the area to is draw smaller than the size of the svg, we wold call draw(), not renderSubTreeToImage(), so I thought we should solve this bug by calling draw() also in the case that the SVG is smaller than the area it should cover.
Please see Image:drawTiled().
thanks.
Comment 17 Eric Seidel (no email) 2010-02-04 16:30:01 PST
Seems we should rename renderSubTreeToImage to indicate that it produces an image with only the foreground phase.

I'm not sure when only the foreground phase is useful.  Are filters not allowed inside <pattern> definitions?
Comment 18 Yael 2010-02-04 17:16:20 PST
(In reply to comment #17)
> Seems we should rename renderSubTreeToImage to indicate that it produces an
> image with only the foreground phase.
> 
> I'm not sure when only the foreground phase is useful.  Are filters not allowed
> inside <pattern> definitions?

What I meant to say is that SVGPatternElement::buildPattren and SVGPatternElement::buildPattern both call renderSubTreeToImage. We should probebly rename renderSubTreeToImage(). DO you have a suggestion for a new name?
Comment 19 Yael 2010-02-06 05:25:08 PST
(In reply to comment #17)
> Seems we should rename renderSubTreeToImage to indicate that it produces an
> image with only the foreground phase.
> 
> I'm not sure when only the foreground phase is useful.  Are filters not allowed
> inside <pattern> definitions?

According to http://www.w3.org/TR/SVG11/filters.html#FillPaint filters are allowed inside patterns, however the pattern in this bug is related to background image in the HTML, not SVG pattern.
Comment 20 Yael 2010-02-09 14:46:49 PST
Created attachment 48441 [details]
Patch v6

Updated the patch to latest baseline and added mac expected results.
Comment 21 Nikolas Zimmermann 2010-02-15 17:34:48 PST
Comment on attachment 48441 [details]
Patch v6

Hi Yael,

the patch looks almost perfect. Though you forgot to add the -expected.txt file.
I highly suggest to remove the text from the test, so you can safely add the -expected.txt file in svg/css/ directly.
There should be no need to skip other platforms than mac & qt.

Can you have a look?
Comment 22 Yael 2010-02-16 12:45:15 PST
(In reply to comment #21)
> (From update of attachment 48441 [details])
> Hi Yael,
> 
> the patch looks almost perfect. Though you forgot to add the -expected.txt
> file.
> I highly suggest to remove the text from the test, so you can safely add the
> -expected.txt file in svg/css/ directly.
> There should be no need to skip other platforms than mac & qt.
> 
> Can you have a look?

Thank you so much for your review. A new patch is coming soon.
Comment 23 Yael 2010-02-16 12:46:37 PST
Created attachment 48816 [details]
Patch v7

Added a text file with expected result and removed the text from the test file.
Comment 24 Nikolas Zimmermann 2010-02-16 16:06:00 PST
Comment on attachment 48816 [details]
Patch v7

Excellent, r=me.
Comment 25 WebKit Commit Bot 2010-02-16 19:54:29 PST
Comment on attachment 48816 [details]
Patch v7

Clearing flags on attachment: 48816

Committed r54865: <http://trac.webkit.org/changeset/54865>
Comment 26 WebKit Commit Bot 2010-02-16 19:54:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Simon Fraser (smfr) 2010-04-02 10:51:39 PDT
This caused bug 37028.