Bug 33630 – SVG not rendered as background-image from data URI

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sidney San Martín]

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>.

[Alexey Proskuryakov]

Confirmed with r53269.

[Yael]

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.

[Sidney San Martín]

(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?

[Yael]

(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.

[Yael]

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!

[Yael]

Created attachment 47856 [details]
Patch v2

Updated test results.

[Eric Seidel (no email)]

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?

[Yael]

(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().

[Yael]

Created attachment 47895 [details]
Patch v3

Updated the patch to use Image::rect().

[Yael]

Created attachment 47930 [details]
Patch v4

I hope I added the test result image correctly this time.

[Darin Adler]

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?

[Yael]

(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 :-(

[Yael]

Created attachment 47960 [details]
Patch v5

Fix the conflicts in the various Skipped files.

[Yael]

Eric, does my answer in c#8 make sense to you?
thanks!

[Eric Seidel (no email)]

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.

[Yael]

(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.

[Eric Seidel (no email)]

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?

[Yael]

(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?

[Yael]

(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.

[Yael]

Created attachment 48441 [details]
Patch v6

Updated the patch to latest baseline and added mac expected results.

[Nikolas Zimmermann]

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?

[Yael]

(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.

[Yael]

Created attachment 48816 [details]
Patch v7

Added a text file with expected result and removed the text from the test file.

[Nikolas Zimmermann]

Comment on attachment 48816 [details]
Patch v7

Excellent, r=me.

[WebKit Commit Bot]

Comment on attachment 48816 [details]
Patch v7

Clearing flags on attachment: 48816

Committed r54865: <http://trac.webkit.org/changeset/54865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This caused bug 37028.