897787 - SVG image as background is flattened horizontally

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Loic]

Attachment: testcase-svg-background.html

STR: open the testcase.

Result: the SVG image is completely flattened horizontally.

Regression range:
good=2013-07-24
bad=2013-07-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2268ff80683a&tochange=5ceea82a79c7

Suspected: 
Nicholas Cameron — Bug 700926; reshuffle background image drawing. r=roc
Nicholas Cameron — Bug 700926. Refactor image sizing to be closer to the spec and not tied to backgrounds. r=roc

Comment 1

[Loic]

Sorry:
good=2013-07-22
bad=2013-07-23

Comment 2

[Nick Cameron [:nrc]]

I believe this is an error in the test case:

background-size: 300px;

should be

background-size: 300px 300px;

The new code is spec-compliant - "The first value gives the width of the corresponding image, the second value its height. If only one value is given the second is assumed to be ‘auto’.", from http://www.w3.org/TR/css3-background/#the-background-size

Comment 3

[Zéfling]

The SVG is not render normally. The width and height of the SVG image is correct, but not the content.

Comment 4

[Jeff Walden [:Waldo]]

The previous rendering was correct.  Treating the background size as:

  background-size: 300px auto;

triggers rendering with width 300px, height the value that preserves the intrinsic ratio, if any.  The image has a viewBox attribute on it, so it has an intrinsic ratio.  The old rendering was correct.

I'm very, very curious as to how all the vector background-size reftests we have in layout/reftests/backgrounds/vector didn't catch this.  Perhaps those all pass because I wrote them as no-repeat backgrounds, while this background repeats?

Comment 5

[Nick Cameron [:nrc]]

I don't see anything obvious from the patch, but I'll look into it.

Comment 6

[Zéfling]

On my website ( http://ikilote.net/ ), the problem is same with this :

	background: url("../../Image/Logo.svg") no-repeat scroll 0 13px / 311px auto transparent;

Comment 7

[Nick Cameron [:nrc]]

Whilst fixing this, I come across the question of how to size images which are -moz-element to SVG elements (that is gradients or patterns, not entire images). These have no intrinsic size, so we have to pick some 'natural' size to render the image at. There are basically two options - the background positing area's size (i.e., the entire size of the element who's background we are filling) or the background painting area's size (aka, the specified size, i.e., the size specified by background-size).

The old code uses the former, but the latter seems more natural to me. The current code is broken, but when fixed, the latter falls out more naturally.

As far as I can see this is not spec'ed, it falls between the cracks of the CSS element spec and the SVG specs, but I might have missed something somewhere.

linear-gradient images (for comparison) use the latter, that is the specified area.

We have two tests which test for the former behaviour, these were added by mstange as part of bug 506826, but I can't find any discussion there. The more interesting test is:

http://dxr.mozilla.org/mozilla-central/source/layout/reftests/image-element/pattern-html-02.html

(the other uses a gradient, but is kind of similar). In this test the second div renders with a background of two large squares because the image is rendered into the background positioning area and _then_ scaled by the 300% size of the background size. I believe it should render as 18 small squares because it should be rendered using the background size after scaling.

mstange, roc: am I missing something? Do you have other opinions on this?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with you --- use the background painting area's size.

Comment 9

[Markus Stange [:mstange]]

My original intention was to have SVG gradients and patterns work just like CSS gradients. It looks like I had the wrong idea how background-size applies to CSS gradients. I agree that background-size should determine the background filling area that's used to render SVG gradients and patterns at their original size, and not as a scale that's applied after rendering them into the whole background area (which seems to be what it's doing at the moment).

Comment 10

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=57d6a4ec12f6

Comment 11

[Nick Cameron [:nrc]]

Attachment: fix

Comment 12

[Nick Cameron [:nrc]]

Attachment: adjust existing tests

Comment 13

[Nick Cameron [:nrc]]

Attachment: new test to catch the squishing issue

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 784016 [details] [diff] [review]
adjust existing tests

Review of attachment 784016 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/image-element/reftest.list
@@ +39,5 @@
>  fuzzy(1,9674) random-if(!cocoaWidget) == gradient-html-06b.html gradient-html-06c.html
>  == gradient-html-06c.html gradient-html-06d.html
>  == gradient-html-06d.html gradient-html-06e.html
>  random-if(!cocoaWidget) fuzzy-if(azureQuartz,1,11367) == gradient-html-07a.html gradient-html-07b.html
> +fuzzy-if(true,1,16900) == gradient-html-07c.html gradient-html-07d.html

Just use fuzzy() instead of fuzzy-if()

Comment 15

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/650bf3681eee
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa64b53b0ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/95dc8e7a8a8d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/650bf3681eee
https://hg.mozilla.org/mozilla-central/rev/caa64b53b0ba
https://hg.mozilla.org/mozilla-central/rev/95dc8e7a8a8d