Negative margin block doesn't properly clear a float enclosed by a previous sibling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[tim bates]

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).

Created attachment 10619 [details]
Reduction

Firefox keeps the blue div below the black box.

[Dave Hyatt]

Safari's rendering is correct according to the CSS2.1 spec.

[Dave Hyatt]

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]

See Opera 9's rendering as well, which also matches the spec.

[Dave Hyatt]

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]

Is this also causing the bug reported to Chromium?  http://code.google.com/p/chromium/issues/detail?id=1144

[Robert Hogan]

Created attachment 101816 [details]
Patch

[Robert Hogan]

Created attachment 101817 [details]
Patch

[Robert Hogan]

Created attachment 107184 [details]
Patch

[Robert Hogan]

If you see this message in your bug spam, could you review me? :)

[Dave Hyatt]

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]

(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]

Created attachment 118691 [details]
Patch

[Robert Hogan]

Created attachment 143643 [details]
Patch

[Dave Hyatt]

Comment on attachment 143643 [details]
Patch

r=me. Nice catch.

[WebKit Review Bot]

Comment on attachment 143643 [details]
Patch

Clearing flags on attachment: 143643

Committed r118395: <http://trac.webkit.org/changeset/118395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=87461

[Ryosuke Niwa]

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]

Re-opened since this is blocked by 87526

[Robert Hogan]

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]

Note the regression reduction is: https://bug-87461-attachments.webkit.org/attachment.cgi?id=143986

[Robert Hogan]

Created attachment 144223 [details]
Patch

[Julien Chaffraix]

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]

(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]

Created attachment 148596 [details]
Patch

[Robert Hogan]

(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 on attachment 148596 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 148596 [details]
Patch

Clearing flags on attachment: 148596

Committed r120844: <http://trac.webkit.org/changeset/120844>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.