Bug 121216

Summary: [test case] http://www.xinia.com completely mangled in Konqueror
Product: [Applications] konqueror Reporter: David Anderson <david>
Component: khtmlAssignee: Germain Garand <germain>
Status: RESOLVED FIXED    
Severity: normal    
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Fedora RPMs   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: test case attached

Description David Anderson 2006-02-02 11:44:48 UTC
Version:            (using KDE KDE 3.5.0)
Installed from:    Fedora RPMs

I'm no HTML expert, so I don't know what the problem is; but in Konqueror, http://www.xinia.com is laid out as a complete mess. Compare with Firefox to see what it should look like.
Comment 1 Pedro Fayolle 2006-03-29 05:40:51 UTC
I've dissected the page to find the problem and brought it down to this:

<html>
<body style="background: lightblue;">
	<div style="width: 600px;">
		<div style="float: left; border: 5px solid green;">
			<div style="float: left; border: 5px solid blue;">
				<div style="width: 400px; float: left; border: 5px solid red;">
					A
				</div>
				<div style="width: 400px; float: left; border: 5px solid orange;">
					B
				</div>
			</div>
			<div style="float: left; border: 5px solid yellow">
				C
			</div>
		</div>
		
		<div style="float: left; border: 5px solid white;">D</div>
	</div>
</body>
</html>

Test it on Konqueror and Mozilla to see the difference. Opera 8 and IE6 also render it correctly.
Comment 2 Pedro Fayolle 2006-03-29 16:20:45 UTC
And for what it's worth, here's an explanation of what I think is going on:

The blue box is floated left, so it should expand only to fit its content. Since the red and orange boxes (both floated left inside the blue one) have a bigger combined width than the outer container (green) they are stacked one on top of the other instead of side by side, but the blue box seems to have trouble with this and expands all the way to the right end of the green container, probably trying to get space for the combined width of the red and orange boxes.
Comment 3 Tommi Tervo 2006-03-29 16:25:55 UTC
Created attachment 15351 [details]
test case attached
Comment 4 Tommi Tervo 2006-03-29 16:30:15 UTC
confirmed (3.5.2)
Comment 5 Germain Garand 2006-03-29 19:10:34 UTC
I'm currently working on a similar issue as part of another bug fix (floating tables) => taking.
Comment 6 Pedro Fayolle 2006-03-31 18:50:48 UTC
What's the bug number?
Comment 7 Germain Garand 2006-06-20 17:40:51 UTC
SVN commit 553276 by ggarand:

 Don't let a float serie grow an object's maxwidth beyond the available
 width if it can be avoided. Matches other engines

BUG: 121216



 M  +10 -0     ChangeLog  
 M  +19 -6     rendering/render_block.cpp  
 M  +19 -0     rendering/render_box.cpp  
 M  +2 -0      rendering/render_box.h  
 M  +18 -16    rendering/render_canvas.cpp  


--- branches/KDE/3.5/kdelibs/khtml/ChangeLog #553275:553276
@@ -1,3 +1,13 @@
+2006-06-20  Germain Garand  <germain@ebooksfrance.org>
+
+        Don't let a float serie grow an object's maxwidth beyond the available width
+
+        * rendering/render_block.cpp (calcInlineMinMaxWidth/calcBlockMinMaxWidth): lazzily check available width
+        so floats don't overflow it if they can break line.
+        * rendering/render_box.{h,cpp} (availableWidth{,Using}): new. Like availableHeight{,Using}
+        * rendering/render_canvas.cpp (RenderCanvas::layout): set m_viewportWidth before recalculating minmax, as 
+        availableWidth needs it.
+
 2006-06-15  Allan Sandfeld Jensen <kde@carewolf.com>
 
         Merge CSS3 properties background-size, background-origin and background-clip from WebCore
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_block.cpp #553275:553276
@@ -2704,7 +2704,8 @@
     int inlineMin=0;
 
     int cw = containingBlock()->contentWidth();
-
+    int floatMaxWidth = 0;
+    
     // If we are at the start of a line, we want to ignore all white-space.
     // Also strip spaces if we previously had text that ended in a trailing space.
     bool stripFrontSpaces = true;
