[css-grid] Fix behavior of positioned items without specific dimensions

[css-grid] Fix behavior of positioned items without specific dimensions

Currently positioned items that doesn't have specific dimensions
are not properly sized. This patch fixes the issues with them.

The patch removes the ExtraInline|BlockOffsets from LayoutBox,
so now LayouGrid is responsible to setting the location of the positioned item.
This will be useful to add alignment support for positioned items later.
It also removes grid specific logic from LayoutBox.

LayoutBox::ComputeInlineStaticDistance() was modified
as it got confused because the container of the positioned grid items
is not the grid container, but the grid area. That caused wrong values
when resolving "auto" in both left and right offset properties.

Note that after this patch we match Firefox behavior on these cases.

BUG=618996
TEST=fast/css-grid-layout/positioned-grid-items-sizing.html
TEST=external/wpt/css/css-grid-1/abspos/

Review-Url: https://codereview.chromium.org/2665133003
Cr-Commit-Position: refs/heads/master@{#470063}
Committed: https://chromium.googlesource.com/chromium/src/+/e48a88ace117744e50c9f9769b40b9d702f286bf

[Manuel Rego, 2017-01-31 12:55:27]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com

[svillar, 2017-01-31 15:28:40]

Awesome removal of code! I don't fully understand your changes though....

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: }
Mind explaining this change?

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2605:
childLayer->setStaticBlockPosition(LayoutUnit(borderBefore()));
Mind explaining this change?

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2653:
child->setLogicalLocation(LayoutPoint(x, y));
I don't get this part too...

[Manuel Rego, 2017-02-01 09:08:20]

I also think all this positioned elements stuff is pretty complex.
I've tried to explain things the best possible way.

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: }
On 2017/01/31 15:28:40, svillar wrote:
> Mind explaining this change?

So this method is used when in a positioned element both
left and right offsets are "auto".
In order to resolve the "auto" value for them, this method resolves
one of them (left in LTR and right in RTL).
Later in computePositionedLogicalWidthUsing() it's required that one
of them was resolved:
  // 'left' and 'right' cannot both be 'auto' because one would of been
  // converted to the static position already
  ASSERT(!(logicalLeft.isAuto() && logicalRight.isAuto()));

Regarding this method, it was using below things like
"enclosingBox->logicalWidth()", for absolutely positioned grid items
that's wrong, as we don't rely on the "enclosingBox" but on the grid area.

For grid items, if both properties are "auto" we can consider that "left"
(or "right" depending on the direction) are simply "0".
LayoutGrid will be in charge of place it properly depending
on the grid placement properties.

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2605:
childLayer->setStaticBlockPosition(LayoutUnit(borderBefore()));
On 2017/01/31 15:28:40, svillar wrote:
> Mind explaining this change?

This was a mistake, we shouldn't include padding here per spec [1]:
"The static position [CSS21] of an absolutely-positioned child
 of a grid container is determined as if it were the sole grid item
 in a grid area whose edges coincide with the padding edges
 of the grid container."

[1] https://drafts.csswg.org/css-grid/

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2653:
child->setLogicalLocation(LayoutPoint(x, y));
On 2017/01/31 15:28:40, svillar wrote:
> I don't get this part too...

I agree this part is tricky but I didn't find a better way to do it.

I'm relying on layoutPositionedObject() to resolve things like
"left: 10px; right: 50px;", so I don't need to do all that computations here.
Then we use the position calculated there to place manually the item.

As you know layoutPositionedObjects() can be called twice,
and in some cases in the first call child->needsLayout() is true,
but 2nd time it's false. That combined with the fact that a grid item
can have a static position (offsets are auto), required this kind of conditions
to get the right values.

Note that the logic when you have a static position or not in
layoutPositionedObject() is quite different.

What's more important about being able to use setLogicalLocation()
is that it allows us to support alignment for positioned items,
which was not possible with the previous approach.

