Bug 172117 - [css-grid] Fix behavior of positioned items without specific dimensions
Summary: [css-grid] Fix behavior of positioned items without specific dimensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on: 172108
Blocks: 60731
  Show dependency treegraph
 
Reported: 2017-05-15 06:46 PDT by Manuel Rego Casasnovas
Modified: 2017-05-30 20:29 PDT (History)
8 users (show)

See Also:


Attachments
Example to reproduce the issue (367 bytes, text/html)
2017-05-15 06:46 PDT, Manuel Rego Casasnovas
no flags Details
Current output (2.95 KB, image/png)
2017-05-15 06:47 PDT, Manuel Rego Casasnovas
no flags Details
Expected output (2.74 KB, image/png)
2017-05-15 06:47 PDT, Manuel Rego Casasnovas
no flags Details
Patch (17.76 KB, patch)
2017-05-15 22:07 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2017-05-16 00:55 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (1.92 MB, application/zip)
2017-05-16 02:19 PDT, Build Bot
no flags Details
Patch (17.64 KB, patch)
2017-05-16 04:44 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (17.75 KB, patch)
2017-05-25 02:27 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2017-05-15 06:46:47 PDT
Created attachment 310131 [details]
Example to reproduce the issue

Open the attached example to reproduce the issue.

This has been already fixed in Blink: https://codereview.chromium.org/2665133003
Comment 1 Manuel Rego Casasnovas 2017-05-15 06:47:05 PDT
Created attachment 310132 [details]
Current output
Comment 2 Manuel Rego Casasnovas 2017-05-15 06:47:18 PDT
Created attachment 310133 [details]
Expected output
Comment 3 Manuel Rego Casasnovas 2017-05-15 22:07:07 PDT
Created attachment 310224 [details]
Patch
Comment 4 Sergio Villar Senin 2017-05-16 00:49:46 PDT
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.
Comment 5 Manuel Rego Casasnovas 2017-05-16 00:55:13 PDT
Created attachment 310241 [details]
Patch
Comment 6 Manuel Rego Casasnovas 2017-05-16 01:00:16 PDT
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 7 Sergio Villar Senin 2017-05-16 02:13:30 PDT
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 8 Build Bot 2017-05-16 02:19:38 PDT
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
Comment 9 Build Bot 2017-05-16 02:19:39 PDT
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
Comment 10 Manuel Rego Casasnovas 2017-05-16 04:44:21 PDT
Created attachment 310253 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2017-05-16 05:24:51 PDT
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 12 Sergio Villar Senin 2017-05-16 07:43:44 PDT
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 13 Manuel Rego Casasnovas 2017-05-18 08:01:31 PDT
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.
Comment 14 Sergio Villar Senin 2017-05-19 01:31:58 PDT
(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)
Comment 15 Manuel Rego Casasnovas 2017-05-22 06:04:13 PDT
(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 16 Sergio Villar Senin 2017-05-25 01:04:41 PDT
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.
Comment 17 Manuel Rego Casasnovas 2017-05-25 02:27:32 PDT
Created attachment 311212 [details]
Patch
Comment 18 Manuel Rego Casasnovas 2017-05-25 02:28:28 PDT
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 19 WebKit Commit Bot 2017-05-25 03:06:49 PDT
Comment on attachment 311212 [details]
Patch

Clearing flags on attachment: 311212

Committed r217411: <http://trac.webkit.org/changeset/217411>
Comment 20 WebKit Commit Bot 2017-05-25 03:06:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2017-05-30 20:29:51 PDT
<rdar://problem/32479858>