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>
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.
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).
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.
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!
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.
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
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
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.
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.
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.
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)
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 :)
Created attachment 153381 [details] Patch
Fix landed in r123171.
Reopening because of a mistake in the checkin.
Created attachment 153419 [details] Patch
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.
(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 ;-)
Committed r123215: <http://trac.webkit.org/changeset/123215>
That last commit was just a rebaseline. Comment #21 indicated that the changes were expected.
(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?