Bug 33630 - SVG not rendered as background-image from data URI
: SVG not rendered as background-image from data URI
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-13 15:48 PST by
Modified: 2010-04-02 10:51 PST (History)


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 Review Patch | Details | Formatted Diff | Diff
Patch v2 (5.60 KB, patch)
2010-02-01 11:27 PST, Yael
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch v3 (5.55 KB, patch)
2010-02-01 18:20 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch v4 (17.19 KB, patch)
2010-02-02 07:20 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch v5 (17.38 KB, patch)
2010-02-02 11:56 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch v6 (41.28 KB, patch)
2010-02-09 14:46 PST, Yael
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
Patch v7 (29.82 KB, patch)
2010-02-16 12:46 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2010-01-13 15:48:52 PST
Created an attachment (id=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 From 2010-01-14 15:56:25 PST -------
Confirmed with r53269.
------- Comment #2 From 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 From 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 From 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 From 2010-01-29 13:10:32 PST -------
Created an attachment (id=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 From 2010-02-01 11:27:40 PST -------
Created an attachment (id=47856) [details]
Patch v2

Updated test results.
------- Comment #7 From 2010-02-01 12:30:02 PST -------
(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?
------- Comment #8 From 2010-02-01 13:08:31 PST -------
(In reply to comment #7)
> (From update of attachment 47856 [details] [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 From 2010-02-01 18:20:17 PST -------
Created an attachment (id=47895) [details]
Patch v3

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

I hope I added the test result image correctly this time.
------- Comment #11 From 2010-02-02 10:00:25 PST -------
(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?
------- Comment #12 From 2010-02-02 10:27:53 PST -------
(In reply to comment #11)
> (From update of attachment 47930 [details] [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 From 2010-02-02 11:56:55 PST -------
Created an attachment (id=47960) [details]
Patch v5

Fix the conflicts in the various Skipped files.
------- Comment #14 From 2010-02-04 08:17:27 PST -------
Eric, does my answer in c#8 make sense to you?
thanks!
------- Comment #15 From 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 From 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 From 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 From 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 From 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 From 2010-02-09 14:46:49 PST -------
Created an attachment (id=48441) [details]
Patch v6

Updated the patch to latest baseline and added mac expected results.
------- Comment #21 From 2010-02-15 17:34:48 PST -------
(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?
------- Comment #22 From 2010-02-16 12:45:15 PST -------
(In reply to comment #21)
> (From update of attachment 48441 [details] [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 From 2010-02-16 12:46:37 PST -------
Created an attachment (id=48816) [details]
Patch v7

Added a text file with expected result and removed the text from the test file.
------- Comment #24 From 2010-02-16 16:06:00 PST -------
(From update of attachment 48816 [details])
Excellent, r=me.
------- Comment #25 From 2010-02-16 19:54:29 PST -------
(From update of attachment 48816 [details])
Clearing flags on attachment: 48816

Committed r54865: <http://trac.webkit.org/changeset/54865>
------- Comment #26 From 2010-02-16 19:54:34 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #27 From 2010-04-02 10:51:39 PST -------
This caused bug 37028.