Bug 172117

Created attachment 310132 [details] Current output

Created attachment 310133 [details] Expected output

Created attachment 310224 [details] Patch

Comment on attachment 310224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review > Source/WebCore/rendering/RenderGrid.cpp:950 > + LayoutUnit columnBreadth = LayoutUnit(); You don't need to do this, the default constructor initializes it to 0. > Source/WebCore/rendering/RenderGrid.cpp:953 > + LayoutUnit rowBreadth = LayoutUnit(); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:961 > + child.setNeedsLayout(); Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout.

Created attachment 310241 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review Thanks for the review. PTAL. >> Source/WebCore/rendering/RenderGrid.cpp:950 >> + LayoutUnit columnBreadth = LayoutUnit(); > > You don't need to do this, the default constructor initializes it to 0. Ok. Done. >> Source/WebCore/rendering/RenderGrid.cpp:953 >> + LayoutUnit rowBreadth = LayoutUnit(); > > Ditto. Done. >> Source/WebCore/rendering/RenderGrid.cpp:961 >> + child.setNeedsLayout(); > > Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout. I couldn't find a better way to do it. As explained in the comment we relay on the generic layout logic to resolve things like "left: 10px; right: 50px;". If the item was not marked for layout, that generic logic was not called, so we were getting different results which needed much more complex code to deal with the different conditions. I don't think an extra layout on positioned items is a big deal, as I won't expect a high usage of positioned items on Grid.

Comment on attachment 310224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review >> Source/WebCore/rendering/RenderGrid.cpp:961 >> + child.setNeedsLayout(); > > Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout. In understand that, the thing is, if the overrides and offsets are the same as the previous ones, do you really need to force a new layout?

Comment on attachment 310241 [details] Patch Attachment 310241 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3749293 New failing tests: fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-2.html fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-1.html

Created attachment 310247 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 310253 [details] Patch

Comment on attachment 310224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:961 >>> + child.setNeedsLayout(); >> >> Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout. > > In understand that, the thing is, if the overrides and offsets are the same as the previous ones, do you really need to force a new layout? The overrides are the same, and the offsets are the same, but the second call to RenderBlock::layoutPositionedObject(child, relayoutChildren, fixedPositionObjectsOnly); is changing the position of the element, the one I got with: child.logicalLeft() later and it makes a different thing if it's marked or not as layout. So marking it as layout always, make the code repeatable. RenderBlock::layoutPositionedObject() is already marking the item for layout in other situations (some related with flexbox, and other more generic ones). I can give it more tries, and check other options, but I'm not sure I'll find anything better. Note that we're not storing the offsets anymore so we cannot compare the new ones with the previous ones.

Comment on attachment 310224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review >>>> Source/WebCore/rendering/RenderGrid.cpp:961 >>>> + child.setNeedsLayout(); >>> >>> Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout. >> >> In understand that, the thing is, if the overrides and offsets are the same as the previous ones, do you really need to force a new layout? > > The overrides are the same, and the offsets are the same, but the second call to > RenderBlock::layoutPositionedObject(child, relayoutChildren, fixedPositionObjectsOnly); > is changing the position of the element, the one I got with: child.logicalLeft() later > and it makes a different thing if it's marked or not as layout. > So marking it as layout always, make the code repeatable. > > RenderBlock::layoutPositionedObject() is already marking the item for layout > in other situations (some related with flexbox, and other more generic ones). > > I can give it more tries, and check other options, > but I'm not sure I'll find anything better. > Note that we're not storing the offsets anymore > so we cannot compare the new ones with the previous ones. I guess the position is different if anything changes, but if you set the same override and everything else is the same I guess layoutPositionedObject will behave exactly as the previous call with the same values. I understand that the offsets are not stored but I guess they don't affect the layout in layoutPositionedObject(), don't they? What we need to know, IMO, is whether the overrides have changed or not and mark the item for layout as needed. Have you tried that?

Comment on attachment 310224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310224&action=review >>>>> Source/WebCore/rendering/RenderGrid.cpp:961 >>>>> + child.setNeedsLayout(); >>>> >>>> Not sure we need to do this. For grid items we check whether the overrides are the same and, in those cases, we don't force a new layout. In the case of positioned items I guess we could do something similar instead of always forcing a layout. >>> >>> In understand that, the thing is, if the overrides and offsets are the same as the previous ones, do you really need to force a new layout? >> >> The overrides are the same, and the offsets are the same, but the second call to >> RenderBlock::layoutPositionedObject(child, relayoutChildren, fixedPositionObjectsOnly); >> is changing the position of the element, the one I got with: child.logicalLeft() later >> and it makes a different thing if it's marked or not as layout. >> So marking it as layout always, make the code repeatable. >> >> RenderBlock::layoutPositionedObject() is already marking the item for layout >> in other situations (some related with flexbox, and other more generic ones). >> >> I can give it more tries, and check other options, >> but I'm not sure I'll find anything better. >> Note that we're not storing the offsets anymore >> so we cannot compare the new ones with the previous ones. > > I guess the position is different if anything changes, but if you set the same override and everything else is the same I guess layoutPositionedObject will behave exactly as the previous call with the same values. > > I understand that the offsets are not stored but I guess they don't affect the layout in layoutPositionedObject(), don't they? What we need to know, IMO, is whether the overrides have changed or not and mark the item for layout as needed. Have you tried that? I tried that but it didn't work. Let me try to explain myself once more. O:-) The call to layoutPositionedObject() is not affected by the offsets, but that's not enough. Imagine a grid with 2 columns of 100px each column, and an item absolutely positioned on the 2nd column (grid-column: 2 / 3;). The first time RenderGrid::layoutPositionedObject() is called the child is marked for layout, after the call to RenderBlock::layoutPositionedObject() we got as logicalLeft() "0px". So we add the offset (100px) and set the child location. The 2nd time RenderGrid::layoutPositionedObject() is called the child is not marked for layout, and after the call to RenderBlock::layoutPositionedObject() we got as logicalLeft() "100px". If we add the offset again, we'll end up with 200px. And if there are more layouts (due to windows resizes or whatever, we get that size increased and increased). We could avoid adding the offsets if the child was not marked for layout, but that seemed pretty unreliable. So marking it for layout, makes us get the same result than in the first run, helping to make the code repeatable. In addition, note that if you change a positioned item from "grid-column: 1 / 2" to "grid-column: 2 / 3" the offset changes. So in that case we need to change the values in the setLogicalLocation(), it's not enough to relay on the overrides.

