Bug 120107

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.

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.

I think the opera rendering is right.

Max-height/width should not change the aspects of the image. I will look it up though.

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.

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;
 

Great! Thank you. :-) Perhaps I will test it as soon as possible.

Should this fix be ported to trunk too?

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? ;-)

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)