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

Issue 2868283002: [css-grid] Check if baseline alignment affects grid areas sizing (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by jfernandez
Modified:
2 months, 1 week ago
CC:
chromium-reviews, blink-reviews-w3ctests_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Check if baseline alignment affects grid areas sizing The bug this CL intend to fix has a very specific root cause; we were wrongly assuming that we need to repeat the track sizing algorithm only if baseline alignment may change the grid container's intrinsic size. Actually, the spec states that we should run a new iteration of the sizing algorithm if any of the grid items has changed its min-content contribution. This affects to the grid item's container, hence the grid area, but not necessarily to the grid container. Since we were running the baseline alignment logic after the first run of the tracks sizing algorithm, we were relying on the extra run to adjust the grid areas' size based on the resulting baseline alignment offsets. This patch fixes the bug by comparing the grid areas's size with the baseline extent of their items, triggering the extra tracks sizing cycle when necessary. BUG=719445 Review-Url: https://codereview.chromium.org/2868283002 Cr-Commit-Position: refs/heads/master@{#485518} Committed: https://chromium.googlesource.com/chromium/src/+/3d184fc5f831f45ca9a5f75f3d04f33e3f5dc2df

Patch Set 1 #

Total comments: 1

Patch Set 2 : New approach #

Total comments: 16

Patch Set 3 : Applied suggested changes. #

Total comments: 8

Patch Set 4 : Applied suggested changes. #

Total comments: 1

Patch Set 5 : Applied suggested changes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+692 lines, -22 lines) Patch
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html View 1 2 3 1 chunk +54 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-002.html View 1 2 3 1 chunk +55 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-003.html View 1 2 3 1 chunk +56 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-004.html View 1 2 3 1 chunk +52 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-005.html View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-006.html View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-007.html View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html View 1 2 3 1 chunk +60 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-009.html View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-010.html View 1 2 3 1 chunk +56 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-011.html View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-012.html View 1 2 1 chunk +56 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 3 chunks +23 lines, -20 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 29 (9 generated)
jfernandez
PTAL
4 months, 1 week ago (2017-05-10 08:12:41 UTC) #2
svillar
On 2017/05/10 08:12:41, jfernandez wrote: > PTAL I think we have talked about this before. ...
4 months, 1 week ago (2017-05-10 08:48:22 UTC) #3
jfernandez
On 2017/05/10 08:48:22, svillar wrote: > On 2017/05/10 08:12:41, jfernandez wrote: > > PTAL > ...
4 months, 1 week ago (2017-05-10 09:36:16 UTC) #4
svillar
https://codereview.chromium.org/2868283002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2868283002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode196 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:196: if (!track_sizing_algorithm_.GetGrid().HasAnyOrthogonalGridItem()) This method is executed only as part ...
4 months, 1 week ago (2017-05-11 07:15:04 UTC) #5
jfernandez
This new approach just fixes the buggy function, but it follows the already implemented approach ...
4 months, 1 week ago (2017-05-11 08:30:32 UTC) #7
Manuel Rego
Some comments on the first test that apply to the rest. https://codereview.chromium.org/2868283002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html File third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html (right): ...
4 months, 1 week ago (2017-05-11 14:10:09 UTC) #8
jfernandez
PTAL https://codereview.chromium.org/2868283002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html File third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html (right): https://codereview.chromium.org/2868283002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html#newcode3 third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html:3: <title>CSS Grid Layout Test: Self-Baseline alignment may change ...
4 months, 1 week ago (2017-05-11 21:51:31 UTC) #9
svillar
https://codereview.chromium.org/2868283002/diff/40001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2868283002/diff/40001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode223 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:223: StyleRef().LogicalHeight().IsIntrinsicOrAuto()) { I am a bit confused by all ...
4 months, 1 week ago (2017-05-12 08:28:59 UTC) #10
jfernandez
https://codereview.chromium.org/2868283002/diff/40001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2868283002/diff/40001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode223 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:223: StyleRef().LogicalHeight().IsIntrinsicOrAuto()) { On 2017/05/12 08:28:59, svillar wrote: > I ...
4 months, 1 week ago (2017-05-12 08:53:36 UTC) #11
jfernandez
4 months, 1 week ago (2017-05-15 09:05:13 UTC) #13
Manuel Rego
https://codereview.chromium.org/2868283002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2868283002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1887 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1887: if (!track_size.IsContentSized()) On 2017/05/11 21:51:30, jfernandez wrote: > On ...
4 months, 1 week ago (2017-05-16 10:29:39 UTC) #14
svillar
There is a typo in the title as well: alingment -> alignment https://codereview.chromium.org/2868283002/diff/60001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp ...
3 months, 4 weeks ago (2017-05-25 07:26:28 UTC) #15
jfernandez
PTAL https://codereview.chromium.org/2868283002/diff/60001/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html File third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html (right): https://codereview.chromium.org/2868283002/diff/60001/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html#newcode48 third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-001.html:48: <div style="top: 0px; left: 50px; font-size: 20px">X</div> On ...
3 months, 3 weeks ago (2017-05-30 10:39:30 UTC) #18
svillar
lgtm https://codereview.chromium.org/2868283002/diff/100001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2868283002/diff/100001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1883 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1883: (group.MaxAscent() + group.MaxDescent() > grid_area_size)) We don't really ...
2 months, 2 weeks ago (2017-07-04 09:46:57 UTC) #19
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/2868283002/120001
2 months, 1 week ago (2017-07-10 23:53:41 UTC) #22
jfernandez
@jeffcarp for some reason this CL hasn't triggered the auto-exporter to generate the associated WPT ...
2 months, 1 week ago (2017-07-11 00:12:53 UTC) #23
commit-bot: I haz the power
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3d184fc5f831f45ca9a5f75f3d04f33e3f5dc2df
2 months, 1 week ago (2017-07-11 03:49:30 UTC) #26
jeffcarp
On 2017/07/11 at 03:49:30, commit-bot wrote: > Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3d184fc5f831f45ca9a5f75f3d04f33e3f5dc2df @jfernandez apologies, ...
2 months, 1 week ago (2017-07-11 15:59:15 UTC) #27
jfernandez
On 2017/07/11 15:59:15, jeffcarp wrote: > On 2017/07/11 at 03:49:30, commit-bot wrote: > > Committed ...
2 months, 1 week ago (2017-07-11 19:58:38 UTC) #28
jeffcarp
2 months, 1 week ago (2017-07-11 21:59:30 UTC) #29
Message was sent while issue was closed.
On 2017/07/11 at 15:59:15, jeffcarp wrote:
> On 2017/07/11 at 03:49:30, commit-bot wrote:
> > Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3d184fc5f831f45ca9a5f75f3d04...
> 
> @jfernandez apologies, I'll look into it!

@jfernandez after this CL landed an export PR was created and successfully
merged: https://github.com/w3c/web-platform-tests/pull/6509

This is expected behavior. For Rietveld code review (this site), provisional PRs
are not created. For all future exportable changes, please use Gerrit
(https://chromium-review.googlesource.com/). It's the default now, so `git cl
upload` will upload there.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b