Bug 120107 - [testcase] wrong interpretation of "max-height" (CSS)
Summary: [testcase] wrong interpretation of "max-height" (CSS)
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml renderer (show other bugs)
Version: unspecified
Platform: Unlisted Binaries Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-14 16:36 UTC by Pierre Schmitz
Modified: 2006-01-16 19:59 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to match Firefox (3.75 KB, patch)
2006-01-14 22:55 UTC, Allan Sandfeld
Details
testcase (33.22 KB, application/x-gzip)
2006-01-16 19:59 UTC, Pierre Schmitz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Schmitz 2006-01-14 16:41:05 UTC
Version:            (using KDE KDE 3.5.0)
Installed from:    Unspecified Linux
Compiler:          gcc-Version 4.0.3 20051222 (prerelease) Ziel: i686-pc-linux-gnu Konfiguriert mit: ../gcc-4.0-20051222/configure --host=i686-pc-linux-gnu --build=i686-pc-linux-gnu --prefix=/usr --enable-shared --enable-languages=c,c++,objc --enable-threads=posix --enable-__cxa_atexit Thread-Modell: posix 
OS:                Linux

Setting the "max-height"-attribute to images results in a wrong resized image when the image is larger than the "max-height"-value.

I have created a simple test-case which can be found at http://www.laber-land.de/~pierre/khtml/testcase.html

First it shows an image which is resized due to the max-height-attribute and then you see the picture in its original size. (You might want to see how Firefox 1.5 renders this)
Comment 1 Allan Sandfeld 2006-01-14 18:21:03 UTC
Actually Firefox rendering is completely wrong, but our rendering is wrong too. 

For some reason the image has lost its aspects which should remain the same unless width and height are both set.
Comment 2 Pierre Schmitz 2006-01-14 18:48:59 UTC
What do you mean with "Firefox rendering is completely wrong". I tested with Firefox 1.07 which renders like Konqueror. Since Version 1.5 it looks allright to me. 

