Bug 5146 – max-height/max-width not resizing images with correct aspect ratio

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andrew Aitken]

When an image has only one of max-height or max-width specified - the spec states (I think) that the 
image should be resized so that the image continues to have the same aspect ratio.

In Safari, it doesn't resize the image with the correct ratio - so images can appear "squashed" when you 
specify only a max-width, or max-height on their own.

Relevant W3 Page: http://www.w3.org/TR/CSS21/visudet.html#min-max-widths


Works as expected in the following (Mac OS X 10.4.2):
Camino 1.0a1
Opera 8.5.2173
Firefox 1.5 beta 1

Works as Safari (suspected buggy behaviour)
Firefox 1.07

[Andrew Aitken]

Created attachment 4061 [details]
Example HTML Page

This is the necessary HTML to demonstrate this issue.

[Andrew Aitken]

Created attachment 4062 [details]
Example Image

This image is naturally a rectangle 200x400 pixels. When viewed through the
example HTML page, Safari "squishes" the image, as only max-width is specified.

[Andrew Aitken]

Created attachment 4063 [details]
Example HTML

This is an example of the bug. The image will appear "squished" in Safari,
while most other browsers resize the image keeping the correct aspect ratio.

[Maks Orlovich]

This was recently fixed in our tree, FYI: 
 
http://lists.kde.org/?l=kde-commits&m=112437812001222&w=2 
  

[Sam Weinig]

Created attachment 6081 [details]
patch

This is a patch is taken from Allan Sandfeld Jensen's (carewolf) patch of a similar bug from the kde side (http://bugs.kde.org/show_bug.cgi?id=120107).  I changed almost nothing, so I am not adding any copyright for myself, but I did add carewolfs name to the top (even though it is not there in the khtml version).

[Darin Adler]

Comment on attachment 6081 [details]
patch

Looks really good. Thanks for doing this!

But we also need a layout test in the patch to land the test. The tests attached to the bug look pretty good, we just need to turn them into layout tests and include them along with expected results in the patch.

I also think that these lines:

+    
+    if (width > maxW)
+        width = maxW;
+    
+    if (width < minW)
+        width = minW;
+    
+    return width;

would read better if it instead it just said:

    return kMax(minW, kMin(width, maxW));

Setting review- due to the lack of a layout test.

[Sam Weinig]

Created attachment 6087 [details]
image needed for test cases

Image intended to go in fast/replaced/resources/square-blue-100x100.png for use in test cases.

[Sam Weinig]

Created attachment 6089 [details]
patch 2

2 test cases added

[Darin Adler]

Comment on attachment 6089 [details]
patch 2

Looks good, r=me

[Darin Adler]

Comment on attachment 6089 [details]
patch 2

I went to land this patch, and discovered that the test case maxwidth-percent.html fails with the patch.

I debugged a bit and found that when computing the width, RenderBox::calcReplacedWidthUsing(MaxWidth) calls containingBlockWidth() and gets 0, so it returns intrinsicWidth, 100, instead of 50% of the containing block's width. The reason containingBlockWidth() gets 0 is that usesLineWidth() returns true.

It's possible this fix worked fine in the KDE tree but we need more work to get it to do the right thing.

[Sam Weinig]

Created attachment 6447 [details]
patch 3

This patch fixes all the issues the last one had by adding a change to the calcMinMaxWidth() function in RenderReplaced that I forgot in the previous one.  I should note that this patch does not add correct functionality for max/min height/width for absolutly positioned images, but it also has no negative affect on absolutly position element either.  The code for absolutly positioned element is handled a little different and as it stands is quite messy.  In fact most of RenderBox up through RenderReplaced and RenderImage could use some love as the code paths are quite unclear at best.

[Dave Hyatt]

Comment on attachment 6447 [details]
patch 3

Can you explain this?

+    case Percent: {
+        const int cw = containingBlockWidth();
+        if (cw > 0)
+            return calcContentBoxWidth(h.minWidth(cw));
+    }
+    // fall through


I do not understand why a containg block width of 0 would suddenly cause you to use intrinsic width instead.

[Sam Weinig]

Created attachment 6452 [details]
patch 4

forgot to addback a call prefix, should work this time

[Sam Weinig]

hyatt,
Now that you mention it, I can't really explain why it is there other that I was trying to match the behavior that the corresponding width function has already.  But I'm not clear why the width has it either.  I'll have to experiment unless anyone knows of reason why this would be necessary.

[Sam Weinig]

Comment on attachment 6452 [details]
patch 4

