Bug 16148 - overflow: hidden with float:left rendering issue
Summary: overflow: hidden with float:left rendering issue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2007-11-26 14:14 PST by Dieter Komendera
Modified: 2012-12-26 15:21 PST (History)
5 users (show)

See Also:


Attachments
testcase which demonstrates the rendering issue (1.33 KB, text/html)
2007-11-26 14:15 PST, Dieter Komendera
no flags Details
Patch calcPrefWidths (use specified width as min/max pref width when 0). (5.12 KB, patch)
2009-05-28 12:12 PDT, Jacob Refstrup
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dieter Komendera 2007-11-26 14:14:27 PST
There is an issue when a parent div (float:left, overflow:hidden) contains several child divs (also float:left, overflow:hidden). If one or more of the child divs have a width set to 0, and have content (eg also a div with a specific width), the parent div will then be rendered as of the child divs hadn't set the width to 0.

I'll attach an example HTML file which demonstrates the problem.

This works for me as expected in Firefox 2.0.
Comment 1 Dieter Komendera 2007-11-26 14:15:34 PST
Created attachment 17537 [details]
testcase which demonstrates the rendering issue
Comment 2 Jacob Refstrup 2009-05-28 12:12:54 PDT
Created attachment 30742 [details]
Patch calcPrefWidths (use specified width as min/max pref width when 0).

It seems that comparing width > 0 is fairly common; there's also table cell code that does this. Perhaps a similar fix should at least be made for the table cell case -- not sure if it'd make a difference for min-width or max-width in calcPrefWidths.
Comment 3 Eric Seidel (no email) 2009-06-06 01:14:45 PDT
Comment on attachment 30742 [details]
Patch calcPrefWidths (use specified width as min/max pref width when 0).

Your test case could be made much smaller by removing unneeded lines of HTML.  (e.g. the DOCTYPE, the entire <head> a little <style> tag would also kill some of the copy/paste style=).  It's also missing a newline at the end of the file.

Hyatt should review this.
Comment 4 Dave Hyatt 2009-06-06 13:00:53 PDT
Comment on attachment 30742 [details]
Patch calcPrefWidths (use specified width as min/max pref width when 0).

Pretty sure it's impossible for width to be negative, so once you've changed it to >= 0 it's equivalent to just not having that in the if statement at all.

Also, you need to patch RenderFlexibleBox::calcPrefWidths, as it has the same mistake.
Comment 5 Jacob Refstrup 2009-06-08 23:06:25 PDT
(In reply to comment #4)
> (From update of attachment 30742 [details] [review])
> Pretty sure it's impossible for width to be negative, so once you've changed it
> to >= 0 it's equivalent to just not having that in the if statement at all.
> 
> Also, you need to patch RenderFlexibleBox::calcPrefWidths, as it has the same
> mistake.
> 

Good catch :-) I'll go ahead and remove the >= 0 and fix the RenderFlexibleBox::calcPrefWidths and simplify the added test case.

Your opinion on whether a similar fix should be applied to the tableCell case that's approx 21 lines below:

        if (isTableCell()) {
            Length w = static_cast<const RenderTableCell*>(this)->styleOrColWidth();
            if (w.isFixed() && w.value() > 0)
                m_maxPrefWidth = max(m_minPrefWidth, calcContentBoxWidth(w.value()));
        }

?
Comment 6 Robert Hogan 2012-12-26 07:22:27 PST
The test case no longer fails as of r138211. OK to close?
Comment 7 Dieter Komendera 2012-12-26 14:24:08 PST
nice, thanks! ok to close from my side.