Bug 91474 - REGRESSION (r112229): SVG images don’t scale proportionally when constrained by percentage max-width and max-height
Summary: REGRESSION (r112229): SVG images don’t scale proportionally when constrained ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-07-17 00:29 PDT by mitz
Modified: 2012-07-20 10:15 PDT (History)
10 users (show)

See Also:


Attachments
Test case (312 bytes, text/html)
2012-07-17 00:29 PDT, mitz
no flags Details
Updated Test Case (362 bytes, text/html)
2012-07-17 17:51 PDT, Dean Jackson
no flags Details
Updated updated test case (3.19 KB, text/html)
2012-07-18 16:33 PDT, Dean Jackson
no flags Details
Patch (15.78 KB, patch)
2012-07-18 21:15 PDT, Dean Jackson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (545.65 KB, application/zip)
2012-07-18 23:01 PDT, WebKit Review Bot
no flags Details
Patch (22.48 KB, patch)
2012-07-19 20:25 PDT, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff
Patch (16.15 KB, patch)
2012-07-19 22:47 PDT, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-07-17 00:29:21 PDT
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.
Comment 1 mitz 2012-07-17 00:30:25 PDT
<rdar://problem/11888490>
Comment 2 Dean Jackson 2012-07-17 17:51:28 PDT
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.
Comment 3 Dean Jackson 2012-07-17 17:53:23 PDT
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).
Comment 4 Dean Jackson 2012-07-18 16:33:35 PDT
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.
Comment 5 Dean Jackson 2012-07-18 16:35:46 PDT
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.
Comment 6 mitz 2012-07-18 19:01:03 PDT
(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!
Comment 7 Dean Jackson 2012-07-18 21:15:22 PDT
Created attachment 153173 [details]
Patch
Comment 8 mitz 2012-07-18 21:50:20 PDT
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.
Comment 9 WebKit Review Bot 2012-07-18 23:01:39 PDT
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
Comment 10 WebKit Review Bot 2012-07-18 23:01:44 PDT
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
Comment 11 Nikolas Zimmermann 2012-07-19 00:11:51 PDT
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.
Comment 12 Simon Fraser (smfr) 2012-07-19 10:16:17 PDT
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.
Comment 13 mitz 2012-07-19 10:56:35 PDT
(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.
Comment 14 Dean Jackson 2012-07-19 12:34:17 PDT
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.
Comment 15 Dean Jackson 2012-07-19 12:46:04 PDT
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)
Comment 16 Dean Jackson 2012-07-19 13:26:05 PDT
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 :)
Comment 17 Dave Hyatt 2012-07-19 20:25:56 PDT
Created attachment 153381 [details]
Patch
Comment 18 Dave Hyatt 2012-07-19 20:59:24 PDT
Fix landed in r123171.
Comment 19 Dave Hyatt 2012-07-19 22:38:55 PDT
Reopening because of a mistake in the checkin.
Comment 20 Dave Hyatt 2012-07-19 22:47:00 PDT
Created attachment 153419 [details]
Patch
Comment 21 Dave Hyatt 2012-07-19 23:14:15 PDT
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.
Comment 22 Nikolas Zimmermann 2012-07-20 00:42:36 PDT
(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 ;-)
Comment 23 Stephen Chenney 2012-07-20 07:31:10 PDT
Committed r123215: <http://trac.webkit.org/changeset/123215>
Comment 24 Stephen Chenney 2012-07-20 07:32:15 PDT
That last commit was just a rebaseline. Comment #21 indicated that the changes were expected.
Comment 25 Simon Fraser (smfr) 2012-07-20 10:15:58 PDT
(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?