Bug 91474 – REGRESSION (r112229): SVG images don’t scale proportionally when constrained by percentage max-width and max-height

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

Created attachment 152706 [details]
Test case

After <http://trac.webkit.org/r112229>, an SVG image with width and height set to auto, max-width and max-height set to 100% does not maintain its aspect ratio when shrinking to fit in the constraints. To reproduce this bug, open the attached test case. The SVG image (yellow) should be square. In Firefox, and in WebKit before r112229 it is. In TOT, it is longer than it is wide.

<rdar://problem/11888490>

[Dean Jackson]

Created attachment 152888 [details]
Updated Test Case

Updated test case to show that the SVG *content* retains its aspect ratio, but the image element doesn't use the intrinsic width and height. Basically the img element resizes to the wrong dimensions and then draws the SVG as it should.

[Dean Jackson]

Instead, it uses the intrinsic height (300) but the bounding width (200), rather than using the bounding width (200) and equivalent bounding height (200 to maintain aspect).

[Dean Jackson]

Created attachment 153138 [details]
Updated updated test case

New test which has two identical divs side by side, each with an img child. The first points to a 300x300 SVG. The second points to a 300x300 png.

The png image is a checkerboard with transparency so you can see the size of the <img> container.

[Dean Jackson]

The issue is that RenderReplaced::computeIntrinsicRatioInformationForRenderBox has a content renderer in the SVG case, but not in the PNG case.

This means the SVG path calls contentRenderer->computeIntrinsicRatioInformation which returns 300x300, and the PNG path calls this->computeIntrinsicRatioInformation which returns 300x200.

I don't know why 300x200 is the correct answer, but later code sets the width to the right value, making it 200x200.

(In reply to comment #5)
> 
> I don't know why 300x200 is the correct answer

Me neither. This was extremely confusing while trying to understand what was going on in this bug!

[Dean Jackson]

Created attachment 153173 [details]
Patch

Comment on attachment 153173 [details]
Patch

Dean and I discussed the patch on IRC. My concern is that computeReplacedLogicalHeight() is not the place to make a distinction between elements that have a content renderer (read: SVG images) and elements that don’t (read: bitmap images), since this function just follows the section of the CSS specification that it’s quoting in the comments, and the spec doesn’t make the same distinction.

[WebKit Review Bot]

Comment on attachment 153173 [details]
Patch

Attachment 153173 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13276795

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml

[WebKit Review Bot]

Created attachment 153183 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Nikolas Zimmermann]

Note that the comments in RenderReplaced.cpp, are a 1:1 copy from CSS 2.1. See http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width and inline-replaced-height.

<quote>
10.6.2 Inline replaced elements, block-level replaced elements in normal flow, 'inline-block' replaced elements in normal flow and floating replaced elements

If 'margin-top', or 'margin-bottom' are 'auto', their used value is 0.

If 'height' and 'width' both have computed values of 'auto' and the element also has an intrinsic height, then that intrinsic height is the used value of 'height'.
</quote>

Of course this doesn't talk about the constraining properties like max-width/max-height, which is probably the cause of this bug.

I overlooked Dans comments, who shares my concern:
> Dean and I discussed the patch on IRC. My concern is that computeReplacedLogicalHeight() is not the place to make a distinction between elements that have a content renderer (read: SVG images) and elements that don’t (read: bitmap images), since this function just follows the section of the CSS specification that it’s quoting in the comments, and the spec doesn’t make the same distinction.

We should keep in mind that this section of the spec is mostly ignored by all browsers. We're the first to actually handle the size negotiation between SVG <-> CSS, so it's also likely that the spec is partly wrong, and we need to take more things into account to correctly determine the size.

Needs more investigation I fear.

[Simon Fraser (smfr)]

There's also some discussion of size negotiation here:
<http://dev.w3.org/csswg/css3-images/#object-negotiation>

Maybe ask on www-style what the correct behavior is? fantasai knows this pretty well.

