Issue 1529083006: [css-grid] Initial support for implicit grid before explicit grid

Issue 1529083006: [css-grid] Initial support for implicit grid before explicit grid (Closed)

Created:
5 years ago by Manuel Rego

Modified:
4 years, 11 months ago

Reviewers:
cbiesinger, svillar, jfernandez, Timothy Loh, rune

CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, blink-reviews-style_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[css-grid] Initial support for implicit grid before explicit grid Change GridSpan to store int instead of size_t. This allows us to resolve positions before the explicit grid with negative values. This patch adds a new type of GridSpan called "Untranslated". This type is only used in populateExplicitGridAndOrderIterator(). Where we store the smallest negative position in both axis. Then the GridSpans are translated into positive values, using the offset calculated before. This is done in placeItemsOnGrid() and from that moment the rest of the code uses "Definite" GridSpans, which returns only positive positions (size_t instead of int). This allows us to don't have to modify the rest of the code, as it keeps using GridSpans as before. Let's use an example to explain how it works. Imagine that we've a 2 columns grid and 2 items placed like: * Item A: grid-column: -5; * Item B: grid-column: 1; Initially we'll use "Unstranslated" GridSpans with the following values: * Item A: GridSpan(-2, -1) * Item B: GridSpan(0, 1) Then we'll translate them using the smallest position as offset (-2) so we've "Definite" GridSpans: * Item A: GridSpan(0, 1) * Item B: GridSpan(2, 3) Updated results in current tests and added specific test for this. BUG=444011 TEST=fast/css-grid-layout/implicit-tracks-before-explicit.html Committed: https://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef Cr-Commit-Position: refs/heads/master@{#367511}

Patch Set 1 #

Patch Set 2 : Try to fix Windows build #

Total comments: 24

Patch Set 3 : Apply suggested changes #

Total comments: 2

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+383 lines, -91 lines)			Patch
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html	View		2 chunks	+4 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-negative-position-resolution.html	View		4 chunks	+8 lines, -8 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-spanning-resolution.html	View		4 chunks	+8 lines, -6 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html	View	1 2	1 chunk	+236 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutGrid.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutGrid.cpp	View	1 2	14 chunks	+35 lines, -22 lines	0 comments	Download
M	third_party/WebKit/Source/core/paint/GridPainter.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/style/GridCoordinate.h	View	1 2	4 chunks	+67 lines, -18 lines	2 comments	Download
M	third_party/WebKit/Source/core/style/GridResolvedPosition.cpp	View	1 2	9 chunks	+19 lines, -28 lines	0 comments	Download

Messages

Total messages: 17 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

svillar

The mechanics look good but I have many doubts about the proposed API (although perhaps ...

5 years ago (2015-12-18 13:08:30 UTC) #3

jfernandez

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html (right): https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html#newcode57 third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html:57: <div class="item autoRowAutoColumn" data-offset-x="0" data-offset-y="0" data-expected-width="100" data-expected-height="50">XX</div> Where is ...

5 years ago (2015-12-18 14:33:22 UTC) #5

Manuel Rego

Thanks for the reviews! I've renamed the GridSpan types, I hope it's clearer now. https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html ...

5 years ago (2015-12-18 22:42:30 UTC) #6

Thanks for the reviews!

I've renamed the GridSpan types, I hope it's clearer now.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html
(right):

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html:57:
<div class="item autoRowAutoColumn" data-offset-x="0" data-offset-y="0"
data-expected-width="100" data-expected-height="50">XX</div>
On 2015/12/18 14:33:21, jfernandez wrote:
> Where is the 'item' class defined ?

Good catch, dunno from where it comes. I've removed it.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html:60:
<div class="grid">
On 2015/12/18 14:33:21, jfernandez wrote:
> Are we able to properly see each test cases in the content_shell ? Wouldn't we
> need a margin or a block container for each grid test case ?

It's true that we cannot see them easily.

So I've added background colours to the items and also margins to the grid.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1178: m_smallestColumnStart
= 0;
On 2015/12/18 13:08:30, svillar wrote:
> Using a single line is allowed in this case.

Acknowledged.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1497:
positions.translate(direction == ForColumns ? m_smallestColumnStart :
m_smallestRowStart);
On 2015/12/18 13:08:30, svillar wrote:
> Not sure why we need this here...

Basically positioned objects are skipped in
populateExplicitGridAndOrderIterator() and placeItemsOnGrid().

We need to resolve the positions here and translate them as usual.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridCoordinate.h (right):

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:53: static GridSpan
untranslatedGridSpan(int resolvedInitialPosition, int resolvedFinalPosition)
On 2015/12/18 14:33:21, jfernandez wrote:
> I find the 'unstranslated" concept a bit obscure. Also, I'd expect something
not
> translated as the regular thing, so why adding a prefix for defining it ? 

Untranslated is only used during the code to resolve positions.
The rest (most) of the code use the translated positions.

Probably you can find more context in the following document:
http://bit.ly/1j3CRdi

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:136: }
On 2015/12/18 13:08:30, svillar wrote:
> I don't like this API because is super-confusing. 
> 
> If isDefinite() returns false then most of developers will think that the
> position is definite. It's quite weird that !isDefinite() != isIndefinite(),
> this could be a source of programming errors.