[svillar, 2017-02-01 09:49:07]

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: }
On 2017/02/01 09:08:20, Manuel Rego wrote:
> On 2017/01/31 15:28:40, svillar wrote:
> > Mind explaining this change?
> 
> So this method is used when in a positioned element both
> left and right offsets are "auto".
> In order to resolve the "auto" value for them, this method resolves
> one of them (left in LTR and right in RTL).
> Later in computePositionedLogicalWidthUsing() it's required that one
> of them was resolved:
>   // 'left' and 'right' cannot both be 'auto' because one would of been
>   // converted to the static position already
>   ASSERT(!(logicalLeft.isAuto() && logicalRight.isAuto()));
> 
> Regarding this method, it was using below things like
> "enclosingBox->logicalWidth()", for absolutely positioned grid items
> that's wrong, as we don't rely on the "enclosingBox" but on the grid area.
> 
> For grid items, if both properties are "auto" we can consider that "left"
> (or "right" depending on the direction) are simply "0".
> LayoutGrid will be in charge of place it properly depending
> on the grid placement properties.

OK now I understand that we're just setting it to 0 because we need to resolve
one of them for another method. We should document this because it's far from
explicit in the code.

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2605:
childLayer->setStaticBlockPosition(LayoutUnit(borderBefore()));
On 2017/02/01 09:08:20, Manuel Rego wrote:
> On 2017/01/31 15:28:40, svillar wrote:
> > Mind explaining this change?
> 
> This was a mistake, we shouldn't include padding here per spec [1]:
> "The static position [CSS21] of an absolutely-positioned child
>  of a grid container is determined as if it were the sole grid item
>  in a grid area whose edges coincide with the padding edges
>  of the grid container."
> 
> [1] https://drafts.csswg.org/css-grid/

I guess you're adding a test for this change. In any case please mention it in
the bug description.

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2653:
child->setLogicalLocation(LayoutPoint(x, y));
On 2017/02/01 09:08:20, Manuel Rego wrote:
> On 2017/01/31 15:28:40, svillar wrote:
> > I don't get this part too...
> 
> I agree this part is tricky but I didn't find a better way to do it.
> 
> I'm relying on layoutPositionedObject() to resolve things like
> "left: 10px; right: 50px;", so I don't need to do all that computations here.
> Then we use the position calculated there to place manually the item.
> 
> As you know layoutPositionedObjects() can be called twice,
> and in some cases in the first call child->needsLayout() is true,
> but 2nd time it's false. That combined with the fact that a grid item
> can have a static position (offsets are auto), required this kind of
conditions
> to get the right values.
> 
> Note that the logic when you have a static position or not in
> layoutPositionedObject() is quite different.
> 
> What's more important about being able to use setLogicalLocation()
> is that it allows us to support alignment for positioned items,
> which was not possible with the previous approach.

Having conditions like (hasStaticBlockPosition || childNeedsLayout) makes the
code extremely mysterious as they seem so unrelated that I feel totally unable
to review it now or to eventually change it in the future. We should find a
clear way to do this.

[Manuel Rego, 2017-02-01 11:08:08]

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: }
On 2017/02/01 09:49:07, svillar wrote:
> On 2017/02/01 09:08:20, Manuel Rego wrote:
> > On 2017/01/31 15:28:40, svillar wrote:
> > > Mind explaining this change?
> > 
> > So this method is used when in a positioned element both
> > left and right offsets are "auto".
> > In order to resolve the "auto" value for them, this method resolves
> > one of them (left in LTR and right in RTL).
> > Later in computePositionedLogicalWidthUsing() it's required that one
> > of them was resolved:
> >   // 'left' and 'right' cannot both be 'auto' because one would of been
> >   // converted to the static position already
> >   ASSERT(!(logicalLeft.isAuto() && logicalRight.isAuto()));
> > 
> > Regarding this method, it was using below things like
> > "enclosingBox->logicalWidth()", for absolutely positioned grid items
> > that's wrong, as we don't rely on the "enclosingBox" but on the grid area.
> > 
> > For grid items, if both properties are "auto" we can consider that "left"
> > (or "right" depending on the direction) are simply "0".
> > LayoutGrid will be in charge of place it properly depending
> > on the grid placement properties.
> 
> OK now I understand that we're just setting it to 0 because we need to resolve
> one of them for another method. We should document this because it's far from
> explicit in the code.

