Bug 139150

Created attachment 242346 [details] Patch

Comment on attachment 242346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242346&action=review Good catch and yay! for the new tests. But I'm setting r- for the moment due to the amount of required changes. > Source/WebCore/rendering/RenderBlockFlow.cpp:79 > + && !block.isGridItem(); Missing guards for GRID_LAYOUT. > Source/WebCore/rendering/RenderBox.h:533 > + bool isGridItem() const { return parent() && parent()->isRenderGrid(); } Ditto. > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:8 > + height: auto; This is auto by default so you don't likely need it. > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:39 > + <div class="cell relative floatLeft firstRowFirstColumn"> Why firstRowFirstColumn? > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:45 > + <div class="cell floatLeft firstRowSecondColumn"> Why firstRowSecondColumn? > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:51 > + <div class="cell floatLeft clearLeft secondRowFirstColumn"></div> Why secondRowFirstColumn? > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:52 > + <div class="cell floatLeft secondRowSecondColumn"></div> Why secondRowSecondColumn? > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:7 > + -webkit-grid-auto-columns: minmax(50px, -webkit-max-content); Wouldn't be enough with 50px without the minmax? > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:8 > + -webkit-grid-auto-rows: minmax(50px, -webkit-max-content); Ditto. > LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:14 > + clear: both; Not sure why you need the 'clear' here. > LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse-expected.html:2 > +<link href="resources/grid.css" rel="stylesheet"> I guess you don't need this here. > LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse-expected.html:7 > + <div ><p margin="20px 0px">XXXXX</p></div> Nit: extra space after div.

Created attachment 242413 [details] Patch Applied suggested changes.

