Bug 5146 - max-height/max-width not resizing images with correct aspect ratio
Summary: max-height/max-width not resizing images with correct aspect ratio
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Major
Assignee: Nobody
URL: http://www.yabbforum.com/community/Ya...
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-27 02:51 PDT by Andrew Aitken
Modified: 2006-02-24 14:05 PST (History)
4 users (show)

See Also:


Attachments
Example HTML Page (416 bytes, text/html)
2005-09-27 02:53 PDT, Andrew Aitken
no flags Details
Example Image (2.93 KB, image/png)
2005-09-27 02:56 PDT, Andrew Aitken
no flags Details
Example HTML (397 bytes, text/html)
2005-09-27 02:57 PDT, Andrew Aitken
no flags Details
patch (4.93 KB, patch)
2006-01-29 12:17 PST, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
image needed for test cases (199 bytes, image/png)
2006-01-29 17:27 PST, Sam Weinig
no flags Details
patch 2 (8.06 KB, patch)
2006-01-29 17:57 PST, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
patch 3 (10.56 KB, patch)
2006-02-12 20:35 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
patch 4 (10.59 KB, patch)
2006-02-12 22:16 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
patch 5 (18.15 KB, patch)
2006-02-15 21:02 PST, Sam Weinig
hyatt: review-
Details | Formatted Diff | Diff
2nd file needed for test cases (91 bytes, image/png)
2006-02-15 21:04 PST, Sam Weinig
no flags Details
patch 6 (17.58 KB, patch)
2006-02-17 14:02 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
patch (take 7) (18.50 KB, patch)
2006-02-21 10:44 PST, Sam Weinig
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Aitken 2005-09-27 02:51:58 PDT
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
Comment 1 Andrew Aitken 2005-09-27 02:53:30 PDT
Created attachment 4061 [details]
Example HTML Page

This is the necessary HTML to demonstrate this issue.
Comment 2 Andrew Aitken 2005-09-27 02:56:08 PDT
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.
Comment 3 Andrew Aitken 2005-09-27 02:57:33 PDT
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.
Comment 4 Maks Orlovich 2005-09-28 06:15:50 PDT
This was recently fixed in our tree, FYI: 
 
http://lists.kde.org/?l=kde-commits&m=112437812001222&w=2 
  
Comment 5 Sam Weinig 2006-01-29 12:17:02 PST
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).
Comment 6 Darin Adler 2006-01-29 14:15:40 PST
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.
Comment 7 Sam Weinig 2006-01-29 17:27:08 PST
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.
Comment 8 Sam Weinig 2006-01-29 17:57:19 PST
Created attachment 6089 [details]
patch 2

2 test cases added
Comment 9 Darin Adler 2006-01-29 20:04:51 PST
Comment on attachment 6089 [details]
patch 2

Looks good, r=me
Comment 10 Darin Adler 2006-02-01 21:26:44 PST
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.
Comment 11 Sam Weinig 2006-02-12 20:35:18 PST
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.
Comment 12 Dave Hyatt 2006-02-12 20:46:35 PST
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.
Comment 13 Sam Weinig 2006-02-12 22:16:42 PST
Created attachment 6452 [details]
patch 4

forgot to addback a call prefix, should work this time
Comment 14 Sam Weinig 2006-02-12 22:37:41 PST
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.
Comment 15 Sam Weinig 2006-02-13 05:12:46 PST
Comment on attachment 6452 [details]
patch 4

removing review request flag until I smooth out some more kinks
Comment 16 Sam Weinig 2006-02-15 21:02:10 PST
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.
Comment 17 Sam Weinig 2006-02-15 21:04:08 PST
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.
Comment 18 Eric Seidel (no email) 2006-02-15 21:11:58 PST
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.
Comment 19 Sam Weinig 2006-02-15 21:31:07 PST
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 :(
Comment 20 Darin Adler 2006-02-16 09:35:18 PST
(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.
Comment 21 Dave Hyatt 2006-02-17 13:05:38 PST
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!
Comment 22 Sam Weinig 2006-02-17 14:02:06 PST
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.
Comment 23 Maciej Stachowiak 2006-02-20 06:31:53 PST
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.
Comment 24 Sam Weinig 2006-02-20 06:58:23 PST
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?  
Comment 25 Sam Weinig 2006-02-20 07:44:58 PST
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.
Comment 26 Darin Adler 2006-02-20 10:05:04 PST
(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.
Comment 27 Sam Weinig 2006-02-21 10:44:05 PST
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.
Comment 28 Dave Hyatt 2006-02-21 14:53:09 PST
Comment on attachment 6645 [details]
patch (take 7)

r=me
Comment 29 Geoffrey Garen 2006-02-24 14:05:12 PST
Landed.