I don`t know if it does help but this happens to everey type of image.
Comment 3 Allan Sandfeld 2006-01-14 19:25:34 UTC
I think the opera rendering is right.

Max-height/width should not change the aspects of the image. I will look it up though.
Comment 4 Allan Sandfeld 2006-01-14 22:55:09 UTC
Created attachment 14254 [details]
Patch to match Firefox

Okay, the Firefox rendering is slightly better, but still broken. Doing it
correct requires a lot of more constraint cases to be auto-detection.

The attached patch will match Firefox and Safari behavior.
Comment 5 Allan Sandfeld 2006-01-15 00:14:25 UTC
SVN commit 498206 by carewolf:

Respect min/max sizes and aspect ratios at the same time.
Matches the CSS 2.1 standard and Opera, but differs from Firefox and Safari.
BUG: 120107


 M  +5 -0      ChangeLog  
 M  +72 -17    rendering/render_image.cpp  
 M  +6 -0      rendering/render_image.h  


--- branches/KDE/3.5/kdelibs/khtml/ChangeLog #498205:498206
@@ -1,3 +1,8 @@
+2006-01-14  Allan Sandfeld Jensen <kde@carewolf.com>
+
+        * rendering/render_image.cpp (calcReplacedWidth,calcReplacedHeight):
+        Respect both min/max sizes and aspect-ratio when resizing.
+
 2006-01-03  Allan Sandfeld Jensen <kde@carewolf.com>
 
         * css/cssparser.cpp: Allow content: normal | none
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_image.cpp #498205:498206
@@ -244,7 +244,7 @@
         static QPixmap *loadingIcon;
         QColor bg = khtml::retrieveBackgroundColor(this);
         QColor fg = khtml::hasSufficientContrast(Qt::gray, bg) ? Qt::gray :
-                    (hasSufficientContrast(Qt::white, bg) ? Qt::white : Qt::black);    
+                    (hasSufficientContrast(Qt::white, bg) ? Qt::white : Qt::black);
 	paintInfo.p->setPen(QPen(fg, 1));
 	paintInfo.p->setBrush( Qt::NoBrush );
 	paintInfo.p->drawRect(_tx, _ty, m_width, m_height);
@@ -255,7 +255,7 @@
             }
             paintInfo.p->drawPixmap(_tx + 4, _ty + 4, *loadingIcon, 0, 0, m_width - 5, m_height - 5);
         }
-        
+
     }
 
     //kdDebug( 6040 ) << "    contents (" << contentWidth << "/" << contentHeight << ") border=" << borderLeft() << " padding=" << paddingLeft() << endl;
@@ -497,32 +497,87 @@
      return image && image->valid_rect().size() == image->pixmap_size() && !needsLayout();
 }
 
-short RenderImage::calcReplacedWidth() const
+bool RenderImage::isWidthSpecified() const
 {
-    const Length w = style()->width();
+    switch (style()->width().type()) {
+        case Fixed:
+        case Percent:
+            return true;
+        default:
+            return false;
+    }
+    assert(false);
+    return false;
+}
 
-    if (w.isVariable()) {
-	int h = RenderReplaced::calcReplacedHeight();
-	if (m_intrinsicHeight > 0 && h!= m_intrinsicHeight) {
-            return (h*intrinsicWidth())/m_intrinsicHeight;
-	}
+bool RenderImage::isHeightSpecified() const
+{
+    switch (style()->height().type()) {
+        case Fixed:
+        case Percent:
+            return true;
+        default:
+            return false;
     }
+    assert(false);
+    return false;
+}
 
-    return RenderReplaced::calcReplacedWidth();
+short RenderImage::calcAspectRatioWidth() const
+{
+    if (intrinsicHeight() == 0)
+        return 0;
+    if (!image || image->isErrorImage())
+        return intrinsicWidth(); // Don't bother scaling.
+    return RenderReplaced::calcReplacedHeight() * intrinsicWidth() / intrinsicHeight();
 }
 
+int RenderImage::calcAspectRatioHeight() const
+{
+    if (intrinsicWidth() == 0)
+        return 0;
+    if (!image || image->isErrorImage())
+        return intrinsicHeight(); // Don't bother scaling.
+    return RenderReplaced::calcReplacedWidth() * intrinsicHeight() / intrinsicWidth();
+}
+
+short RenderImage::calcReplacedWidth() const
+{
+    int width;
+    if (isWidthSpecified())
+        width = calcReplacedWidthUsing(Width);
+    else
+        width = calcAspectRatioWidth();
+    int minW = calcReplacedWidthUsing(MinWidth);
+    int maxW = style()->maxWidth().value() == UNDEFINED ? width : calcReplacedWidthUsing(MaxWidth);
+
+    if (width > maxW)
+        width = maxW;
+
+    if (width < minW)
+        width = minW;
+
+    return width;
+}
+
 int RenderImage::calcReplacedHeight() const
 {
-    const Length h = style()->height();
+    int height;
+    if (isHeightSpecified())
+        height = calcReplacedHeightUsing(Height);
+    else
+        height = calcAspectRatioHeight();
 
-    if (h.isVariable()) {
-	int w = RenderReplaced::calcReplacedWidth();
-        if( m_intrinsicWidth > 0 && w != m_intrinsicWidth )
-            return (w*intrinsicHeight())/m_intrinsicWidth;
+    int minH = calcReplacedHeightUsing(MinHeight);
+    int maxH = style()->maxHeight().value() == UNDEFINED ? height : calcReplacedHeightUsing(MaxHeight);
 
-    }
+    if (height > maxH)
+        height = maxH;
 
-    return RenderReplaced::calcReplacedHeight();
+    if (height < minH)
+        height = minH;
+
+    return height;
 }
 
 #if 0
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_image.h #498205:498206
@@ -66,6 +66,12 @@
     virtual void notifyFinished(CachedObject *finishedObj);
     virtual bool nodeAtPoint(NodeInfo& info, int x, int y, int tx, int ty, HitTestAction hitTestAction, bool inside);
 
+    bool isWidthSpecified() const;
+    bool isHeightSpecified() const;
+
+    short calcAspectRatioWidth() const;
+    int   calcAspectRatioHeight() const;
+
     virtual short calcReplacedWidth() const;
     virtual int   calcReplacedHeight() const;
 
Comment 6 Pierre Schmitz 2006-01-15 00:21:24 UTC
Great! Thank you. :-) Perhaps I will test it as soon as possible.
Comment 7 Tim Beaulen 2006-01-15 12:58:56 UTC
Should this fix be ported to trunk too?
Comment 8 Maksim Orlovich 2006-01-16 01:59:23 UTC
I periodically merge the whole of 3.5 changes to trunk, so it will be less work for everyone if this is not merged individually. But, I don't see testcases added? ;-)
Comment 9 Pierre Schmitz 2006-01-16 19:59:05 UTC
Created attachment 14276 [details]
testcase

I added the testcase as attachment. (So you have everything at the same place
even if I delete the file from my webspace)