RESOLVED FIXED 10900
Negative margin block doesn't properly clear a float enclosed by a previous sibling
https://bugs.webkit.org/show_bug.cgi?id=10900
Summary Negative margin block doesn't properly clear a float enclosed by a previous s...
tim bates
Reported 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).
Attachments
Reduction (238 bytes, text/html)
2006-09-17 22:46 PDT, mitz
no flags
Patch (16.60 KB, patch)
2011-07-23 11:13 PDT, Robert Hogan
no flags
Patch (16.38 KB, patch)
2011-07-23 11:15 PDT, Robert Hogan
no flags
Patch (16.56 KB, patch)
2011-09-13 09:40 PDT, Robert Hogan
no flags
Patch (6.83 KB, patch)
2011-12-10 05:45 PST, Robert Hogan
no flags
Patch (6.24 KB, patch)
2012-05-23 13:54 PDT, Robert Hogan
no flags
Patch (17.61 KB, patch)
2012-05-27 04:18 PDT, Robert Hogan
no flags
Patch (17.56 KB, patch)
2012-06-20 10:28 PDT, Robert Hogan
no flags
mitz
Comment 1 2006-09-17 22:46:11 PDT
Created attachment 10619 [details] Reduction Firefox keeps the blue div below the black box.
Dave Hyatt
Comment 2 2006-09-17 22:57:56 PDT
Safari's rendering is correct according to the CSS2.1 spec.
Dave Hyatt
Comment 3 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).
Dave Hyatt
Comment 4 2006-09-17 23:03:23 PDT
See Opera 9's rendering as well, which also matches the spec.
Dave Hyatt
Comment 5 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.
Jon@Chromium
Comment 6 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
Robert Hogan
Comment 7 2011-07-23 11:13:40 PDT
Robert Hogan
Comment 8 2011-07-23 11:15:50 PDT
Robert Hogan
Comment 9 2011-09-13 09:40:18 PDT
Robert Hogan
Comment 10 2011-12-09 13:57:40 PST
If you see this message in your bug spam, could you review me? :)
Dave Hyatt
Comment 11 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.
Robert Hogan
Comment 12 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?
Robert Hogan
Comment 13 2011-12-10 05:45:33 PST
Robert Hogan
Comment 14 2012-05-23 13:54:23 PDT
Dave Hyatt
Comment 15 2012-05-23 13:58:10 PDT
Comment on attachment 143643 [details] Patch r=me. Nice catch.
WebKit Review Bot
Comment 16 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>
WebKit Review Bot
Comment 17 2012-05-24 11:13:56 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 18 2012-05-24 23:31:59 PDT
This patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=87461
Ryosuke Niwa
Comment 19 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.
WebKit Review Bot
Comment 20 2012-05-25 12:14:03 PDT
Re-opened since this is blocked by 87526
Robert Hogan
Comment 21 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.
Ryosuke Niwa
Comment 22 2012-05-25 13:36:32 PDT
Robert Hogan
Comment 23 2012-05-27 04:18:54 PDT
Julien Chaffraix
Comment 24 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)
Robert Hogan
Comment 25 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.
Robert Hogan
Comment 26 2012-06-20 10:28:20 PDT
Robert Hogan
Comment 27 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.
Eric Seidel (no email)
Comment 28 2012-06-20 10:42:09 PDT
Comment on attachment 148596 [details] Patch OK.
WebKit Review Bot
Comment 29 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>
WebKit Review Bot
Comment 30 2012-06-20 11:40:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.