@@ -2808,12 +2809,15 @@
                 // go ahead and terminate maxwidth as well.
                 if (child->isFloating()) {
                     if (prevFloat &&
-                        (((prevFloat->style()->floating() & FLEFT) && (child->style()->clear() & CLEFT)) ||
+                        ((inlineMax + childMax > floatMaxWidth) ||
+                         ((prevFloat->style()->floating() & FLEFT) && (child->style()->clear() & CLEFT)) ||
                          ((prevFloat->style()->floating() & FRIGHT) && (child->style()->clear() & CRIGHT)))) {
                         m_maxWidth = kMax(inlineMax, (int)m_maxWidth);
                         inlineMax = 0;
                     }
                     prevFloat = child;
+                    if (!floatMaxWidth)
+                        floatMaxWidth = availableWidth();
                 }
 
                 // Add in text-indent.  This is added in only once.
@@ -2955,6 +2959,8 @@
     RenderObject *child = firstChild();
     RenderObject* prevFloat = 0;
     int floatWidths = 0;
+    int floatMaxWidth = 0;
+
     while(child != 0)
     {
         // positioned children don't affect the minmaxwidth
@@ -3008,9 +3014,13 @@
 
         if(m_maxWidth < w) m_maxWidth = w;
 
-        if (child->isFloating())
-            floatWidths += w;
-        else if (m_maxWidth < w)
+        if (child->isFloating()) {
+            if (prevFloat && (floatWidths + w > floatMaxWidth)) {
+               m_maxWidth = kMax(floatWidths, m_maxWidth);
+               floatWidths = w;
+            } else                        
+               floatWidths += w;
+        } else if (m_maxWidth < w)
             m_maxWidth = w;
 
         // A very specific WinIE quirk.
@@ -3034,8 +3044,11 @@
             if (!cb->isTableCell())
                 m_maxWidth = BLOCK_MAX_WIDTH;
         }
-        if (child->isFloating())
+        if (child->isFloating()) {
             prevFloat = child;
+            if (!floatMaxWidth)
+                floatMaxWidth = availableWidth();
+        }
         child = child->nextSibling();
     }
     m_maxWidth = kMax(floatWidths, m_maxWidth);
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_box.cpp #553275:553276
@@ -1262,6 +1262,25 @@
     return containingBlock()->availableHeight();
 }
 
+int RenderBox::availableWidth() const
+{
+    return availableWidthUsing(style()->width());
+}
+
+int RenderBox::availableWidthUsing(const Length& w) const
+{
+    if (w.isFixed())
+        return calcContentWidth(w.value());
+
+    if (isCanvas())
+        return static_cast<const RenderCanvas*>(this)->viewportWidth();
+
+    if (w.isPercent())
+       return calcContentWidth(w.width(containingBlock()->availableWidth()));
+
+    return containingBlock()->availableWidth();
+}
+
 void RenderBox::calcVerticalMargins()
 {
     if( isTableCell() ) {
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_box.h #553275:553276
@@ -100,6 +100,7 @@
     virtual int   calcReplacedHeight() const;
 
     virtual int availableHeight() const;
+    virtual int availableWidth() const;
 
     void calcVerticalMargins();
 
@@ -130,6 +131,7 @@
     int calcReplacedHeightUsing(HeightType heightType) const;
     int calcPercentageHeight(const Length& height, bool treatAsReplaced = false) const;
     int availableHeightUsing(const Length& h) const;
+    int availableWidthUsing(const Length& w) const;
     int calcImplicitHeight() const;
     bool hasImplicitHeight() const {
         return isPositioned() && !style()->top().isVariable() && !style()->bottom().isVariable();
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_canvas.cpp #553275:553276
@@ -151,10 +151,24 @@
     for(RenderObject* c = firstChild(); c; c = c->nextSibling())
         c->setChildNeedsLayout(true);
 
+    int oldWidth = m_width;
+    int oldHeight = m_height;
+
+    if (m_pagedMode || !m_view) {
+        m_width = m_rootWidth;
+        m_height = m_rootHeight;
+    }
+    else
+    {
+        m_viewportWidth = m_width = m_view->visibleWidth();
+        m_viewportHeight = m_height = m_view->visibleHeight();
+    }
+
 #ifdef SPEED_DEBUG
     QTime qt;
     qt.start();
 #endif
+
     if ( recalcMinMax() )
 	recalcMinMaxWidths();
 
@@ -163,27 +177,15 @@
     qt.start();
 #endif
 
+    bool relayoutChildren = (oldWidth != m_width) || (oldHeight != m_height);
+
+    RenderBlock::layoutBlock( relayoutChildren );
+
 #ifdef SPEED_DEBUG
     kdDebug() << "RenderCanvas::layout time used=" << qt.elapsed() << endl;
     qt.start();
 #endif
-    int oldWidth = m_width;
-    int oldHeight = m_height;
 
-    if (m_pagedMode || !m_view) {
-        m_width = m_rootWidth;
-        m_height = m_rootHeight;
-    }
-    else
-    {
-        m_viewportWidth = m_width = m_view->visibleWidth();
-        m_viewportHeight = m_height = m_view->visibleHeight();
-    }
-
-    bool relayoutChildren = (oldWidth != m_width) || (oldHeight != m_height);
-
-    RenderBlock::layoutBlock( relayoutChildren );
-
     int docW = docWidth();
     int docH = docHeight();