Comment on attachment 242413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242413&action=review r- > Source/WebCore/rendering/RenderBlock.cpp:1103 > -bool RenderBlock::expandsToEncloseOverhangingFloats() const > +bool RenderBlock::createsNewFormattingContext() const > { > - return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || (parent() && parent()->isFlexibleBoxIncludingDeprecated()) > - || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isRoot() || isRenderFlowThread(); > + return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || isFlexItemIncludingDeprecated() > + || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isRoot() || isRenderFlowThread() > +#if ENABLE(CSS_GRID_LAYOUT) > + || isGridItem() > +#endif > + || style().specifiesColumns() || style().columnSpan(); > } Need to add isRenderRegion to this list. From http://dev.w3.org/csswg/css-regions/ section 3.2: "CSS Regions establish a new block formatting Context." > Source/WebCore/rendering/RenderBlockFlow.cpp:223 > + while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) { Anything that creates a new formatting context also avoids floats, so I'd rather see you fix avoidsFloats to be implemented in terms of createsNewFormattingContext (plus anything additional that avoidsFloats also checks). Then the only change you'd need to make here is the removal of prev->isFloatingOrOutOfFlowPositioned(). > Source/WebCore/rendering/RenderBlockFlow.cpp:2587 > + if (!child.containsFloats() || child.isRoot() || child.isRenderRegion() || child.createsNewFormattingContext()) No need for child.isRoot() here or child.isRenderRegion() here (once you add isRenderRegion to createsNewFormattingContext).

Created attachment 242480 [details] avoids-floats-refactoring The avoidFloats function uses now the new createsNewFormattingContext. Rebaselining the forms/form-hides-table.html test.

Comment on attachment 242480 [details] avoids-floats-refactoring Attachment 242480 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6712604277866496 New failing tests: fast/forms/form-hides-table.html

Created attachment 242485 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 242480 [details] avoids-floats-refactoring Attachment 242480 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5678057452994560 New failing tests: fast/forms/form-hides-table.html

Created attachment 242486 [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

Created attachment 242750 [details] avoidsFloats-refactoring. The avoidsFloats() uses now the new createsNewFormattingContext function. Some tests needed a rebaseline.

Comment on attachment 242750 [details] avoidsFloats-refactoring. r=me, although we should get the test rebaselined ASAP so the behavior change for table captions is recorded in a test.

Comment on attachment 242750 [details] avoidsFloats-refactoring. Clearing flags on attachment: 242750 Committed r176957: <http://trac.webkit.org/changeset/176957>

All reviewed patches have been landed. Closing bug.

Comment on attachment 242750 [details] avoidsFloats-refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=242750&action=review > Source/WebCore/rendering/RenderBlock.cpp:2378 > return RenderBox::avoidsFloats() > - || !style().hasAutoColumnCount() > - || !style().hasAutoColumnWidth() > || style().hasFlowFrom(); Should probably be changed into a 1-liner; style is now peculiar. > Source/WebCore/rendering/RenderBlockFlow.cpp:226 > RenderObject* prev = previousSibling(); > - while (prev && (prev->isFloatingOrOutOfFlowPositioned() || !is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats())) { > + while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) { > if (prev->isFloating()) > parentHasFloats = true; > prev = prev->previousSibling(); This is a confusing way to write this condition. The RenderBox check is not useful since it’s redundant with the RenderBlockFlow check, and the new check is written in a different style than the old. And there is no reason to cast to RenderBlock instead of RenderBlockFlow since we know the object is a RenderBlockFlow. Ideally we would use a helper function, and also rename “prev” to some kind of word rather than an abbreviation, but at least we should write it this way: RenderBlockFlow* previousBlock = nullptr; for (RenderObject* sibling = previousSibling(); sibling; sibling = sibling->previousSibling()) { if (is<RenderBlockFlow>(*sibling)) { auto& siblingBlock = downcast<RenderBlockFlow>(*sibling); if (!siblingBlock.avoidsFloats() && !siblingBlock.createsNewFormattingContext()) previousBlock = &siblingBlock; break; } if (sibling->isFloating()) parentHasFloats = true; } Then, below, we can use previousBlock without having to cast it, since we know it’s a RenderBlockFlow. But also, why do we need to check both avoidsFloats and createsNewFormattingContext? Doesn’t one call the other? > Source/WebCore/rendering/RenderBox.h:535 > +#if ENABLE(CSS_GRID_LAYOUT) > + bool isGridItem() const { return parent() && parent()->isRenderGrid(); } > +#endif Does this really need to be public? Lets have it be private until we find someone who needs to call it.

(In reply to comment #14) Suggested changes will be added in bug #139445. > Comment on attachment 242750 [details] > avoidsFloats-refactoring. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242750&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:2378 > > return RenderBox::avoidsFloats() > > - || !style().hasAutoColumnCount() > > - || !style().hasAutoColumnWidth() > > || style().hasFlowFrom(); > > Should probably be changed into a 1-liner; style is now peculiar. > > > Source/WebCore/rendering/RenderBlockFlow.cpp:226 > > RenderObject* prev = previousSibling(); > > - while (prev && (prev->isFloatingOrOutOfFlowPositioned() || !is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats())) { > > + while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) { > > if (prev->isFloating()) > > parentHasFloats = true; > > prev = prev->previousSibling(); > > This is a confusing way to write this condition. The RenderBox check is not > useful since it’s redundant with the RenderBlockFlow check, and the new > check is written in a different style than the old. And there is no reason > to cast to RenderBlock instead of RenderBlockFlow since we know the object > is a RenderBlockFlow. Ideally we would use a helper function, and also > rename “prev” to some kind of word rather than an abbreviation, but at least > we should write it this way: > > RenderBlockFlow* previousBlock = nullptr; > for (RenderObject* sibling = previousSibling(); sibling; sibling = > sibling->previousSibling()) { > if (is<RenderBlockFlow>(*sibling)) { > auto& siblingBlock = downcast<RenderBlockFlow>(*sibling); > if (!siblingBlock.avoidsFloats() && > !siblingBlock.createsNewFormattingContext()) > previousBlock = &siblingBlock; > break; > } > if (sibling->isFloating()) > parentHasFloats = true; > } > > Then, below, we can use previousBlock without having to cast it, since we > know it’s a RenderBlockFlow. > > But also, why do we need to check both avoidsFloats and > createsNewFormattingContext? Doesn’t one call the other? > > > Source/WebCore/rendering/RenderBox.h:535 > > +#if ENABLE(CSS_GRID_LAYOUT) > > + bool isGridItem() const { return parent() && parent()->isRenderGrid(); } > > +#endif > > Does this really need to be public? Lets have it be private until we find > someone who needs to call it.