removing review request flag until I smooth out some more kinks

[Sam Weinig]

Created attachment 6526 [details]
patch 5

In this latest version of the patch, I've cleaned up the changes and tried to focus them into RenderImage as to avoid unintended side effects with RenderReplaced.  There is still no functionality for absolutly positioned images.  I've also added more test cases to more accuratly show all of the changes.

[Sam Weinig]

Created attachment 6527 [details]
2nd file needed for test cases

Adding another image that is used in the new test cases involving min-width and min-height.

[Eric Seidel (no email)]

I wonder if there is any possiblity long term of sharing code with similar changes needed in SVG:
http://bugzilla.opendarwin.org/show_bug.cgi?id=5966

Currently SVG does not use the "Length" object, which would likely make any sharing now impossible.  SVG Images are a bit more configurable (and thus complicated) in this area.

[Sam Weinig]

Going forward sharing the code SVG images might be really helpful especially because some of the CSS 3 box model stuff includes the extra functionality.  But perhaps I am getting ahead of myself.  First someone really needs to rename the width and minWidth functions in the Length class.  They really confuse me when dealing with heights :(

[Darin Adler]

(In reply to comment #19)
> First someone really needs to rename the width and minWidth functions in the Length class.
> They really confuse me when dealing with heights :(

Sounds good. Please propose some new names -- we can discuss then on IRC -- and then submit a patch to rename them.

[Dave Hyatt]

Comment on attachment 6526 [details]
patch 5

Everything looks fine except for this section:

@@ -1216,15 +1213,15 @@ int RenderBox::calcReplacedWidthUsing(Wi
     else
         w = style()->maxWidth();
     
-    switch (w.type) {
+    switch (w.type)
+    {
     case Fixed:
         return calcContentBoxWidth(w.value);
-    case Percent: {
+    case Percent: 
         const int cw = containingBlockWidth();
         if (cw > 0)
             return calcContentBoxWidth(w.minWidth(cw));
-    }
-    // fall through
+    //fall through
     default:
         return intrinsicWidth();
     }

You changed things here to no long match our style guidelines.  Can you just undo that section and re-attach the patch? Thanks!

[Sam Weinig]

Created attachment 6567 [details]
patch 6

I reverted that section back, but I am not quite clear as to what the rule was being broken.  If you could elaborate or specify which part of the guidlines it broke it would help me in the future.

[Maciej Stachowiak]

This part violated the style guidelines:

-    switch (w.type) {
+    switch (w.type)
+    {

Our coding style guidelines call for the open brace to be on the same line as switch keyword.

[Sam Weinig]

I see (but in my defence, I did notice that change was only made in the last couple of days ;)  ), that means that there is actually another violation I made just a coulple lines down in the patch as well.  While I am changing it to be to spec, would it be ok for me to change the lines

-    case Percent: {
+    case Percent: 
         const int cw = containingBlockWidth();
         if (cw > 0)
             return calcContentBoxWidth(w.minWidth(cw));
-    }
-    // fall through
+    //fall through

as I noticed in general, braces are no used inside switch statements in the code?  

[Sam Weinig]

For that matter, what is the correct style (for webkit) for 'case' statments in a 'switch' statement?  Should braces be used at all?  If so, should the open brace be on the same line as the case or on the next line? How should indenting work?  Also, are their rules for how to style multiple case statements relating to the same thing?  I know not every situation can be specified in the style guidelines but perhaps answering some of the questions might help consistency, and appease the obsessively compulsive of us out there.

[Darin Adler]

(In reply to comment #24)
> -    case Percent: {
> +    case Percent: 
>          const int cw = containingBlockWidth();
>          if (cw > 0)
>              return calcContentBoxWidth(w.minWidth(cw));
> -    }
> -    // fall through
> +    //fall through
>       default:

It's true that we don't always use braces for case statements.

But in this case, the braces are needed to scope the "cw" variable. If you don't have braces, jumping to the default case puts you in a place where "cw" is in scope, but not initialized. I believe gcc gives a warning about this.

So the braces are there for a reason and should not be removed. But it's true that many case statements won't have braces.

Also, I don't see the point in removing the space after the "//" in the comment. We always use a space there although it's not called out in the guidelines.

[Sam Weinig]

Created attachment 6645 [details]
patch (take 7)

After fleshing out the finer points of switch statement styling with darin, here is take 7 of the patch.  Hopefully ready for primetime.

[Dave Hyatt]

Comment on attachment 6645 [details]
patch (take 7)

r=me

[Geoffrey Garen]

Landed.