Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(299)

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

Created:
3 years, 7 months ago by jfernandez
Modified:
3 years, 5 months 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

Messages

Total messages: 29 (9 generated)
jfernandez
PTAL
3 years, 7 months 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. ...
3 years, 7 months 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 > ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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): ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months ago (2017-05-12 08:53:36 UTC) #11
jfernandez
3 years, 7 months 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 ...
3 years, 7 months 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 years, 7 months 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 years, 6 months 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 ...
3 years, 5 months 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
3 years, 5 months 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 ...
3 years, 5 months 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
3 years, 5 months 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, ...
3 years, 5 months 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 ...
3 years, 5 months ago (2017-07-11 19:58:38 UTC) #28
jeffcarp
3 years, 5 months 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.

Powered by Google App Engine
This is Rietveld 408576698