(In reply to comment #12)
> There's also some discussion of size negotiation here:
> <http://dev.w3.org/csswg/css3-images/#object-negotiation>

Computing the size of the replaced element in the test case is fully specified in <http://www.w3.org/TR/CSS21/visudet.html> as far as I can tell. The behavior in this regard prior to r112229 was correct. The above link doesn’t add to or contradict anything in <http://www.w3.org/TR/CSS21/visudet.html>. I’m not sure what there is to ask.

[Dean Jackson]

I had two choices to investigate (maybe more, but here are two):

A. RenderReplaced::computeIntrinsicRatioInformationForRenderBox returns 300x200 for the PNG image (which is actually 300x300). Why is this the "correct" answer? At the very least, the name of the function is a lie.

B. Why doesn't the 300x300 returned for the SVG image get set to 200x200 in computeReplacedLogicalWidth/Height? Instead only width is changed (because the height of 300 is less than the available 400).

I chose to investigate B because it seemed the least intrusive. However, after I did that, Dan pointed out that I'd changed the spot that is clearly following the specification to the letter. We then agreed that it wasn't the right solution even though it fixes the bug.

I'm looking into A now, but I fear it will be a more significant change.

[Dean Jackson]

It seems like we are not following the 2nd entry in the table under
http://www.w3.org/TR/CSS2/visudet.html#min-max-widths

Where resolved height should be max(max-width * h/w, min-height)

[Dean Jackson]

In particular, http://www.w3.org/TR/CSS21/visudet.html#min-max-heights
says:

[[[
The following algorithm describes how the two properties influence the used value of the 'height' property:

1. The tentative used height is calculated (without 'min-height' and 'max-height') following the rules under "Calculating heights and margins" above.
2. If this tentative height is greater than 'max-height', the rules above are applied again, but this time using the value of 'max-height' as the computed value for 'height'.

]]]

In our case step 1 would produce a height of 300. Step 2 would be skipped since max-height calculates to 400. But there is another rule just after this:

[[[
However, for replaced elements with both 'width' and 'height' computed as 'auto', use the algorithm under Minimum and maximum widths above to find the used width and height.
]]]

which means we should follow the rules in http://www.w3.org/TR/CSS2/visudet.html#min-max-widths

I think :)

[Dave Hyatt]

Created attachment 153381 [details]
Patch

[Dave Hyatt]

Fix landed in r123171.

[Dave Hyatt]

Reopening because of a mistake in the checkin.

[Dave Hyatt]

Created attachment 153419 [details]
Patch

[Dave Hyatt]

2nd fix landed in r123183. I went ahead and changed roundedIntSize to flooredIntSize since more layout tests favored that. I had to reset one result because of a precision error introduced by this change. As a follow-up, we're going to need to fix m_intrinsicSize to have subpixel precision to get rid of the rounding errors I have now introduced.

[Nikolas Zimmermann]

(In reply to comment #21)
> 2nd fix landed in r123183. I went ahead and changed roundedIntSize to flooredIntSize since more layout tests favored that. I had to reset one result because of a precision error introduced by this change. As a follow-up, we're going to need to fix m_intrinsicSize to have subpixel precision to get rid of the rounding errors I have now introduced.

Thanks Dave for the quick fix! We obviously missed to take http://www.w3.org/TR/CSS21/visudet.html#min-max-heights into account, in the original checkin of the size negotiation code. Luckily you r+ it, so I feel less ashamed ;-)

[Stephen Chenney]

Committed r123215: <http://trac.webkit.org/changeset/123215>

[Stephen Chenney]

That last commit was just a rebaseline. Comment #21 indicated that the changes were expected.

[Simon Fraser (smfr)]

(In reply to comment #21)
> 2nd fix landed in r123183. I went ahead and changed roundedIntSize to flooredIntSize since more layout tests favored that. I had to reset one result because of a precision error introduced by this change. As a follow-up, we're going to need to fix m_intrinsicSize to have subpixel precision to get rid of the rounding errors I have now introduced.

What's the bug for that?