[CSS Grid Layout] Fix positioned grid children position and size

[CSS Grid Layout] Fix positioned grid children position and size

According to the spec positioned grid children have a special behavior
described at:
http://dev.w3.org/csswg/css-grid/#abspos-items

The idea is that for positioned children the containing block
will correspond to the padding edges of the grid container, unless the
grid-placement properties are defined.
This not only affects to positioned grid items (direct children) but
also to any descendant where the containing block is the grid
container.

In order to manage this special behavior, the patch is overriding
RenderBlock::layoutPositionedObjects() to calculate the position and
size depending on the grid-placement properties.

RenderBox class has some changes to calculate the containing block width
and height for positioned objects (using the override value). And also
to compute their static position.

Finally, the positioned items are not taken into account in all the
different grid methods, in order that they do not interfere the layout
of the grid as stated in the spec.

BUG=273898
TEST=fast/css-grid-layout/absolutely-positioned-grid-children.html
TEST=fast/css-grid-layout/grid-add-item-with-positioned-items.html
TEST=fast/css-grid-layout/grid-positioned-children-writing-modes.html
TEST=fast/css-grid-layout/grid-positioned-items-background.html
TEST=fast/css-grid-layout/grid-sizing-positioned-items.html
TEST=fast/css-grid-layout/positioned-grid-items-should-not-take-up-space.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186610

[Manuel Rego, 2014-10-07 22:34:42]

rego@igalia.com changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Julien - ping for review, 2014-10-20 19:34:44]

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2769: staticPosition -=
valueForLength(logicalRight, containerLogicalWidth);
It's unclear to me why this doesn't impact regular blocks as we should change
their offset too. I also couldn't see any justification as to why it's good.

Also there is probably a case to be made to split between grid and other blocks
for readability and avoid introducing unintended bugs for the other classes
relying on RenderBlock. If we could do it without extra code duplication, /me
would be more happy!

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2786: staticPosition -=
curr->style()->logicalLeft().value();
O_o

That looks like a bug to me. It will happily do the following conversion: 100%
-> 100px

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (left):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1060: //
http://www.w3.org/TR/css-grid-1/#abspos-items
Please do not link to the Working Draft as it's super old. We should look at the
Editor's draft for implementation
(http://dev.w3.org/csswg/css-grid/#abspos-items). This is true for the
description too.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1073: bool isHorizontal =
isHorizontalWritingMode();
Not a great variable. How about containerHasHorizontalWritingMode?

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1074: LayoutPoint position =
findChildLogicalPosition(*child);
findChildLogicalPosition is going to apply 'justify-content' and 'align-content'
based on the grid cell's position / dimension which doesn't seem to be what we
should be doing.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1123: LayoutUnit
RenderGrid::gridAreaBreadthForChild(const RenderBox& child,
GridTrackSizingDirection direction, const Vector<GridTrack>& tracks, LayoutUnit*
extraSize) const
This really looks like it should be split into 2 functions:
* gridAreaBreadthForChild is unchanged.
* gridAreaBreadthForPositionedChild should call gridAreaBreadthForChild (or some
part of gridAreaBreadthForChild that is suitable for it).

The code in the patch spends most of its time dealing with positioned oddities.

[Manuel Rego, 2014-10-22 14:06:20]

Thanks for the review, I was checking the bug you discovered, then I'll get back
to the rest of the review actually related to this change :-)

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2786: staticPosition -=
curr->style()->logicalLeft().value();
On 2014/10/20 19:34:43, Julien Chaffraix - PST wrote:
> O_o
> 
> That looks like a bug to me. It will happily do the following conversion: 100%
> -> 100px

Though this is unrelated to my patch, I've been checking it and you're right,
it's doing the wrong conversion as you explained.

I've just reported the bug
http://code.google.com/p/chromium/issues/detail?id=425991

[Manuel Rego, 2014-10-23 12:48:38]

Thanks for the review, I'm uploading a new version applying the
suggested changes.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2769: staticPosition -=
valueForLength(logicalRight, containerLogicalWidth);
On 2014/10/20 19:34:43, Julien Chaffraix - PST wrote:
> It's unclear to me why this doesn't impact regular blocks as we should change
> their offset too. I also couldn't see any justification as to why it's good.

Basically it doesn't affect regular blocks, because of the early return
at the beginning of the method.
If any of these conditions are true, we're sure that our parent is a grid.

> Also there is probably a case to be made to split between grid and other
blocks
> for readability and avoid introducing unintended bugs for the other classes
> relying on RenderBlock. If we could do it without extra code duplication, /me
> would be more happy!

Anyway, I agree with you that it'd be simpler to understand if
we split it, so I've done it in the new version of the patch.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2797: staticPosition -=
valueForLength(logicalLeft, containerLogicalWidth);
Like in the previous method, I've done something similar here too.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1073: bool isHorizontal =
isHorizontalWritingMode();
On 2014/10/20 19:34:44, Julien Chaffraix - PST wrote:
> Not a great variable. How about containerHasHorizontalWritingMode?

Acknowledged.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1074: LayoutPoint position =
findChildLogicalPosition(*child);
On 2014/10/20 19:34:43, Julien Chaffraix - PST wrote:
> findChildLogicalPosition is going to apply 'justify-content' and
'align-content'
> based on the grid cell's position / dimension which doesn't seem to be what we
> should be doing.

Yes, I've changed it to avoid this issue.

https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1123: LayoutUnit
RenderGrid::gridAreaBreadthForChild(const RenderBox& child,
GridTrackSizingDirection direction, const Vector<GridTrack>& tracks, LayoutUnit*
extraSize) const
On 2014/10/20 19:34:44, Julien Chaffraix - PST wrote:
> This really looks like it should be split into 2 functions:
> * gridAreaBreadthForChild is unchanged.
> * gridAreaBreadthForPositionedChild should call gridAreaBreadthForChild (or
some
> part of gridAreaBreadthForChild that is suitable for it).
> 
> The code in the patch spends most of its time dealing with positioned
oddities.

I agree, most of my changes were only dealing with positioned stuff, so I've
separated it in 2 different methods as suggested.

[Julien - ping for review, 2014-10-23 15:24:44]

https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html
(right):

https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html:218: <div
class="grid">
In these examples, the grid container is not the containing block for the grid
item, the .unconstrainedContainer is (see
http://www.w3.org/TR/CSS21/visudet.html#containing-block-details)

The fix is to put position != static on the .grid rule.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2744: static LayoutUnit
computeInlineStaticPositionGridItem(Length& logicalLeft, Length& logicalRight,
const RenderBox* child, LayoutUnit containerLogicalWidth)
computeInlineStaticPositionForGridItem (note the 'for' as we would say in
English, which gives you extra point at the blink Scrabble!)

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1150: LayoutUnit
RenderGrid::gridAreaBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, const Vector<GridTrack>& tracks, LayoutUnit&
extraInitialOffset) const
'positioned' element covers any position != static in CSS 2.1. It's my mistake
on this one, I should have been precise in the first review.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1152:
ASSERT(child.isOutOfFlowPositioned());
out-of-flow positioned convers absolutely positioned *and fixed positioned*. You
want to check style()->position() == AbsolutePostion instead.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1182: const GridCoordinate& coordinate =
cachedGridCoordinate(child);
If the grid placement property is 'auto', we should position the positioned
element against the grid container's edges.

[Manuel Rego, 2014-10-23 21:51:13]

Thanks for the review, I've uploaded a new patch fixing the different
issues.

https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html
(right):

https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html:218: <div
class="grid">
On 2014/10/23 15:24:43, Julien Chaffraix - PST wrote:
> In these examples, the grid container is not the containing block for the grid
> item, the .unconstrainedContainer is (see
> http://www.w3.org/TR/CSS21/visudet.html#containing-block-details)

Thanks for the link, now I think I've got it.

> The fix is to put position != static on the .grid rule.

Yeah, I'm adding "position: relative;" to the ".grid" class.
The numbers of the test are now even easier to understand thanks to this change.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2744: static LayoutUnit
computeInlineStaticPositionGridItem(Length& logicalLeft, Length& logicalRight,
const RenderBox* child, LayoutUnit containerLogicalWidth)
On 2014/10/23 15:24:43, Julien Chaffraix - PST wrote:
> computeInlineStaticPositionForGridItem (note the 'for' as we would say in
> English, which gives you extra point at the blink Scrabble!)

Acknowledged. Next time I'll try to use XYZ to get even more points :-)

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:3135: static LayoutUnit
computeBlockStaticPositionGridItem(Length& logicalTop, Length& logicalBottom,
const RenderBox* child, LayoutUnit containerLogicalHeight)
Using "for" here too.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1150: LayoutUnit
RenderGrid::gridAreaBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, const Vector<GridTrack>& tracks, LayoutUnit&
extraInitialOffset) const
On 2014/10/23 15:24:44, Julien Chaffraix - PST wrote:
> 'positioned' element covers any position != static in CSS 2.1. It's my mistake
> on this one, I should have been precise in the first review.

Yes, I changed it to gridAreaBreadthForAbsolutelyPositionedChild().

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1152:
ASSERT(child.isOutOfFlowPositioned());
On 2014/10/23 15:24:43, Julien Chaffraix - PST wrote:
> out-of-flow positioned convers absolutely positioned *and fixed positioned*.
You
> want to check style()->position() == AbsolutePostion instead.

So, I've added a new method to check if it's actually an AbsolutePosition,
but also if the containing block is the grid.
Otherwise, we shouldn't do need any special code to manage those elements.

https://codereview.chromium.org/637033003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1182: const GridCoordinate& coordinate =
cachedGridCoordinate(child);
On 2014/10/23 15:24:44, Julien Chaffraix - PST wrote:
> If the grid placement property is 'auto', we should position the positioned
> element against the grid container's edges.

Actually according to the spec the containing block corresponds to the
padding edges of the grid.

That's why we're adding the padding to the breadth and also setting the
initial and final positions accordingly if start and end are auto (just a few
lines below).

I think this is already working as expected unless I'm missing something.

[Julien - ping for review, 2014-11-11 17:27:35]

https://codereview.chromium.org/637033003/diff/60001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html
(right):

https://codereview.chromium.org/637033003/diff/60001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html:205: <div
class="sizedToGridArea absolute
onlyThirdRowOnlyThirdColumnSpanning2Rows2Columns"
There is definitely no cases where the absolutely positioned grid item is
wrapped inside some statically positioned element(s) and we should have some
testing for that too!

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2746:
ASSERT(child->parent()->isRenderGrid());
I think this should be child->containingBlock()->isRenderGrid() (even better,
just passing |containerBlock| below).

This can be easily tested by wrapping one of your absolutely positioned element
below into a statically positioned element.

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2749: if
(child->parent()->style()->direction() == LTR) {
Same comment.

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1043: bool
RenderGrid::isAbsolutelyPositionedGridItemWithContainingBlockThis(const
RenderBox& child) const
That's better in my book (feel free to adapt / disagree / improve):
isContainingBlockForAbsolutelyPositionedGridItem

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1045: return (child.style()->position() ==
AbsolutePosition) && (child.containingBlock() == this);
The parentheses are not required and we don't put them in Blink.

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1072: if
(isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {
I think the logic is off actually.

RenderBlock ignores absolutely / fixed positioned elements during layout and lay
them out after using the positioned map. I think we will want to do something
similar.

The reason is that grid items now can be descendants and not just direct
children (which means we will have to rethink this deep assumption in
RenderGrid).

[Manuel Rego, 2014-11-12 13:36:34]

Thanks for the review, before addressing other comments
I'd like to clarify one issue.

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1072: if
(isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {
On 2014/11/11 17:27:35, Julien Chaffraix - PST wrote:
> I think the logic is off actually.
> 
> RenderBlock ignores absolutely / fixed positioned elements during layout and
lay
> them out after using the positioned map. I think we will want to do something
> similar.
> 
> The reason is that grid items now can be descendants and not just direct
> children (which means we will have to rethink this deep assumption in
> RenderGrid).

I'm just focusing in this comment right now, as I'm not sure if I'm getting it
properly.

Do you mean that any absolutely positioned element under a grid, where the
containing block is the grid container, should be considered a grid item?

If I'm not wrong according to the spec grid items are direct children of the
grid container.
And placement properties like grid-row/column only apply to grid items.

For example:
<div id="grid" style="display: grid; position: relative;">
  <div id="item">
    <div id="abspos" style="position: absolute;"></div>
  </div>
</div>

In this case, despite "abspos" containing block is "grid", IMHO it's not a grid
item.
It seems to me that these modifications related to the position inside the grid
shouldn't be applied.
And "abspos" should behave almost the same than if "grid" was a regular block.
Following what is defined at:
http://dev.w3.org/csswg/css-grid/#static-position

However, for grid items we should do what is explained at:
http://dev.w3.org/csswg/css-grid/#abspos-items

Which is basically what this patch is trying to implement. The other part is
pending yet (I'm not sure if it's better to do it in the same patch or a
separated
one, but that's a different issue).

What do you think?

[Julien - ping for review, 2014-11-12 19:16:57]

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1072: if
(isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {
On 2014/11/12 13:36:34, Manuel Rego wrote:
> On 2014/11/11 17:27:35, Julien Chaffraix - PST wrote:
> > I think the logic is off actually.
> > 
> > RenderBlock ignores absolutely / fixed positioned elements during layout and
> lay
> > them out after using the positioned map. I think we will want to do
something
> > similar.
> > 
> > The reason is that grid items now can be descendants and not just direct
> > children (which means we will have to rethink this deep assumption in
> > RenderGrid).
> 
> I'm just focusing in this comment right now, as I'm not sure if I'm getting it
> properly.
> 
> Do you mean that any absolutely positioned element under a grid, where the
> containing block is the grid container, should be considered a grid item?

That's a good question and something the specification is not clear on actually.
I would have called them grid items as their containing block is the grid
container but you're right that grid item is defined as direct children of the
grid container.

Whether we call them grid item or not doesn't change the fact that any
descendants should follow http://dev.w3.org/csswg/css-grid/#abspos-items (note
that the paragraph doesn't mention grid item at all, just what containing block
an element has).

> If I'm not wrong according to the spec grid items are direct children of the
> grid container.
> And placement properties like grid-row/column only apply to grid items.
> 
> For example:
> <div id="grid" style="display: grid; position: relative;">
>   <div id="item">
>     <div id="abspos" style="position: absolute;"></div>
>   </div>
> </div>
> 
> In this case, despite "abspos" containing block is "grid", IMHO it's not a
grid
> item.
> It seems to me that these modifications related to the position inside the
grid
> shouldn't be applied.
> And "abspos" should behave almost the same than if "grid" was a regular block.
> Following what is defined at:
> http://dev.w3.org/csswg/css-grid/#static-position
> 
> However, for grid items we should do what is explained at:
> http://dev.w3.org/csswg/css-grid/#abspos-items
> 
> Which is basically what this patch is trying to implement. The other part is
> pending yet (I'm not sure if it's better to do it in the same patch or a
> separated
> one, but that's a different issue).
> 
> What do you think?

I think we should get the specification clarified if you disagree with me. I
think the specification is pretty clear on what to do with absolutely positioned
elements though.

Also regardless of our disagreement over how to interpret the specification, the
absolutely positioned elements should be handled separately like other
containers, which should make the code clearer.

[Manuel Rego, 2014-11-18 14:24:06]

So, after making this clear with the CSS WG, I've been redoing the
patch.

Please could you take a new look?

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1072: if
(isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {
On 2014/11/12 19:16:57, Julien Chaffraix - PST wrote:
> On 2014/11/12 13:36:34, Manuel Rego wrote:
> > Do you mean that any absolutely positioned element under a grid, where the
> > containing block is the grid container, should be considered a grid item?
> 
> That's a good question and something the specification is not clear on
actually.
> I would have called them grid items as their containing block is the grid
> container but you're right that grid item is defined as direct children of the
> grid container.
> 
> Whether we call them grid item or not doesn't change the fact that any
> descendants should follow http://dev.w3.org/csswg/css-grid/#abspos-items (note
> that the paragraph doesn't mention grid item at all, just what containing
block
> an element has).

The title of the section was talking about grid items, but I've
clarified it in www-style and it seems that you were right (check the
following thread
http://lists.w3.org/Archives/Public/www-style/2014Nov/0099.html for more
details).

> Also regardless of our disagreement over how to interpret the specification,
the
> absolutely positioned elements should be handled separately like other
> containers, which should make the code clearer.

Yeah, it's clear that we need to separate this code, so I've overridden
RenderBlock::layoutPositionedObjects() to deal with all this logic for
both grit items (direct children) but also any other descendant.

[Julien - ping for review, 2014-11-18 21:12:45]

https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html
(right):

https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html:13:
padding: 15px;
Ideally we don't want border + margin == padding as you don't know which box
you're using below for placement.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:260: for (sibling = child.nextSiblingBox();
sibling; sibling = sibling->nextSiblingBox()) {
nextSiblingBox doesn't check the type and IIRC we had quite some issues with bad
casts. Won't that make us more prone to such mistakes?

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:965: if (child->isOutOfFlowPositioned()) {
FWIW that's the kind of code that makes me advocate for some
no-out-of-flow-positioned-iterator. Most rendering traversals follow this
pattern and those who need it should be investigated.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1187: RenderBox* child;
Why is child defined outside the for-loop?

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192: if (child->style()->position() ==
AbsolutePosition) {
Fixed positioned elements are never handled here. The specification doesn't
account for them either, which is an oversight as some properties make them
containing block for fixed positioned elements (e.g 'transform')

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1203:
child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
columnBreadth is the client box, which is the padding box. You want to use the
content box here (as the name of the function implies).

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1220: breadth -= (direction == ForColumns)
? paddingLeft() : paddingTop();
Columns / rows follow the logical order so those should be paddingLogicalLeft /
paddingLogicalTop. We should also ASSERT that we are not in an orthogonal flow
as the logic would be totally wrong in this case.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1231: offset -= (direction == ForColumns) ?
paddingLeft() : paddingTop();
Same comment, physical properties vs logical ones.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1235: breadth -= (direction == ForColumns)
? paddingRight() : paddingBottom();
Ditto.

[Manuel Rego, 2014-11-19 15:18:30]

Thanks for the quick review.

I've just uploaded a new version fixing the different comments.

https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html
(right):

https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html:13:
padding: 15px;
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Ideally we don't want border + margin == padding as you don't know which box
> you're using below for placement.

Sure, I've modified it so now we have margin 30, padding 15 and
border 5.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:260: for (sibling = child.nextSiblingBox();
sibling; sibling = sibling->nextSiblingBox()) {
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> nextSiblingBox doesn't check the type and IIRC we had quite some issues with
bad
> casts. Won't that make us more prone to such mistakes?

We solved those issues directly in RenderGrid::addChild().

I don't really know what we can do here as we need to skip the
positioned grid items. Besides, we were already calling nextSiblingBox()
before. Any idea?

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:965: if (child->isOutOfFlowPositioned()) {
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> FWIW that's the kind of code that makes me advocate for some
> no-out-of-flow-positioned-iterator. Most rendering traversals follow this
> pattern and those who need it should be investigated.

Yeah, I understand what you mean, in several cases we don't need the
positioned items.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1187: RenderBox* child;
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Why is child defined outside the for-loop?

A mistake, sorry.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192: if (child->style()->position() ==
AbsolutePosition) {
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Fixed positioned elements are never handled here. The specification doesn't
> account for them either, which is an oversight as some properties make them
> containing block for fixed positioned elements (e.g 'transform')

Yeah, I think you're right. I've removed the if and notified CSS WG in
order to update the spec.
In addition, I've added some examples with fixed elements in the tests.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1203:
child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> columnBreadth is the client box, which is the padding box. You want to use the
> content box here (as the name of the function implies).

It's the padding box of the grid container, and it seems that should be
the containing block of the positioned items. From the spec:
"the containing block will correspond to the padding edges of the grid
container"

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1220: breadth -= (direction == ForColumns)
? paddingLeft() : paddingTop();
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Columns / rows follow the logical order so those should be paddingLogicalLeft
/
> paddingLogicalTop. We should also ASSERT that we are not in an orthogonal flow
> as the logic would be totally wrong in this case.

Yes, you're totally right.
I've added a new test to check that writing modes behavior is right.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1231: offset -= (direction == ForColumns) ?
paddingLeft() : paddingTop();
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Same comment, physical properties vs logical ones.

Done.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1235: breadth -= (direction == ForColumns)
? paddingRight() : paddingBottom();
On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> Ditto.

Done.

[Julien - ping for review, 2014-11-19 18:46:43]

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:260: for (sibling = child.nextSiblingBox();
sibling; sibling = sibling->nextSiblingBox()) {
On 2014/11/19 15:18:29, Manuel Rego wrote:
> On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> > nextSiblingBox doesn't check the type and IIRC we had quite some issues with
> bad
> > casts. Won't that make us more prone to such mistakes?
> 
> We solved those issues directly in RenderGrid::addChild().
> 
> I don't really know what we can do here as we need to skip the
> positioned grid items. Besides, we were already calling nextSiblingBox()
> before. Any idea?

I didn't have a good way forward, only mentioned that as you may have had some
better idea :-)

There is not much we can do AFAICT so let's ignore that.

https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1203:
child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
On 2014/11/19 15:18:29, Manuel Rego wrote:
> On 2014/11/18 21:12:44, Julien Chaffraix - PST wrote:
> > columnBreadth is the client box, which is the padding box. You want to use
the
> > content box here (as the name of the function implies).
> 
> It's the padding box of the grid container, and it seems that should be
> the containing block of the positioned items. From the spec:
> "the containing block will correspond to the padding edges of the grid
> container"

You're totally right about the specification. I meant that
setOverrideContainingBlockContentLogicalWidth expects the containing block's
content box and we are passing in the padding box. I am concern that this
mismatch will hurt us, especially with box-sizing.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:267: for (sibling =
child.previousSiblingBox(); sibling; sibling = sibling->previousSiblingBox()) {
That would probably be nice to move into a helper function like
previousInFlowSiblingBox(). Same for the nextInFlowSiblingBox().

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1213: LayoutUnit
RenderGrid::offsetForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, LayoutUnit& breadth)
The API is weird: you wouldn't assume that it touched |breadth| as the name
doesn't imply it.

I would probably modify it to something that conveys the intent more clearly
(feel free to disagree / improve upon, it's rough):

void RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, LayoutUnit& offset, LayoutUnit& breadth)
{
    offset = LayoutUnit(0);
    breadth = (direction == ForColumn) ? clientLogicalWidth() :
clientLogicalHeight();
    ....
}

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1217: LayoutUnit offset = LayoutUnit(0);
You don't use offset until below so we should move this definition down.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1221: return offset;
And explicitly return LayoutUnit(0) here (I like explicit return better as you
don't have to search the code for the definition).

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1223: if ((direction == ForColumns &&
!child.style()->gridColumnStart().isAuto()) || (direction == ForRows &&
!child.style()->gridRowStart().isAuto())) {
Unfortunately this doesn't handle correctly spanning column or named grid lines.
We can put a FIXME but we will have to think about that.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1234: } else if ((direction == ForColumns
&& !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows &&
!child.style()->gridRowEnd().isAuto())) {
Not sure why this is inside an 'else'. Nothing in the specification explains
that.

[Manuel Rego, 2014-11-19 23:26:24]

Thanks for the fast review, really appreciated!

I've uploaded a new version fixing some of the comments.
However, I still have some doubts commented inline.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:267: for (sibling =
child.previousSiblingBox(); sibling; sibling = sibling->previousSiblingBox()) {
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> That would probably be nice to move into a helper function like
> previousInFlowSiblingBox(). Same for the nextInFlowSiblingBox().

Yes I agree, it makes the code easier to understand.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1213: LayoutUnit
RenderGrid::offsetForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, LayoutUnit& breadth)
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> The API is weird: you wouldn't assume that it touched |breadth| as the name
> doesn't imply it.
> 
> I would probably modify it to something that conveys the intent more clearly
> (feel free to disagree / improve upon, it's rough):
> 
> void RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child,
> GridTrackSizingDirection direction, LayoutUnit& offset, LayoutUnit& breadth)
> {
>     offset = LayoutUnit(0);
>     breadth = (direction == ForColumn) ? clientLogicalWidth() :
> clientLogicalHeight();
>     ....
> }

I added "andBreadth" but kept the return value, as the final code seems
simpler for me.
But I can make it an argument if needed.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1217: LayoutUnit offset = LayoutUnit(0);
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> You don't use offset until below so we should move this definition down.

Done.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1221: return offset;
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> And explicitly return LayoutUnit(0) here (I like explicit return better as you
> don't have to search the code for the definition).

Done.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1223: if ((direction == ForColumns &&
!child.style()->gridColumnStart().isAuto()) || (direction == ForRows &&
!child.style()->gridRowStart().isAuto())) {
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> Unfortunately this doesn't handle correctly spanning column or named grid
lines.
> We can put a FIXME but we will have to think about that.

I'm not sure if I'm getting what you mean.

For example if you have:
grid-column: 1 / span 2;

Both gridColumnStart() and gridColumnEnd() won't be auto.

The same would happen for:
grid-column: A / span C;

In which case are you thinking of?

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1234: } else if ((direction == ForColumns
&& !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows &&
!child.style()->gridRowEnd().isAuto())) {
On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> Not sure why this is inside an 'else'. Nothing in the specification explains
> that.

Let me explain with an example. Imagine a grid with:
  padding: 15px;
  grid-template-rows: 50px;
  grid-template-columns: 50px;
  position: relative;

We have one absolutely positioned items with:
  position: absolute;
  width: 100%;
  height: 100%;

So, we have different values for grid-column property (for example):
A) grid-row: 1 / 1; grid-column: 1 / 1;
  In this case we want the item to have a 50x50 size and be positioned at 15x15.
B) grid-row: 1 / auto; grid-column: 1 / auto;
  Size 65x65 and position 15x15.
C) grid-row: auto / 1; grid-column: auto / 1;
  Size 65x65 and position 0x0.
D) grid-row: auto / auto; grid-column: auto / auto;
  Size 80x80 and position 15x15.

This "else if" is checking that the start was auto but the end is not auto.
Which matches with case C), where we have to decrease the offset in order
to place the element properly.

If we remove the "else", it'll match also A). However in that case we don't
want to move the element.

However, now I've doubts regarding D), if painting the element in 15x15
is the expected behavior or not. I was doing it because if that matches
how display flex and block work for this case. But, maybe for grids we
should actually move it to 0x0 following the spec:
"the containing block will correspond to the padding edges of the grid
container"

In the 2nd example of this mail:
http://lists.w3.org/Archives/Public/www-style/2014Nov/0101.html
fantasai seems to agreed that the expected behavior would be using
the padding edge (in this case 0x0 and not 15x15) so different from
flex and block.

So probably I've to modify that, as my tests are considering what I
explained before (using 15x15).

What do you think?

[Manuel Rego, 2014-11-21 10:11:01]

I think I've made up my mind now, sorry for so many comments
on this :-(

Please, could you take a new look? Thanks.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1234: } else if ((direction == ForColumns
&& !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows &&
!child.style()->gridRowEnd().isAuto())) {
On 2014/11/19 23:26:23, Manuel Rego wrote:
> On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> > Not sure why this is inside an 'else'. Nothing in the specification explains
> > that.

I've removed the "else" as it was not needed at all.

> Let me explain with an example. Imagine a grid with:
>   padding: 15px;
>   grid-template-rows: 50px;
>   grid-template-columns: 50px;
>   position: relative;
> 
> We have one absolutely positioned items with:
>   position: absolute;
>   width: 100%;
>   height: 100%;
> 
> So, we have different values for grid-column property (for example):
> A) grid-row: 1 / 1; grid-column: 1 / 1;
>   In this case we want the item to have a 50x50 size and be positioned at
15x15.
> B) grid-row: 1 / auto; grid-column: 1 / auto;
>   Size 65x65 and position 15x15.
> C) grid-row: auto / 1; grid-column: auto / 1;
>   Size 65x65 and position 0x0.

This one was wrong, it should be:
  Size 65x65 and position 15x15. 

Yeah, exactly the same position than B). However, if you set "left: 0; top: 0"
to both B) and C), only C) will be moved to "0x0".
And it seems to make sense after checking the specs:
* left defines "how far a box's left margin edge is offset to the right of
the left edge of the the box's containing block".
* The "box's containing block" in this case would be the "padding edges of
the grid container".
* Then if left is auto, the spec says that in this situation:
"set 'left' to the static position".

So, while left is not set, we should keep it in the static position, which
includes the padding in C).

> D) grid-row: auto / auto; grid-column: auto / auto;
>   Size 80x80 and position 15x15.

I'm uploading a new patch, removing the else, and updating the values
in the test.

[Julien - ping for review, 2014-11-24 21:44:41]

lgtm

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1223: if ((direction == ForColumns &&
!child.style()->gridColumnStart().isAuto()) || (direction == ForRows &&
!child.style()->gridRowStart().isAuto())) {
On 2014/11/19 23:26:24, Manuel Rego wrote:
> On 2014/11/19 18:46:43, Julien Chaffraix - PST wrote:
> > Unfortunately this doesn't handle correctly spanning column or named grid
> lines.
> > We can put a FIXME but we will have to think about that.
> 
> I'm not sure if I'm getting what you mean.
> 
> For example if you have:
> grid-column: 1 / span 2;
> 
> Both gridColumnStart() and gridColumnEnd() won't be auto.
> 
> The same would happen for:
> grid-column: A / span C;
> 
> In which case are you thinking of?

This is the case I am thinking of:

grid-column: span 2 / span 2;

In this example, we end up resolving it as auto / auto but will not apply this
treatment here as we only look at the computed values.

The comment was also pointing out at:

grid-column: inexistent / span 2;
grid-column: 1 / span 2 nonexistent;

but they don't apply as there are rules to not resolve them as 'auto'.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:3090: else if (!logicalBottom.isAuto())
Shoudn't we remove the logical bottom even if we have a logical top? (again an
'else' that seems suspicious)

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:782: for (previous = previousSiblingBox();
previous && previous->isOutOfFlowPositioned(); previous =
previous->previousSiblingBox()) { }
Nit: I prefer 2 lines to abide by Blink's 1 statement per line intent.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:794: for (next = nextSiblingBox(); next &&
next->isOutOfFlowPositioned(); next = next->nextSiblingBox()) { }
Ditto.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:227: // Positioned items that shouldn't
take up space or otherwise participate in the layout of the grid.
s/that//

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1164: }
Do we still need this as we place the items correctly in
layoutPositionedObjects? It seems like we could just move this below and avoid
an update in 2 passes.

[Manuel Rego, 2014-12-01 11:10:37]

Thanks for the review, it helped me to find some issues in
the previous patch.

Also, I've been testing more cases and asking to the CSS WG
for feedback. As the behavior is not 100% clear in all the cases.

I've uploaded a new version of the patch, despite of
the previous LGTM, I'd like that you take a new look as
I've modified several things.

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1223: if ((direction == ForColumns &&
!child.style()->gridColumnStart().isAuto()) || (direction == ForRows &&
!child.style()->gridRowStart().isAuto())) {
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> This is the case I am thinking of:
> 
> grid-column: span 2 / span 2;
> 
> In this example, we end up resolving it as auto / auto but will not apply this
> treatment here as we only look at the computed values.

I'm not sure about this one, so I've asked in www-style.
I mean, if we want to resolve this, we need to run
the auto-placement algorithm, to know the final resolved positions.

> The comment was also pointing out at:
> 
> grid-column: inexistent / span 2;
> grid-column: 1 / span 2 nonexistent;
> 
> but they don't apply as there are rules to not resolve them as 'auto'.

Yeah, in the case of inexistent named grid lines
my patch is not detecting it properly.

For example:
  grid-column: 3 / span C;

Where:
* gridColumnEnd().isAuto() would be false.
* And the resolved positions won't be null.

For the moment, I've added the FIXME.
Probably a solution could be to check if it's auto,
or check if the named grid line exists or not.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:3090: else if (!logicalBottom.isAuto())
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> Shoudn't we remove the logical bottom even if we have a logical top? (again an
> 'else' that seems suspicious)

Yeah, thanks for your comment.

I was testing this and detected some issues,
so I ended up rewriting this part of the patch.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:782: for (previous = previousSiblingBox();
previous && previous->isOutOfFlowPositioned(); previous =
previous->previousSiblingBox()) { }
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> Nit: I prefer 2 lines to abide by Blink's 1 statement per line intent.

I've changed it for a while,
which probably makes it easer to understand.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:794: for (next = nextSiblingBox(); next &&
next->isOutOfFlowPositioned(); next = next->nextSiblingBox()) { }
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:227: // Positioned items that shouldn't
take up space or otherwise participate in the layout of the grid.
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> s/that//

Done.

https://codereview.chromium.org/637033003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1164: }
On 2014/11/24 21:44:40, Julien Chaffraix - PST wrote:
> Do we still need this as we place the items correctly in
> layoutPositionedObjects? It seems like we could just move this below and avoid
> an update in 2 passes.

You were right, I've moved it to layoutPositionedObjects().

[Julien - ping for review, 2014-12-02 20:46:05]

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2899: if (hasOverrideInlineOffset() &&
!style()->hasStaticInlinePosition(isHorizontal))
This adds one hash lookup for every layout with potentially 2 for grid.

To avoid the hash lookup, we could reorganize the code to do at most a single
hash lookup always.

if (!style()->hasStaticInlinePosition(isHorizontal)) {
    // overrideInlineOffset returns 0 (or we should make it to by using a
correct empty HashMap trait value) if we don't have an offset.
    computedValues.m_position += overrideInlineOffset();
}

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:953: 
Unneeded space.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1174: // FIXME: Detect properly if
start/end is auto for inexistent named grid lines.
Should we refer to the www-style thread here for future reference?

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1185:
child->setOverrideBlockOffset(rowOffset);
Those 2 fields are a lot cleaner of the previous version but:
- the name is not aligned with the intent (it's an extra offset, not an override
offset).
- the logic is now harder to follow as you set the values here but read them in
another part of the code.

The increase in cleanliness seems greater than the small loss in hackability
though.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1187: if (child->parent() == this) {
I don't understand that check. It seems *very wrong* to check parent() and not
containingBlock() (which would be trivially true but that's beside the point).

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192:
childLayer->setStaticInlinePosition(borderStart() + columnOffset);
AFAICT the column starts in the content box so the offset for an element should
always include the border and padding from the containing block.

[Manuel Rego, 2014-12-03 09:48:58]

Thanks for the review.
I've updated the patch following your comments.

Could you take a new look?

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2899: if (hasOverrideInlineOffset() &&
!style()->hasStaticInlinePosition(isHorizontal))
On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> This adds one hash lookup for every layout with potentially 2 for grid.
> 
> To avoid the hash lookup, we could reorganize the code to do at most a single
> hash lookup always.
> 
> if (!style()->hasStaticInlinePosition(isHorizontal)) {
>     // overrideInlineOffset returns 0 (or we should make it to by using a
> correct empty HashMap trait value) if we don't have an offset.
>     computedValues.m_position += overrideInlineOffset();
> }

Nice catch. We can even minimize this more if we check that
the container block is a RenderGrid.
That way we'll only do the hash lookup in the case of grids.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1174: // FIXME: Detect properly if
start/end is auto for inexistent named grid lines.
On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> Should we refer to the www-style thread here for future reference?

Not sure if to the thread itself, as it's expected that the spec gets
an update on this topic (also there're already 2 related threads :-)).

At this moment, there isn't a final resolution. And some things
might change depending on the discussion in the CSS WG.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1185:
child->setOverrideBlockOffset(rowOffset);
On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> Those 2 fields are a lot cleaner of the previous version but:
> - the name is not aligned with the intent (it's an extra offset, not an
override
> offset).

Yeah, I've changed the name.

> - the logic is now harder to follow as you set the values here but read them
in
> another part of the code.

Before it was similar as I was setting it here and using it in RenderBox too.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1187: if (child->parent() == this) {
On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> I don't understand that check. It seems *very wrong* to check parent() and not
> containingBlock() (which would be trivially true but that's beside the point).

This was on purpose to differentiate grid items (direct children)
from other descendant.
As you said checking the containingBlock() won't be needed
as it's going to be always true.

Basically this follows what Tab said in the next mail (2nd question):
http://lists.w3.org/Archives/Public/www-style/2014Nov/0549.html
Because of the not direct children will use the standard static position
already defined, and we won't need to update it here.

Of course, this might change as it's still under discussion.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192:
childLayer->setStaticInlinePosition(borderStart() + columnOffset);
On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> AFAICT the column starts in the content box so the offset for an element
should
> always include the border and padding from the containing block.

This is for the case where we have grid-column-start different from auto.
And we don't have set any offset (left or right), so it's going to use
the static position.

For example:
grid-column: 3 / auto;
In this case the columnOffset already includes the padding
(included in RenderGrid::offsetAndBreadthForPositionedChild())
so we don't need to add the padding too.

[Julien - ping for review, 2014-12-05 00:32:55]

lgtm

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192:
childLayer->setStaticInlinePosition(borderStart() + columnOffset);
On 2014/12/03 09:48:58, Manuel Rego wrote:
> On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> > AFAICT the column starts in the content box so the offset for an element
> should
> > always include the border and padding from the containing block.
> 
> This is for the case where we have grid-column-start different from auto.
> And we don't have set any offset (left or right), so it's going to use
> the static position.
> 
> For example:
> grid-column: 3 / auto;
> In this case the columnOffset already includes the padding
> (included in RenderGrid::offsetAndBreadthForPositionedChild())
> so we don't need to add the padding too.

Let's put a comment in this case. It's weird that offset doesn't include the
border too.

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2900: computedValues.m_position +=
extraInlineOffset();
Note that by adding the isRenderGrid() call above, you've made extra*Offset tied
to RenderGrid when it wasn't before. It's not great IMO as we may want to add it
to other renderers and its name doesn't imply that it's a grid concept.

I could live with that but had to mention this :)

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1202: LayoutUnit
RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, bool startIsAuto, bool endIsAuto,
LayoutUnit& breadth)
I prefer avoiding mixing a return value and out parameters as it's arbitrary
which one belongs where (unless one is a success result).

It would be better with 2 out parameters and a void function IMO but I could
live with the current code as nothing in the style guide mandates this.

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1229: breadth -= isHorizontalWritingMode()
? verticalScrollbarWidth() : horizontalScrollbarHeight();
Should we add scrollbarLogicalWidth()? (we have scrollbarLogicalHeight so it's a
good addition).

[Manuel Rego, 2014-12-05 10:47:08]

Thanks for your patience reviewing all these iterations. :-)

Just one last note, due to http://crbug.com/437356 the following
new test is not passing:
fast/css-grid-layout/grid-positioned-children-writing-modes.html

I've added it to TestExpectations, and I'll add a note on the bug
to get it removed from TestExpectations once it's fixed.

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1192:
childLayer->setStaticInlinePosition(borderStart() + columnOffset);
On 2014/12/05 00:32:55, Julien Chaffraix - PST wrote:
> On 2014/12/03 09:48:58, Manuel Rego wrote:
> > On 2014/12/02 20:46:05, Julien Chaffraix - PST wrote:
> > > AFAICT the column starts in the content box so the offset for an element
> > should
> > > always include the border and padding from the containing block.
> > 
> > This is for the case where we have grid-column-start different from auto.
> > And we don't have set any offset (left or right), so it's going to use
> > the static position.
> > 
> > For example:
> > grid-column: 3 / auto;
> > In this case the columnOffset already includes the padding
> > (included in RenderGrid::offsetAndBreadthForPositionedChild())
> > so we don't need to add the padding too.
> 
> Let's put a comment in this case. It's weird that offset doesn't include the
> border too.

Done.

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2900: computedValues.m_position +=
extraInlineOffset();
On 2014/12/05 00:32:55, Julien Chaffraix - PST wrote:
> Note that by adding the isRenderGrid() call above, you've made extra*Offset
tied
> to RenderGrid when it wasn't before. It's not great IMO as we may want to add
it
> to other renderers and its name doesn't imply that it's a grid concept.
> 
> I could live with that but had to mention this :)

Yeah, I agree with you, so I've removed the isRenderGrid() check.
In addition, I've also removed hasExtraInline/BlockOffset()
as it was only used in an ASSERT in extraInline/BlockOffset().

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1202: LayoutUnit
RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, bool startIsAuto, bool endIsAuto,
LayoutUnit& breadth)
On 2014/12/05 00:32:55, Julien Chaffraix - PST wrote:
> I prefer avoiding mixing a return value and out parameters as it's arbitrary
> which one belongs where (unless one is a success result).
> 
> It would be better with 2 out parameters and a void function IMO but I could
> live with the current code as nothing in the style guide mandates this.

Done.

https://codereview.chromium.org/637033003/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:1229: breadth -= isHorizontalWritingMode()
? verticalScrollbarWidth() : horizontalScrollbarHeight();
On 2014/12/05 00:32:55, Julien Chaffraix - PST wrote:
> Should we add scrollbarLogicalWidth()? (we have scrollbarLogicalHeight so it's
a
> good addition).

Done.

[Manuel Rego, 2014-12-05 12:10:18]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-12-05 12:10:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637033003/280001

[commit-bot: I haz the power, 2014-12-05 12:27:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-05 12:27:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[Manuel Rego, 2014-12-05 16:19:48]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-12-05 16:20:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637033003/300001

[commit-bot: I haz the power, 2014-12-05 17:35:20]

Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186610