(In reply to Manuel Rego Casasnovas from comment #13) > I tried that but it didn't work. Let me try to explain myself once more. O:-) > > The call to layoutPositionedObject() is not affected by the offsets, but > that's not enough. > Imagine a grid with 2 columns of 100px each column, and an item absolutely > positioned on the 2nd column (grid-column: 2 / 3;). > The first time RenderGrid::layoutPositionedObject() is called the child is > marked for layout, > after the call to RenderBlock::layoutPositionedObject() we got as > logicalLeft() "0px". > So we add the offset (100px) and set the child location. > The 2nd time RenderGrid::layoutPositionedObject() is called the child is not > marked for layout, > and after the call to RenderBlock::layoutPositionedObject() we got as > logicalLeft() "100px". > If we add the offset again, we'll end up with 200px. > And if there are more layouts (due to windows resizes or whatever, we get > that size increased and increased). Thanks for the explanation, I understand it better now. > We could avoid adding the offsets if the child was not marked for layout, > but that seemed pretty unreliable. Why? > So marking it for layout, makes us get the same result than in the first > run, helping to make the code repeatable. I understand this, but that's not a reason persè (we could relayout the whole thing completely to make it more repeatable but we know that is not a good idea)

(In reply to Sergio Villar Senin from comment #14) > (In reply to Manuel Rego Casasnovas from comment #13) > > We could avoid adding the offsets if the child was not marked for layout, > > but that seemed pretty unreliable. > > Why? So let's try to explain this one, and the issues I'm facing, with a similar example to the one in the previous comment. Imagine a grid with 2 rows of 100px each, and an item absolutely positioned on the 2nd row (grid-row: 2 / 3;). The first time RenderGrid::layoutPositionedObject() is called the child is marked for layout, after the call to RenderBlock::layoutPositionedObject() we got as logicalTop() "0px". So we add the offset (100px) and set the child location. The 2nd time RenderGrid::layoutPositionedObject() is called the child is not marked for layout, and after the call to RenderBlock::layoutPositionedObject() we got as logicalTop() "100px". We avoid to add the offset again, so the position is still 100px. That's going fine. But then for example if you open the inspector on the positioned item, a call to RenderBox::updateLogicalHeight() is triggered. That method doesn't know anything about the offsets, so it changes the top position to 0px again. Then when RenderGrid::layoutPositionedObject() is called, the item is still not marked for layout, and after the call to RenderBlock::layoutPositionedObject() we got as logicalTop() "0px". As it's not marked for layout we don't add the offset, so the final position is 0px (which is wrong). And that would happen every time a call to updateLogicalHeight() with the item not marked for layout, and then a new layout is triggered. We don't want that RenderBox::updateLogicalHeight() knows anything about the grid layout offsets, as we want to be able to later align stuff manually in LayoutGrid. So I cannot think in a good solution for this, other than forcing the layout and be sure we got reliable data to work with.

Comment on attachment 310253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310253&action=review > Source/WebCore/ChangeLog:17 > + as it got confused because the container of the positioned grid items "was modified as it got confused" sounds really weird, please rephrase. > Source/WebCore/rendering/RenderGrid.cpp:961 > + child.setChildNeedsLayout(MarkOnlyThis); I'm pretty happy to see the RenderBox code removed but I guess we can do better than forcing a layout here. Add a FIXME here.

Created attachment 311212 [details] Patch

Comment on attachment 310253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310253&action=review Thanks for the review. >> Source/WebCore/ChangeLog:17 >> + as it got confused because the container of the positioned grid items > > "was modified as it got confused" sounds really weird, please rephrase. Done. >> Source/WebCore/rendering/RenderGrid.cpp:961 >> + child.setChildNeedsLayout(MarkOnlyThis); > > I'm pretty happy to see the RenderBox code removed but I guess we can do better than forcing a layout here. Add a FIXME here. Ok, I've added the FIXME and I'll keep digging to see if I can avoid this somehow.

Comment on attachment 311212 [details] Patch Clearing flags on attachment: 311212 Committed r217411: <http://trac.webkit.org/changeset/217411>

All reviewed patches have been landed. Closing bug.

<rdar://problem/32479858>