Actually I've realized we can do this in a different patch:
https://codereview.chromium.org/2665983004/

So that part could be probably easy to review.

[Manuel Rego, 2017-02-01 11:10:34]

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: }
On 2017/02/01 11:08:08, Manuel Rego wrote:
> Actually I've realized we can do this in a different patch:
> https://codereview.chromium.org/2665983004/
> 
> So that part could be probably easy to review.

Oops sorry this comment is wrong, it was about a different part of the patch.

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2605:
childLayer->setStaticBlockPosition(LayoutUnit(borderBefore()));
On 2017/02/01 09:49:07, svillar wrote:
> On 2017/02/01 09:08:20, Manuel Rego wrote:
> > On 2017/01/31 15:28:40, svillar wrote:
> > > Mind explaining this change?
> > 
> > This was a mistake, we shouldn't include padding here per spec [1]:
> > "The static position [CSS21] of an absolutely-positioned child
> >  of a grid container is determined as if it were the sole grid item
> >  in a grid area whose edges coincide with the padding edges
> >  of the grid container."
> > 
> > [1] https://drafts.csswg.org/css-grid/
> 
> I guess you're adding a test for this change. In any case please mention it in
> the bug description.

This is the part that I've moved to a different patch:
https://codereview.chromium.org/2665983004/

