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

Issue 2665133003: [css-grid] Fix behavior of positioned items without specific dimensions (Closed)

Created:
2 years, 1 month ago by Manuel Rego
Modified:
1 year, 10 months ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Fix behavior of positioned items without specific dimensions Currently positioned items that doesn't have specific dimensions are not properly sized. This patch fixes the issues with them. The patch removes the ExtraInline|BlockOffsets from LayoutBox, so now LayouGrid is responsible to setting the location of the positioned item. This will be useful to add alignment support for positioned items later. It also removes grid specific logic from LayoutBox. LayoutBox::ComputeInlineStaticDistance() was modified as it got confused because the container of the positioned grid items is not the grid container, but the grid area. That caused wrong values when resolving "auto" in both left and right offset properties. Note that after this patch we match Firefox behavior on these cases. BUG=618996 TEST=fast/css-grid-layout/positioned-grid-items-sizing.html TEST=external/wpt/css/css-grid-1/abspos/ Review-Url: https://codereview.chromium.org/2665133003 Cr-Commit-Position: refs/heads/master@{#470063} Committed: https://chromium.googlesource.com/chromium/src/+/e48a88ace117744e50c9f9769b40b9d702f286bf

Patch Set 1 #

Total comments: 12

Patch Set 2 : Rebased patch #

Patch Set 3 : New version with more tests #

Patch Set 4 : Add comment requested in the review #

Total comments: 8

Patch Set 5 : Applied suggested changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1747 lines, -58 lines) Patch
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-001.html View 1 2 1 chunk +57 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-001-ref.html View 1 2 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-002.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-003.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-004.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-005.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-005-ref.html View 1 2 1 chunk +44 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-006.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-006-ref.html View 1 2 1 chunk +44 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-007.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-007-ref.html View 1 2 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-008.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-008-ref.html View 1 2 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-009.html View 1 2 1 chunk +61 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-009-ref.html View 1 2 1 chunk +42 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-010.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-011.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-011-ref.html View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-012.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-012-ref.html View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-013.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-013-ref.html View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-014.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-014-ref.html View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-015.html View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-015-ref.html View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-016.html View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-016-ref.html View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-017.html View 1 2 1 chunk +61 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/abspos/positioned-grid-items-017-ref.html View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-items-sizing.html View 1 chunk +70 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-items-sizing-expected.html View 1 chunk +64 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.h View 1 2 1 chunk +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 1 2 3 4 6 chunks +16 lines, -40 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 2 chunks +10 lines, -12 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
Manuel Rego
2 years, 1 month ago (2017-01-31 12:55:27 UTC) #2
svillar
Awesome removal of code! I don't fully understand your changes though.... https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): ...
2 years, 1 month ago (2017-01-31 15:28:40 UTC) #3
Manuel Rego
I also think all this positioned elements stuff is pretty complex. I've tried to explain ...
2 years, 1 month ago (2017-02-01 09:08:20 UTC) #4
svillar
https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode3701 third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: } On 2017/02/01 09:08:20, Manuel Rego wrote: > On ...
2 years, 1 month ago (2017-02-01 09:49:07 UTC) #5
Manuel Rego
https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode3701 third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: } On 2017/02/01 09:49:07, svillar wrote: > On 2017/02/01 ...
2 years, 1 month ago (2017-02-01 11:08:08 UTC) #6
Manuel Rego
https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2665133003/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode3701 third_party/WebKit/Source/core/layout/LayoutBox.cpp:3701: } On 2017/02/01 11:08:08, Manuel Rego wrote: > Actually ...
2 years, 1 month ago (2017-02-01 11:10:34 UTC) #7
Manuel Rego
Coming back to this after a long time... First I've rebased the patch which was ...
1 year, 10 months ago (2017-05-08 10:48:09 UTC) #9
svillar
The patch looks much better now. Also it's great that you remove the hacks from ...
1 year, 10 months ago (2017-05-08 11:18:10 UTC) #10
jfernandez
Wow, now it's extremely simple. I like it, but let's address first sergio's comments. https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Source/core/layout/LayoutBox.cpp ...
1 year, 10 months ago (2017-05-08 12:56:07 UTC) #11
Manuel Rego
Thanks for the reviews. Uploaded new version, PTAL. https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2665133003/diff/60001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode3867 third_party/WebKit/Source/core/layout/LayoutBox.cpp:3867: // ...
1 year, 10 months ago (2017-05-08 13:29:44 UTC) #12
svillar
Amazing fix. lgtm
1 year, 10 months ago (2017-05-08 14:50:55 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2665133003/80001
1 year, 10 months ago (2017-05-08 17:20:38 UTC) #15
commit-bot: I haz the power
1 year, 10 months ago (2017-05-08 18:47:28 UTC) #18
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e48a88ace117744e50c9f9769b40...

Powered by Google App Engine
This is Rietveld 408576698