I agree this is confusing.

So I've changed the possible types of GridSpan, now the possible types are:
UntranslatedDefinite, TranslatedDefinite and Indefinite.

Indefinite are unknown positions so we don't need to know
if they're untranslated or translated, those don't make sense
for indefinite positions.

The general idea is that during the position resolution code,
we always work with UntranslatedDefinite or Indefinite positions.
Then, in placeItemsOnGrid() we translate those positions to
TransaltedDefinite and the rest of the code always use
translated positions.

I've also renamed isDefinite() to isTranslatedDefinite() so the meaning is
clear.

I hope this changes make the patch more readable
but any other suggestion is welcomed.

> Also it looks to me that we need some more ASSERTs to check that we're not
using
> untranslated positions like for example when using and iterator

We already had ASSERTs in begin() and end() checking
that we're working with translated positions.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:145: 
On 2015/12/18 13:08:30, svillar wrote:
> Looks like it's missing the kGridMaxTrack checks?

I don't think we need them here.
We've already the checks in GridSpan constructor,
so we know we've as maximum kGridMaxTrack * 2 tracks.

This will be just translating the positions to positive values,
but not creating extra tracks.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:147:
ASSERT(m_resolvedFinalPosition > 0);
On 2015/12/18 14:33:21, jfernandez wrote:
> I don't get the purpose of these asserts. If 'offset' is an unsigned value,
how
> this operation may result in a m_resolvedXXX fields attributes negative or
zero
> ? 

I think you're missing the fact that m_resolvedInitialPosition can be negative.
Imagine that m_resolvedInitialPosition is "-5", and we pass an offest of "2"
(because of we calculate it wrong).
We'll end up in "-3" which is not a translated position (it should be >= 0).

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:166:
m_resolvedInitialPosition = std::min(resolvedInitialPosition,
static_cast<int>(kGridMaxTracks) - 1);
On 2015/12/18 13:08:30, svillar wrote:
> Just convert kGridMaxTracks to int and remove the casts.

Done.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:166:
m_resolvedInitialPosition = std::min(resolvedInitialPosition,
static_cast<int>(kGridMaxTracks) - 1);
On 2015/12/18 14:33:21, jfernandez wrote:
> I think many other casts have been added because of this; we may want to
remove
> them all.

I've converted it to int so I think we don't have issues anymore.

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:129: return
GridSpan::untranslatedGridSpan(resolvedOppositePosition - positionOffset,
resolvedOppositePosition);
On 2015/12/18 14:33:21, jfernandez wrote:
> Are we handling here the case where resolvedOppositePosition is zero ? 

Precisely now we support negative values, so we don't care.

If resolvedOppositePosition is "0" and positionOffset is "2"
we simply return GridSpan(-2, 0).

svillar

The naming is indeed much better but I think we still need another iteration. https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h ...

