Created attachment 209028 [details] Reduction ..
Created attachment 209029 [details] Patch
Comment on attachment 209029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209029&action=review r- > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2584 > +LayoutUnit RenderBlock::LineBreaker::marginOffsetForSelfCollapsingBlock(RenderBlock* child) This function should be in RenderBlock.cpp and not in the line layout file. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2586 > + while (child) { child is a bad name for this variable, since you're looping up through parents. I'd assign to a current variable. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2589 > + if (child->previousSibling() || !child->parent() || !child->parent()->isRenderBlock()) The previousSibling() could be floating or positioned. I think you are wanting to ask if there is a previous normal flow sibling.
Created attachment 209127 [details] Patch
Comment on attachment 209127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209127&action=review r- > Source/WebCore/rendering/RenderObject.h:171 > + RenderObject* previousInFlowSibling() const > + { > + RenderObject* current = previousSibling(); > + while (current && current->isOutOfFlowPositioned()) > + current = current->previousSibling(); > + return current; > + } I would not put this function on RenderObject. I'd put it on RenderBox and call it previousInFlowSiblingBox. Also, you need to check isFloating() as well, since you need to skip floats as well as positioned objects.
Created attachment 209216 [details] Patch
Comment on attachment 209216 [details] Patch r=me
Comment on attachment 209216 [details] Patch Clearing flags on attachment: 209216 Committed r154399: <http://trac.webkit.org/changeset/154399>
All reviewed patches have been landed. Closing bug.
This caused bug 120126.
Re-opened since this is blocked by bug 120129
Created attachment 211641 [details] Patch
Comment on attachment 211641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211641&action=review r- > Source/WebCore/rendering/RenderBlockFlow.cpp:643 > +LayoutUnit RenderBlockFlow::marginOffsetForSelfCollapsingBlock() > +{ > + RenderBlock* current = this; > + while (current && current->isSelfCollapsingBlock()) { > + if (current->style()->clear() && current->getClearDelta(current, LayoutUnit())) > + return current->collapsedMarginBeforeForChild(current); > + current = current->containingBlock(); > + } > + return LayoutUnit(); > +} Doesn't look right that you're crossing objects that could be block formatting contexts (e.g., positioned/floating ancestors). I still don't understand why this information isn't coming down properly. Having to crawl up through ancestors seems like the wrong approach.
Comment on attachment 211641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211641&action=review >> Source/WebCore/rendering/RenderBlockFlow.cpp:643 >> +} > > Doesn't look right that you're crossing objects that could be block formatting contexts (e.g., positioned/floating ancestors). I still don't understand why this information isn't coming down properly. Having to crawl up through ancestors seems like the wrong approach. In this situation we're laying out a float at the top of a self-collapsing block that's nested inside one or more self-collapsing blocks, the topmost of which has clearance with a preceding float. The clearance rules mean that the margin top of this top-most self-collapsing block intrudes into the preceding float. So as we lay out a float that is contained in the self-collapsing block (in our case in another self-collapsing block nested inside it) it's the margin position of the topmost block we're interested in. It *has* been passed down to the float's direct parent, that's why the parent is positioning itself up at the margin-top position that intrudes into the float and that's why it will end up trying to position the float beside it unless we do something. What the parent doesn't know how much that margin top value is so that's why we have to go up to the ancestor to find it out and use it to temporarily give us the real bottom of the preceding float and place our own float below it.
Created attachment 211939 [details] Patch
Comment on attachment 211939 [details] Patch r- I still think this is too late. The logicalTop() of the blocks just needs to be set correctly. You're compensating after the fact for a mistake that was already made earlier. I think the bug is just in collapseMargins somewhere.
Created attachment 214652 [details] Patch
Created attachment 214749 [details] Patch
When we last talked about this, I thought we had worked out that the position of the self-collapsing block was just wrong, i.e., that a mistake had been made in collapseMargins. Are you saying you believe the position of the block is correct? It's just not clear to me why you would have to compensate after-the-fact like this unless the position of the block was set to something wrong by mistake.
Comment on attachment 214749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214749&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:821 > - if (!marginInfo.canCollapseWithMarginBefore()) > + if (!marginInfo.canCollapseWithMarginBefore() || (marginInfo.atBeforeSideOfBlock() && style()->clear() && getClearDelta(this, logicalTop))) Minusing for this bit. collapseMargins is computing the logical top prior to clearance, so checking for clearance inside collapseMargins is a no-no. (Look at the call site of collapseMargins.)
I posted this in IRC, but my suggestion would be to put outlines on the elements. The outline of the child should paint over the outline of the parent, and this should expose the position of these self-collapsing blocks in a way that will make them easier to see. In the example we talked about (the second test), the self-collapsing child should be at the same absolute position as its container, so its outline should paint over the parent. If it is ending up in a different place, then that's the bug you need to fix. In general this patch is still trying to "correct" for a mispositioning as far as I can tell, and there are many ways to expose this mispositioning besides the ones you're trying to correct for. Outlines, clipping, specified height of 0 with children, etc. are all ways to expose the wrong position besides just the positioning of floating children. In other words if the bug is that the self-collapsing block is in the wrong place, all of these other things that depend on the position being correct should be malfunctioning too, so let's find that out.
Created attachment 216601 [details] Patch
Created attachment 216603 [details] Patch
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/22599449
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/22369436
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22589405
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22719402
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/22369439
Comment on attachment 216603 [details] Patch Attachment 216603 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/22699526
Created attachment 216692 [details] Patch
Comment on attachment 216692 [details] Patch Attachment 216692 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/21249765
Comment on attachment 216692 [details] Patch Attachment 216692 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22639742 New failing tests: fast/block/margin-collapse/block-inside-inline/025.html fast/block/margin-collapse/025.html fast/block/float/024.html accessibility/aria-help.html fast/block/margin-collapse/empty-clear-blocks.html
Created attachment 216695 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 216692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216692&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:221 > - if (parentHasFloats) > + if (parentHasFloats || parentBlock->lowestFloatLogicalBottom() > logicalTopOffset) This feels a little weird to me. It seems like it could catch cases other than self-collapsing stuff. Did you mean for this to be qualified with isSelfCollapsingBlock perhaps? I'm wondering also if there's a way to just get parentHasFloats set correctly during the loop, but I can see the trickiness if you did not want to update prev. > Source/WebCore/rendering/RenderBlockFlow.cpp:1183 > + if (lastBlock && lastBlock->isRenderBlockFlow()) I think this would read better as: if (lastBlock && lastBlock->isSelfCollapsingBlock())
Comment on attachment 216692 [details] Patch Attachment 216692 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22539750 New failing tests: fast/block/margin-collapse/block-inside-inline/025.html fast/block/float/024.html fast/block/margin-collapse/025.html fast/block/margin-collapse/empty-clear-blocks.html
Created attachment 216721 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216692 [details] Patch Attachment 216692 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22539760 New failing tests: fast/block/margin-collapse/block-inside-inline/025.html fast/block/float/024.html fast/block/margin-collapse/025.html fast/block/margin-collapse/empty-clear-blocks.html
Created attachment 216727 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 217066 [details] Patch
Comment on attachment 217066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review Something to possibly consider in a follow-up patch would be to add isSelfCollapsingBlock as a virtual function so that you don't have to ask isRenderBlockFlow and then cast to ask isSelfCollapsingBlock... would be nice to just be able to ask any child if they are self-collapsing block without having to call other methods first. > Source/WebCore/rendering/RenderBlockFlow.cpp:221 > + if (parentHasFloats || (parentBlock->lowestFloatLogicalBottom() > logicalTopOffset && prev && toRenderBlockFlow(prev)->isSelfCollapsingBlock())) Minor quibble. I'd move prev && prev->isSelfCollapsingBlock before parentBlock->lowestFloatLogicalBottom() > logicalTopOffset. > Source/WebCore/rendering/RenderBlockFlow.cpp:714 > + ASSERT(isSelfCollapsingBlock()); Good call.
Comment on attachment 217066 [details] Patch Clearing flags on attachment: 217066 Committed r159575: <http://trac.webkit.org/changeset/159575>
Comment on attachment 217066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review > LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43 > - RenderText {#text} at (100,0) size 268x18 > - text run at (100,0) width 268: "This text should be to the right of the float." > + RenderText {#text} at (0,0) size 268x18 > + text run at (0,0) width 268: "This text should be to the right of the float." This is wrong. Text now overlaps the float. The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path.
(In reply to comment #42) > (From update of attachment 217066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review > > > LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43 > > - RenderText {#text} at (100,0) size 268x18 > > - text run at (100,0) width 268: "This text should be to the right of the float." > > + RenderText {#text} at (0,0) size 268x18 > > + text run at (0,0) width 268: "This text should be to the right of the float." > > This is wrong. Text now overlaps the float. > > The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path. Ow. This doesn't happen in blink and I didn't notice it when porting the patch. We need to reset the line layout path away from simple when we add a float. I'll open a separate bug for this.
(In reply to comment #43) > > The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path. > > Ow. This doesn't happen in blink and I didn't notice it when porting the patch. We need to reset the line layout path away from simple when we add a float. I'll open a separate bug for this. See https://bugs.webkit.org/show_bug.cgi?id=124728
(In reply to comment #42) > (From update of attachment 217066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review > > > LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43 > > - RenderText {#text} at (100,0) size 268x18 > > - text run at (100,0) width 268: "This text should be to the right of the float." > > + RenderText {#text} at (0,0) size 268x18 > > + text run at (0,0) width 268: "This text should be to the right of the float." > > This is wrong. Text now overlaps the float. > > The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path. OK, I now understand you. r159579 fixed the temporary regression caused by r159589. However from talking on IRC empty-clear-blocks.html doesn't work in the complex line layout path. So I'll look into that and see what I can do.