|Summary:||Negative margin block doesn't properly clear a float enclosed by a previous sibling|
|Product:||WebKit||Reporter:||tim bates <timothy.c.bates>|
|Component:||Layout and Rendering||Assignee:||Robert Hogan <robert>|
|Severity:||Normal||CC:||eric, hyatt, jchaffraix, mitz, rniwa, robert, simon.fraser, webkit.review.bot|
|OS:||OS X 10.4|
|Bug Depends on:||87461, 87508, 87526|
Description tim bates 2006-09-17 07:34:14 PDT
You will see tgat in Safari, the line "Download all commentaries" is rendered on top of the table of downloadable links, but in Firefox, 1.5, it is rendered above it (as was desired).
Comment 1 mitz 2006-09-17 22:46:11 PDT
Created attachment 10619 [details] Reduction Firefox keeps the blue div below the black box.
Comment 2 Dave Hyatt 2006-09-17 22:57:56 PDT
Safari's rendering is correct according to the CSS2.1 spec.
Comment 3 Dave Hyatt 2006-09-17 22:58:47 PDT
Someone should file a bug in Mozilla and point them at this reduction. See also the bug Eric Seidel filed with an <hr> example. That was the exact same bug in Mozilla but with a positive margin (so might be easier to understand).
Comment 4 Dave Hyatt 2006-09-17 23:03:23 PDT
See Opera 9's rendering as well, which also matches the spec.
Comment 5 Dave Hyatt 2006-09-18 01:07:26 PDT
This is a subtle bug and not the same as the other bug I mentioned. This is a bug in Safari and Opera.
Comment 6 Jon@Chromium 2008-10-07 16:52:10 PDT
Is this also causing the bug reported to Chromium? http://code.google.com/p/chromium/issues/detail?id=1144
Comment 10 Robert Hogan 2011-12-09 13:57:40 PST
If you see this message in your bug spam, could you review me? :)
Comment 11 Dave Hyatt 2011-12-09 14:11:34 PST
Comment on attachment 107184 [details] Patch This isn't the right way to handle this. If/when things shift to cause new floats to potentially overhang, we should just call addOverhangingFloats again. We should not change the definition of what addOverhangingFloats does.
Comment 12 Robert Hogan 2011-12-10 05:43:52 PST
(In reply to comment #11) > (From update of attachment 107184 [details]) > This isn't the right way to handle this. If/when things shift to cause new floats to potentially overhang, we should just call addOverhangingFloats again. We should not change the definition of what addOverhangingFloats does. In this case the float never overhangs its parent, it ends up intruding into a sibling block that has collapsed into its parent. I've updated the fix to only worry about this exact case. This still allows blocks with cleanace that are children of the parent's sibling to collapse into the parent and not clear the float though. But I think that's a separate bug, such children should only collapse as far as the top of their parent and not into their parent's previous sibling. Am I right in thinking that?
Comment 15 Dave Hyatt 2012-05-23 13:58:10 PDT
Comment on attachment 143643 [details] Patch r=me. Nice catch.
Comment 16 WebKit Review Bot 2012-05-24 11:13:50 PDT
Comment on attachment 143643 [details] Patch Clearing flags on attachment: 143643 Committed r118395: <http://trac.webkit.org/changeset/118395>
Comment 17 WebKit Review Bot 2012-05-24 11:13:56 PDT
All reviewed patches have been landed. Closing bug.
Comment 18 Ryosuke Niwa 2012-05-24 23:31:59 PDT
This patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=87461
Comment 19 Ryosuke Niwa 2012-05-25 11:28:10 PDT
Robert & Dave: could either one of you respond to the regression this patch caused? It's pretty bad regression and I'm afraid we might need to revert the patch if we can't fix it in a timely manner or if we don't hear from you guys by the end of the day.
Comment 20 WebKit Review Bot 2012-05-25 12:14:03 PDT
Re-opened since this is blocked by 87526
Comment 21 Robert Hogan 2012-05-25 12:24:06 PDT
I won't get to it today but was planning to fix or roll out by Monday. I'm not in a position to roll out or fix today so feel free to take action today if required.
Comment 22 Ryosuke Niwa 2012-05-25 13:36:32 PDT
Note the regression reduction is: https://bug-87461-attachments.webkit.org/attachment.cgi?id=143986
Comment 24 Julien Chaffraix 2012-05-30 19:36:24 PDT
Comment on attachment 144223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144223&action=review > Source/WebCore/rendering/RenderBlock.cpp:2025 > + if (block->m_floatingObjects && block->lowestFloatLogicalBottom() > logicalTop) Shouldn't you use block->containsFloats() instead of m_floatingObjects here? (the code doesn't seem to draw a strong line between the 2 though) > Source/WebCore/rendering/RenderBlock.cpp:2026 > + addOverhangingFloats(block, false); Overflow: hidden seems covered inside addOverhangingFloats - and in your testing - but how about other cases where CSS 2.1 explicitly mention not to allow a float to overlap: "The border box of a table, a block-level replaced element, or an element in the normal flow that establishes a new block formatting context [...] must not overlap the margin box of any floats in the same block formatting context as the element itself". Are those cases also covered? > Source/WebCore/rendering/RenderBlock.cpp:2027 > + } The logic is very similar to what is done in RenderBlock::layoutBlock, maybe there could be a way for use to share more code between the 2 methods? (I haven't tried so I don't know how difficult / messy it would be)
Comment 25 Robert Hogan 2012-06-06 11:04:35 PDT
(In reply to comment #24) > > Overflow: hidden seems covered inside addOverhangingFloats - and in your testing - but how about other cases where CSS 2.1 explicitly mention not to allow a float to overlap: > > "The border box of a table, a block-level replaced element, or an element in the normal flow that establishes a new block formatting context [...] must not overlap the margin box of any floats in the same block formatting context as the element itself". > > Are those cases also covered? > This part of the spec is covered by the tests in bugs 88434 and to a lesser extent 88171. I have a patch for 88171 and am working on 88434.
Comment 27 Robert Hogan 2012-06-20 10:39:34 PDT
(In reply to comment #26) > Created an attachment (id=148596) [details] > Patch The version of this patch that broke Gmail and got rolled out didn't check whether the previous sibling was a float or positioned. In that situation the previous sibling will get avoided anyway and attempting to avoid floated children of floats causes incorrect layout. The four previous-sibling-* tests in the new patch test for this.
Comment 28 Eric Seidel (no email) 2012-06-20 10:42:09 PDT
Comment on attachment 148596 [details] Patch OK.
Comment 29 WebKit Review Bot 2012-06-20 11:40:29 PDT
Comment on attachment 148596 [details] Patch Clearing flags on attachment: 148596 Committed r120844: <http://trac.webkit.org/changeset/120844>
Comment 30 WebKit Review Bot 2012-06-20 11:40:47 PDT
All reviewed patches have been landed. Closing bug.