Sorry for the noise. :-(

[Manuel Rego, 2017-05-08 10:43:54]

Description was changed from

==========
[css-grid] Fix behavior of positioned items without specific dimensions

Currently positioned items that doesn't have specific dimensions
are not properly sized. This patch fixes the issues with them.

The patch removes the extraInline|BlockOffsets from LayoutBox,
so now LayouGrid is responsible to setting the location of the positioned item.
This will be useful to add alignment support for positioned items later.
It also removes grid specific logic from LayoutBox.

LayoutBox::adjustBorderBoxLogicalWidthForBoxSizing() was modified
as it got confused because the container of the positioned grid items
is not the grid container, but the grid area. That caused wrong values
when resolving "auto" in both left and right offset properties.

One test is updated as the expected results were wrong.
Note that after this patch we match Firefox behavior on these cases.

BUG=618996
TEST=fast/css-grid-layout/positioned-grid-items-sizing.html
==========

to

==========
[css-grid] Fix behavior of positioned items without specific dimensions

Currently positioned items that doesn't have specific dimensions
are not properly sized. This patch fixes the issues with them.

The patch removes the ExtraInline|BlockOffsets from LayoutBox,
so now LayouGrid is responsible to setting the location of the positioned item.
This will be useful to add alignment support for positioned items later.
It also removes grid specific logic from LayoutBox.

LayoutBox::ComputeInlineStaticDistance() was modified
as it got confused because the container of the positioned grid items
is not the grid container, but the grid area. That caused wrong values
when resolving "auto" in both left and right offset properties.

Note that after this patch we match Firefox behavior on these cases.

BUG=618996
TEST=fast/css-grid-layout/positioned-grid-items-sizing.html
TEST=external/wpt/css/css-grid-1/abspos/
==========

[Manuel Rego, 2017-05-08 10:48:09]

Coming back to this after a long time...

First I've rebased the patch which was still working,
but I found an issue on it (not covered by current tests).
For that reason I'm adding a bunch of new tests under:
external/wpt/css/css-grid-1/abspos/
Most of them fail with the original patch.

I've removed the strange conditions checking if the item
needs layout or not which were pretty weird.
I believe the code is simpler now.

PTAL. Thanks!

[svillar, 2017-05-08 11:18:10]

The patch looks much better now. Also it's great that you remove the hacks from
LayoutBox.

I just need a few clarifications.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3867: // positioned gird
items, as they rely on the grid area. So for grid items if
Nit. gird->grid

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1268:
child->SetY(LayoutUnit());
Why do we need this?

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1274:
layout_scope.SetChildNeedsLayout(child);
Why are you using a SubtreeLayoutScope here? Wouldn't it be enough just to mark
the child as NeedsLayout ?

[jfernandez, 2017-05-08 12:56:07]

Wow, now it's extremely simple. I like it, but let's address first sergio's
comments.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3870: if
(child->IsGridItem() && child->ContainingBlock() == child->Parent()) {
Wouldn't be better to use the child's parent instead, as follows:

  parent = child->Parent();
  if (parent && parent->isLayoutGrid() && parent == child->ContainingBlock())
  
That way we can reuse it for the style direction as well.

[Manuel Rego, 2017-05-08 13:29:44]

Thanks for the reviews. Uploaded new version, PTAL.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3867: // positioned gird
items, as they rely on the grid area. So for grid items if
On 2017/05/08 11:18:10, svillar wrote:
> Nit. gird->grid

Fixed.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:3870: if
(child->IsGridItem() && child->ContainingBlock() == child->Parent()) {
On 2017/05/08 12:56:07, jfernandez wrote:
> Wouldn't be better to use the child's parent instead, as follows:
> 
>   parent = child->Parent();
>   if (parent && parent->isLayoutGrid() && parent == child->ContainingBlock())
>   
> That way we can reuse it for the style direction as well.

Done.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1268:
child->SetY(LayoutUnit());
On 2017/05/08 11:18:10, svillar wrote:
> Why do we need this?

Actually this is a leftover from a previous version of the patch.
We don't need this.

https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1274:
layout_scope.SetChildNeedsLayout(child);
On 2017/05/08 11:18:10, svillar wrote:
> Why are you using a SubtreeLayoutScope here? Wouldn't it be enough just to
mark
> the child as NeedsLayout ?

I just copied it from LayoutBlock::LayoutPositionedObject(),
but it's true I can simply use SetNeedsLayout().
Changed it in the last version.

[svillar, 2017-05-08 14:50:55]

Amazing fix. lgtm

[Manuel Rego, 2017-05-08 17:20:09]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2017-05-08 17:20:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 18:47:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494264009168140,
"parent_rev": "c4a2186ea71c614e959404ea0e5125ad57d38288", "commit_rev":
"e48a88ace117744e50c9f9769b40b9d702f286bf"}

[commit-bot: I haz the power, 2017-05-08 18:47:27]

Description was changed from

==========
[css-grid] Fix behavior of positioned items without specific dimensions

Currently positioned items that doesn't have specific dimensions
are not properly sized. This patch fixes the issues with them.

The patch removes the ExtraInline|BlockOffsets from LayoutBox,
so now LayouGrid is responsible to setting the location of the positioned item.
This will be useful to add alignment support for positioned items later.
It also removes grid specific logic from LayoutBox.

LayoutBox::ComputeInlineStaticDistance() was modified
as it got confused because the container of the positioned grid items
is not the grid container, but the grid area. That caused wrong values
when resolving "auto" in both left and right offset properties.

Note that after this patch we match Firefox behavior on these cases.

BUG=618996
TEST=fast/css-grid-layout/positioned-grid-items-sizing.html
TEST=external/wpt/css/css-grid-1/abspos/
==========

to

==========
[css-grid] Fix behavior of positioned items without specific dimensions

Currently positioned items that doesn't have specific dimensions
are not properly sized. This patch fixes the issues with them.

The patch removes the ExtraInline|BlockOffsets from LayoutBox,
so now LayouGrid is responsible to setting the location of the positioned item.
This will be useful to add alignment support for positioned items later.
It also removes grid specific logic from LayoutBox.

LayoutBox::ComputeInlineStaticDistance() was modified
as it got confused because the container of the positioned grid items
is not the grid container, but the grid area. That caused wrong values
when resolving "auto" in both left and right offset properties.

Note that after this patch we match Firefox behavior on these cases.

BUG=618996
TEST=fast/css-grid-layout/positioned-grid-items-sizing.html
TEST=external/wpt/css/css-grid-1/abspos/

Review-Url: https://codereview.chromium.org/2665133003
Cr-Commit-Position: refs/heads/master@{#470063}
Committed:
https://chromium.googlesource.com/chromium/src/+/e48a88ace117744e50c9f9769b40...
==========

[commit-bot: I haz the power, 2017-05-08 18:47:28]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e48a88ace117744e50c9f9769b40...