4 years, 11 months ago (2016-01-04 09:10:06 UTC) #7

Manuel Rego

Thanks for the review. https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode62 third_party/WebKit/Source/core/style/GridCoordinate.h:62: On 2016/01/04 09:10:06, svillar wrote: ...

4 years, 11 months ago (2016-01-04 10:00:11 UTC) #8

svillar

OK, lgtm for layout/, you'll need other owners for style/ and parser/ stuff

4 years, 11 months ago (2016-01-04 11:01:05 UTC) #9

Manuel Rego

Thanks for the new review! @rune and/or @timloh could you please take a look?

4 years, 11 months ago (2016-01-04 11:35:58 UTC) #11

Timothy Loh

On 2016/01/04 11:35:58, Manuel Rego wrote: > Thanks for the new review! > > @rune ...

4 years, 11 months ago (2016-01-05 02:31:47 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529083006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529083006/40001

4 years, 11 months ago (2016-01-05 10:31:18 UTC) #14

commit-bot: I haz the power

Description was changed from ========== [css-grid] Initial support for implicit grid before explicit grid Change ...

4 years, 11 months ago (2016-01-05 12:56:45 UTC) #16

Message was sent while issue was closed.

Description was changed from

==========
[css-grid] Initial support for implicit grid before explicit grid

Change GridSpan to store int instead of size_t. This allows us to
resolve positions before the explicit grid with negative values.

This patch adds a new type of GridSpan called "Untranslated".
This type is only used in populateExplicitGridAndOrderIterator().
Where we store the smallest negative position in both axis.

Then the GridSpans are translated into positive values, using the offset
calculated before. This is done in placeItemsOnGrid() and from that
moment the rest of the code uses "Definite" GridSpans, which returns
only positive positions (size_t instead of int).
This allows us to don't have to modify the rest of the code, as it keeps
using GridSpans as before.

Let's use an example to explain how it works. Imagine that we've a 2
columns grid and 2 items placed like:
* Item A: grid-column: -5;
* Item B: grid-column: 1;

Initially we'll use "Unstranslated" GridSpans with the following values:
* Item A: GridSpan(-2, -1)
* Item B: GridSpan(0, 1)

Then we'll translate them using the smallest position as offset (-2)
so we've "Definite" GridSpans:
* Item A: GridSpan(0, 1)
* Item B: GridSpan(2, 3)

Updated results in current tests and added specific test for this.

BUG=444011
TEST=fast/css-grid-layout/implicit-tracks-before-explicit.html
==========

to

==========
[css-grid] Initial support for implicit grid before explicit grid

Change GridSpan to store int instead of size_t. This allows us to
resolve positions before the explicit grid with negative values.

This patch adds a new type of GridSpan called "Untranslated".
This type is only used in populateExplicitGridAndOrderIterator().
Where we store the smallest negative position in both axis.

Then the GridSpans are translated into positive values, using the offset
calculated before. This is done in placeItemsOnGrid() and from that
moment the rest of the code uses "Definite" GridSpans, which returns
only positive positions (size_t instead of int).
This allows us to don't have to modify the rest of the code, as it keeps
using GridSpans as before.

Let's use an example to explain how it works. Imagine that we've a 2
columns grid and 2 items placed like:
* Item A: grid-column: -5;
* Item B: grid-column: 1;

Initially we'll use "Unstranslated" GridSpans with the following values:
* Item A: GridSpan(-2, -1)
* Item B: GridSpan(0, 1)

Then we'll translate them using the smallest position as offset (-2)
so we've "Definite" GridSpans:
* Item A: GridSpan(0, 1)
* Item B: GridSpan(2, 3)

Updated results in current tests and added specific test for this.

BUG=444011
TEST=fast/css-grid-layout/implicit-tracks-before-explicit.html

Committed: https://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef
Cr-Commit-Position: refs/heads/master@{#367511}
==========

commit-bot: I haz the power

4 years, 11 months ago (2016-01-05 12:56:46 UTC) #17

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef
Cr-Commit-Position: refs/heads/master@{#367511}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages