[CSS Grid Layout] Implement auto-margins alignment of grid items
The Grid Layout spec states that we must center items with auto margins,
so it will absorb any positive free space, both horiontal and vertical.
https://drafts.csswg.org/css-grid/#auto-margins
Besides, auto-margins imply that any alignment property in the same axis
will have no effect
BUG=79180, 249451, 376823
Committed: https://crrev.com/68a823dcd4e9c63a0d324bc96695fd010d734d87
Cr-Commit-Position: refs/heads/master@{#351796}
5 years, 4 months ago
(2015-08-16 22:59:09 UTC)
#2
cbiesinger
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp#newcode1688 Source/core/layout/LayoutGrid.cpp:1688: child.clearOverrideLogicalContentHeight(); Why do you have this line?
5 years, 4 months ago
(2015-08-17 23:50:40 UTC)
#3
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp#newcode1688 Source/core/layout/LayoutGrid.cpp:1688: child.clearOverrideLogicalContentHeight(); On 2015/08/17 23:50:40, cbiesinger wrote: > Why do ...
5 years, 4 months ago
(2015-08-24 10:28:23 UTC)
#4
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1688:
child.clearOverrideLogicalContentHeight();
On 2015/08/17 23:50:40, cbiesinger wrote:
> Why do you have this line?
Good you realized about this "obscure" change. The reason for that line is to
avoid the assertion in LayoutBox::computeLogicalHeight
https://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutBox.cpp?...
This assertion was added sometime ago by @rego to deal with minHeight auto
values when block has an oiverrideLogicalContentHeight (hence, it was
stretched). Since we only stretch grid items with auto height, this condition
was always true, hence he decided to ensure that with an assert.
Now in this patch we are requesting to computeLogicalHeight to reset the
previously set auto-margins, hence it triggers the assert when forcing a
relayout (for instance, when changing margins value, which is one of the
relayout tests cases available for grid).
We have been discussing about this and @rego thinks we can change change the
assert by an additional condition in the mentioned branch. Obviously clearing
the override height would work as well, but it's perhaps a but hacky.
I'd appreciate your feedback on this issue.
cbiesinger
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGrid.cpp#newcode1688 Source/core/layout/LayoutGrid.cpp:1688: child.clearOverrideLogicalContentHeight(); On 2015/08/24 10:28:23, jfernandez wrote: > On 2015/08/17 ...
5 years, 3 months ago
(2015-08-26 23:07:45 UTC)
#5
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1688:
child.clearOverrideLogicalContentHeight();
On 2015/08/24 10:28:23, jfernandez wrote:
> On 2015/08/17 23:50:40, cbiesinger wrote:
> > Why do you have this line?
>
> Good you realized about this "obscure" change. The reason for that line is to
> avoid the assertion in LayoutBox::computeLogicalHeight
>
>
https://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutBox.cpp?...
>
> This assertion was added sometime ago by @rego to deal with minHeight auto
> values when block has an oiverrideLogicalContentHeight (hence, it was
> stretched). Since we only stretch grid items with auto height, this condition
> was always true, hence he decided to ensure that with an assert.
>
> Now in this patch we are requesting to computeLogicalHeight to reset the
> previously set auto-margins, hence it triggers the assert when forcing a
> relayout (for instance, when changing margins value, which is one of the
> relayout tests cases available for grid).
>
> We have been discussing about this and @rego thinks we can change change the
> assert by an additional condition in the mentioned branch. Obviously clearing
> the override height would work as well, but it's perhaps a but hacky.
>
> I'd appreciate your feedback on this issue.
Is your link correct? It does not point to computeLogicalHeight, or an assert at
all, so I am not sure I completely follow the argument. I hope you can show me
the right link but for now, without seeing the assert, it seems better to adjust
the assert?
5 years, 3 months ago
(2015-08-27 08:50:53 UTC)
#6
On 2015/08/26 23:07:45, cbiesinger wrote:
>
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
> File Source/core/layout/LayoutGrid.cpp (right):
>
>
https://codereview.chromium.org/1298623002/diff/1/Source/core/layout/LayoutGr...
> Source/core/layout/LayoutGrid.cpp:1688:
> child.clearOverrideLogicalContentHeight();
> On 2015/08/24 10:28:23, jfernandez wrote:
> > On 2015/08/17 23:50:40, cbiesinger wrote:
> > > Why do you have this line?
> >
> > Good you realized about this "obscure" change. The reason for that line is
to
> > avoid the assertion in LayoutBox::computeLogicalHeight
> >
> >
>
https://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutBox.cpp?...
> >
> > This assertion was added sometime ago by @rego to deal with minHeight auto
> > values when block has an oiverrideLogicalContentHeight (hence, it was
> > stretched). Since we only stretch grid items with auto height, this
condition
> > was always true, hence he decided to ensure that with an assert.
> >
> > Now in this patch we are requesting to computeLogicalHeight to reset the
> > previously set auto-margins, hence it triggers the assert when forcing a
> > relayout (for instance, when changing margins value, which is one of the
> > relayout tests cases available for grid).
> >
> > We have been discussing about this and @rego thinks we can change change the
> > assert by an additional condition in the mentioned branch. Obviously
clearing
> > the override height would work as well, but it's perhaps a but hacky.
> >
> > I'd appreciate your feedback on this issue.
>
> Is your link correct? It does not point to computeLogicalHeight, or an assert
at
> all, so I am not sure I completely follow the argument. I hope you can show me
> the right link but for now, without seeing the assert, it seems better to
adjust
> the assert?
Sorry for the mistake, here is the correct link:
https://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutBox.cpp?...
jfernandez
In (PatchSet 2) I removed the resetAutoMarginsAndLogicalTopInColumnAxis function, since now I compute margins for getting ...
5 years, 3 months ago
(2015-09-11 23:43:44 UTC)
#7
In (PatchSet 2) I removed the resetAutoMarginsAndLogicalTopInColumnAxis
function,
since now I compute margins for getting the available space for stretching if
child needs layout.
This removal avoids the issues mentioned in previous reviews regarding
updateLogicalHeight calls while having an override height, which lead to raise
an assertion.
Manuel Rego
Non-owner LGTM. It's a pain that we've so many duplicated code shared with flexbox. It'd ...
5 years, 3 months ago
(2015-09-24 11:38:27 UTC)
#8
Replied to some of the comments. https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html File LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html (right): https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html#newcode14 LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html:14: width: -webkit-fit-content; On ...
5 years, 2 months ago
(2015-09-25 13:47:02 UTC)
#9
Replied to some of the comments.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html
(right):
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html:14:
width: -webkit-fit-content;
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Comments from previous test also apply here.
Acknowledged.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-lr.html:37:
<p>This test checks that aling-self and justify-self properties are not applied
when there is auto-margin in the correponding axis. Instead, auto-margin
alignment should be applied.</p>
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Nit: Maybe add a comment about the writing mode.
>
> I'm not sure if that's common or not in similar tests.
Done.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-rl.html
(right):
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment-vertical-rl.html:14:
width: -webkit-fit-content;
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Ditto.
Done.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html
(right):
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:14:
width: -webkit-fit-content;
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Do we really need this?
Well, not mandatory but it helps to check grid item's position, using meaningful
values, so we don't depend on the viewport's size.
> I'm also wondering if we really need to check
> the size of the grid below.
> It seems we've always the same value.
I can agree with that, perhaps better not to check it out to focus on what the
tests really tries to verify.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:39:
<p>Direction: LTR | Self Aligmment: center | fixed size items | 1
auto-margin</p>
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Typo "Aligmment" vs "Alignment".
>
> This is repeated in the rest of the test.
Done.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:41: <div
class="grid itemsCenter" data-expected-width="200" data-expected-height="400">
On 2015/09/24 11:38:26, Manuel Rego wrote:
> I was referring to these lines,
> I don't think we need to check the size of the grid in this test.
Acknowledged.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:49:
<p>Direction: LTR | Self Aligmment: start | fixed size items | 4 auto-margin</p>
On 2015/09/24 11:38:26, Manuel Rego wrote:
> I guess the right thing would be:
> "Self Alignment: stretch"
>
> I know that the items are fixed and the result is the same than "stretch".
> But it seems more accurate.
Acknowledged.
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:69:
<p>Direction: LTR | Self Aligmment: start | auto size items | 4 auto-margin</p>
On 2015/09/24 11:38:26, Manuel Rego wrote:
> "stretch" instead of "start" again.
Acknowledged.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1355: // We need pending layouts to be done in
order to compute auto-margins properly.
On 2015/09/24 11:38:26, Manuel Rego wrote:
> Probably we could add an ASSERT in the methods
> to check that the layout has been done.
Acknowledged.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1691: Length topOrLeft = isHorizontal ?
child.style()->marginLeft() : child.style()->marginTop();
On 2015/09/24 11:38:26, Manuel Rego wrote:
> To simplify the code and all the checks related to the writting mode
> I guess you can use marginStart() here.
I think it wouldn't be an equivalent logic. Be aware that we should check out
here the value set in the CSS style, not the one finally resolved.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1692: Length bottomOrRight = isHorizontal ?
child.style()->marginRight() : child.style()->marginBottom();
On 2015/09/24 11:38:27, Manuel Rego wrote:
> Ditto.
Replied.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1700: }
On 2015/09/24 11:38:27, Manuel Rego wrote:
> Probably here you could use setMarginStart().
This could be true, but I'm not sure whether it'd be confusing, considering that
we cannot use start/end when retrieving the input data.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1705:
child.setMarginTop(availableAlignmentSpace);
On 2015/09/24 11:38:27, Manuel Rego wrote:
> Ditto.
Replied.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1710:
child.setMarginBottom(availableAlignmentSpace);
On 2015/09/24 11:38:27, Manuel Rego wrote:
> Ditto.
Replied.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1724: Length topOrLeft = isHorizontal ?
child.style()->marginTop() : child.style()->marginLeft();
On 2015/09/24 11:38:26, Manuel Rego wrote:
> The same than in the row axis method.
Replied.
Manuel Rego
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html File LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html (right): https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html#newcode14 LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:14: width: -webkit-fit-content; On 2015/09/25 13:47:01, jfernandez wrote: > On ...
5 years, 2 months ago
(2015-09-25 16:00:59 UTC)
#10
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html
(right):
https://codereview.chromium.org/1298623002/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-margins-alignment.html:14:
width: -webkit-fit-content;
On 2015/09/25 13:47:01, jfernandez wrote:
> On 2015/09/24 11:38:26, Manuel Rego wrote:
> > Do we really need this?
>
> Well, not mandatory but it helps to check grid item's position, using
meaningful
> values, so we don't depend on the viewport's size.
Yeah, specially for the different writing modes.
Sorry but I didn't really before.
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):
https://codereview.chromium.org/1298623002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1691: Length topOrLeft = isHorizontal ?
child.style()->marginLeft() : child.style()->marginTop();
On 2015/09/25 13:47:02, jfernandez wrote:
> On 2015/09/24 11:38:26, Manuel Rego wrote:
> > To simplify the code and all the checks related to the writting mode
> > I guess you can use marginStart() here.
>
> I think it wouldn't be an equivalent logic. Be aware that we should check out
> here the value set in the CSS style, not the one finally resolved.
BTW, I was thinking in style()->marginStart()
which is already checking the writing mode by itself.
jfernandez
5 years, 2 months ago
(2015-09-28 09:18:52 UTC)
#11
cbiesinger
lgtm
5 years, 2 months ago
(2015-09-30 14:11:55 UTC)
#12
lgtm
jfernandez
The CQ bit was checked by jfernandez@igalia.com
5 years, 2 months ago
(2015-10-01 09:36:28 UTC)
#13
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1298623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1298623002/40001
5 years, 2 months ago
(2015-10-01 09:37:03 UTC)
#15
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/76349)
5 years, 2 months ago
(2015-10-01 12:12:53 UTC)
#17
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1298623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1298623002/40001
5 years, 2 months ago
(2015-10-01 13:28:13 UTC)
#19
Issue 1298623002: [CSS Grid Layout] Implement auto-margins alignment of grid items
(Closed)
Created 5 years, 4 months ago by jfernandez
Modified 5 years, 2 months ago
Reviewers: svillar, cbiesinger, Manuel Rego
Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Comments: 35