Bug 80394

Created attachment 130316 [details] Reduction

Created attachment 130359 [details] Corrected Reduction

When a self-collapsing block has margin-top and needs to clear a float, the clearance calculated is the height of the float minus the margin-top and this should get added to the height of the parent. If the element has a margin-bottom (and it's the last element in the block) then this needs to get added to the height of the parent. At the moment, the clearance calculated for a self-collapsing block is the top of the block minus its bottom margin and this is what gets added to the height of the parent in clearFloatsIfNeeded(). The bottom margin of the last block is not getting added to the height of the parent.

Created attachment 140536 [details] Patch

Comment on attachment 140536 [details] Patch Looks like you have an incorrect rendering in http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/block/margin-collapse/empty-clear-blocks-expected.png#L0 Also, it makes no sense to me that you'd only remove the block-inside-inline version of a test (025.html), which should be the same as the normal block version of the test.

Created attachment 150455 [details] Patch

Created attachment 153695 [details] Patch

Created attachment 153702 [details] Patch

Comment on attachment 153702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153702&action=review > Source/WebCore/rendering/RenderBlock.cpp:2061 > + marginInfo.setCanCollapseMarginAfterWithChildren(false); This line seems wrong to me. What if another sibling is encountered? You've permanently turned off collapsing the parent with the child. If another normal block child comes along after this self-collapsing one, you'll mess up because you set this bit, won't you? That's the reason for the lookahead to figure out if we're really at the bottom of the block.. the lookahead you removed. Basically to fix the bug it seems like all you had to fix was the setLogicalHeight line, no? The other code was fine.

Created attachment 160478 [details] Patch

(In reply to comment #9) > (From update of attachment 153702 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153702&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:2061 > > + marginInfo.setCanCollapseMarginAfterWithChildren(false); > > This line seems wrong to me. What if another sibling is encountered? You've permanently turned off collapsing the parent with the child. If another normal block child comes along after this self-collapsing one, you'll mess up because you set this bit, won't you? Indeed I will - I've fixed this and added a test - margin-collapse-top-margin-clearance-with-sibling.html. > > That's the reason for the lookahead to figure out if we're really at the bottom of the block.. the lookahead you removed. > > Basically to fix the bug it seems like all you had to fix was the setLogicalHeight line, no? The other code was fine. No I still have to fix the other code and collapse the margins of the self-collapsing block together and with any subsequent self-collapsing blocks, but avoid collapsing them with the parent if no normal block child intervenes.

Comment on attachment 160478 [details] Patch Attachment 160478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13592317 New failing tests: fast/css/margin-collapse-013-reduction.html fast/css/margin-collapse-top-margin-clearance.html

Created attachment 160495 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160500 [details] Patch

Comment on attachment 160500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160500&action=review Couple of comments: > Source/WebCore/rendering/RenderBlock.cpp:2059 > + bool wouldCollapseMarginsWithParent = true; Minor optimization, but it seems like you could initialize this to canCollapseMarginAfterWithChildren. That way if the bit is already set to false, you won't waste time doing the lookahead. > Source/WebCore/rendering/RenderBlock.cpp:2089 > + LayoutUnit logicalTop = yPos + heightIncrease; > + // After margin collapsing, one of our floats may now intrude into the child. > + if (containsFloats() && child->isRenderBlock() && lowestFloatLogicalBottom() > logicalTop) > + toRenderBlock(child)->addIntrudingFloats(this, logicalLeftOffsetForContent(), logicalTop); I'm not following this change. It doesn't seem like it should be necessary.

Comment on attachment 160500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160500&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2089 >> + toRenderBlock(child)->addIntrudingFloats(this, logicalLeftOffsetForContent(), logicalTop); > > I'm not following this change. It doesn't seem like it should be necessary. The new clearance calculation combined with margin-collapsing can result in the child moving up past the bottom of the lowest float. If that happens it will need to avoid it. This is tested in the third paragraph of 'empty-clear-blocks.html' below - without it the text overlaps the float rather than avoiding it and moving to the right.

Created attachment 161267 [details] Patch

Created attachment 161276 [details] Patch

Comment on attachment 161276 [details] Patch r=me

Committed r127163: <http://trac.webkit.org/changeset/127163>

(In reply to comment #20) > Committed r127163: <http://trac.webkit.org/changeset/127163> This started causing failures on Apple Mountain Lion http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r127166%20(431)/results.html I know you added a line to the platform/mac/TestExpectations file, but you said you expected IMAGE+TEXT, whereas the actual result is only TEXT. Was that a mistake?

(In reply to comment #21) > (In reply to comment #20) > > Committed r127163: <http://trac.webkit.org/changeset/127163> > > This started causing failures on Apple Mountain Lion > > http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r127166%20(431)/results.html > > I know you added a line to the platform/mac/TestExpectations file, but you said you expected IMAGE+TEXT, whereas the actual result is only TEXT. Was that a mistake? Changed to expect only TEXT in order to get the bots greener. Let me know if this was in error. http://trac.webkit.org/changeset/127192