Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 637033003: [CSS Grid Layout] Fix positioned grid children position and size (Closed)

Created:
3 years, 3 months ago by Manuel Rego
Modified:
3 years, 1 month ago
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, rune+blink, svillar, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 12

Patch Set 2 : New version applying suggested changes #

Total comments: 11

Patch Set 3 : New version checking for absolutely posotioned elements #

Patch Set 4 : Minor fix in a comment #

Total comments: 9

Patch Set 5 : New version using layoutPositionedObjects() #

Patch Set 6 : Remove FIXME #

Total comments: 20

Patch Set 7 : New version fixing writing mode issues #

Patch Set 8 : Add missing orthogonal modes check #

Total comments: 15

Patch Set 9 : New version following review comments #

Patch Set 10 : Remove "if else" #

Total comments: 10

Patch Set 11 : New version fixing some bugs #

Patch Set 12 : Add missing FIXME related to auto determination #

Total comments: 13

Patch Set 13 : Update patch following review comments #

Total comments: 6

Patch Set 14 : Patch for landing #

Patch Set 15 : Rebased patch #

Patch Set 16 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1040 lines, -13 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +231 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments Download
A + LayoutTests/fast/css-grid-layout/grid-add-item-with-positioned-items.html View 1 2 3 4 2 chunks +9 lines, -6 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-add-item-with-positioned-items-expected.txt View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-positioned-children-writing-modes.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +80 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-positioned-children-writing-modes-expected.html View 1 2 3 4 5 6 1 chunk +57 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-positioned-items-background.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +77 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-positioned-items-background-expected.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +40 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-sizing-positioned-items.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +125 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-sizing-positioned-items-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/positioned-grid-items-should-not-take-up-space.html View 1 2 3 4 5 6 1 chunk +184 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/positioned-grid-items-should-not-take-up-space-expected.txt View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/grid.css View 1 2 3 4 5 6 7 8 9 10 1 chunk +30 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBox.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +25 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +47 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 7 chunks +98 lines, -7 lines 0 comments Download

Messages

Total messages: 29 (4 generated)
Manuel Rego
3 years, 3 months ago (2014-10-07 22:34:42 UTC) #2
Julien - ping for review
https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/RenderBox.cpp#newcode2769 Source/core/rendering/RenderBox.cpp:2769: staticPosition -= valueForLength(logicalRight, containerLogicalWidth); It's unclear to me why ...
3 years, 3 months ago (2014-10-20 19:34:44 UTC) #3
Manuel Rego
Thanks for the review, I was checking the bug you discovered, then I'll get back ...
3 years, 2 months ago (2014-10-22 14:06:20 UTC) #4
Manuel Rego
Thanks for the review, I'm uploading a new version applying the suggested changes. https://codereview.chromium.org/637033003/diff/1/Source/core/rendering/RenderBox.cpp File ...
3 years, 2 months ago (2014-10-23 12:48:38 UTC) #5
Julien - ping for review
https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html (right): https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html#newcode218 LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html:218: <div class="grid"> In these examples, the grid container is ...
3 years, 2 months ago (2014-10-23 15:24:44 UTC) #6
Manuel Rego
Thanks for the review, I've uploaded a new patch fixing the different issues. https://codereview.chromium.org/637033003/diff/20001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html File ...
3 years, 2 months ago (2014-10-23 21:51:13 UTC) #7
Julien - ping for review
https://codereview.chromium.org/637033003/diff/60001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html (right): https://codereview.chromium.org/637033003/diff/60001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html#newcode205 LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-items.html:205: <div class="sizedToGridArea absolute onlyThirdRowOnlyThirdColumnSpanning2Rows2Columns" There is definitely no cases ...
3 years, 2 months ago (2014-11-11 17:27:35 UTC) #8
Manuel Rego
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/RenderGrid.cpp ...
3 years, 2 months ago (2014-11-12 13:36:34 UTC) #9
Julien - ping for review
https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/637033003/diff/60001/Source/core/rendering/RenderGrid.cpp#newcode1072 Source/core/rendering/RenderGrid.cpp:1072: if (isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) { On 2014/11/12 13:36:34, Manuel Rego wrote: ...
3 years, 2 months ago (2014-11-12 19:16:57 UTC) #10
Manuel Rego
So, after making this clear with the CSS WG, I've been redoing the patch. Please ...
3 years, 2 months ago (2014-11-18 14:24:06 UTC) #11
Julien - ping for review
https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html File LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html (right): https://codereview.chromium.org/637033003/diff/100001/LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html#newcode13 LayoutTests/fast/css-grid-layout/absolutely-positioned-grid-children.html:13: padding: 15px; Ideally we don't want border + margin ...
3 years, 2 months ago (2014-11-18 21:12:45 UTC) #12
Manuel Rego
Thanks for the quick review. I've just uploaded a new version fixing the different comments. ...
3 years, 2 months ago (2014-11-19 15:18:30 UTC) #13
Julien - ping for review
https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/637033003/diff/100001/Source/core/rendering/RenderGrid.cpp#newcode260 Source/core/rendering/RenderGrid.cpp:260: for (sibling = child.nextSiblingBox(); sibling; sibling = sibling->nextSiblingBox()) { ...
3 years, 2 months ago (2014-11-19 18:46:43 UTC) #14
Manuel Rego
Thanks for the fast review, really appreciated! I've uploaded a new version fixing some of ...
3 years, 2 months ago (2014-11-19 23:26:24 UTC) #15
Manuel Rego
I think I've made up my mind now, sorry for so many comments on this ...
3 years, 2 months ago (2014-11-21 10:11:01 UTC) #16
Julien - ping for review
lgtm https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/637033003/diff/140001/Source/core/rendering/RenderGrid.cpp#newcode1223 Source/core/rendering/RenderGrid.cpp:1223: if ((direction == ForColumns && !child.style()->gridColumnStart().isAuto()) || (direction ...
3 years, 1 month ago (2014-11-24 21:44:41 UTC) #17
Manuel Rego
Thanks for the review, it helped me to find some issues in the previous patch. ...
3 years, 1 month ago (2014-12-01 11:10:37 UTC) #18
Julien - ping for review
https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/RenderBox.cpp#newcode2899 Source/core/rendering/RenderBox.cpp:2899: if (hasOverrideInlineOffset() && !style()->hasStaticInlinePosition(isHorizontal)) This adds one hash lookup ...
3 years, 1 month ago (2014-12-02 20:46:05 UTC) #19
Manuel Rego
Thanks for the review. I've updated the patch following your comments. Could you take a ...
3 years, 1 month ago (2014-12-03 09:48:58 UTC) #20
Julien - ping for review
lgtm https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/637033003/diff/220001/Source/core/rendering/RenderGrid.cpp#newcode1192 Source/core/rendering/RenderGrid.cpp:1192: childLayer->setStaticInlinePosition(borderStart() + columnOffset); On 2014/12/03 09:48:58, Manuel Rego ...
3 years, 1 month ago (2014-12-05 00:32:55 UTC) #21
Manuel Rego
Thanks for your patience reviewing all these iterations. :-) Just one last note, due to ...
3 years, 1 month ago (2014-12-05 10:47:08 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637033003/280001
3 years, 1 month ago (2014-12-05 12:10:50 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/37112)
3 years, 1 month ago (2014-12-05 12:27:39 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637033003/300001
3 years, 1 month ago (2014-12-05 16:20:50 UTC) #28
commit-bot: I haz the power
3 years, 1 month ago (2014-12-05 17:35:20 UTC) #29
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186610

Powered by Google App Engine
This is Rietveld 0eb02b776