Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Sign in to add a comment
[CSS Grid Layout] Support implicit grid before explicit grid
Project Member Reported by r...@igalia.com, Dec 19 2014 Back to list
Currently the implicit grid is only after the explicit grid.

For example, in a 2x2 grid, if you have an item with:
  grid-row: 3;
An implicit 3rd row is created.

However, if you have an item like:
  grid-column: -4;
We're creating an implicit column before the first one.

We've to add support for this. It's important to take into consideration some stuff:
* Current internal structure (m_grid) might be a trouble to support implicit rows/columns before the grid.
* Clamping the implicit grid should also consider negative values and not only positive.

More info in the spec: http://dev.w3.org/csswg/css-grid/#line-placement
 
Comment 1 by r...@igalia.com, Dec 19 2014
Blocking: chromium:79180
Comment 2 by laforge@google.com, Jan 9 2015
Labels: -Cr-Blink-Rendering-Grid Cr-Blink-Layout-Grid
Migrate from Cr-Blink-Rendering-Grid to Cr-Blink-Layout-Grid
Comment 3 by svil...@igalia.com, Apr 30 2015
Blocking: chromium:392200
Comment 4 by r...@igalia.com, Nov 12 2015
Blocking: chromium:442954
Comment 5 by r...@igalia.com, Nov 12 2015
Blocking: chromium:273898
Comment 6 by r...@igalia.com, Nov 12 2015
Owner: r...@igalia.com
Status: Assigned
Comment 7 by r...@igalia.com, Nov 13 2015
I've been thinking about the best plan to support this (see comment https://codereview.chromium.org/1424913009/#msg17).

I crafted a small document explaining the steps to be done to support the implicit grid before the explicit grid:
https://docs.google.com/document/d/1D9x8IUZxirhc3RUma16L6p0yJGv8h199GMnmBA6zLAI/edit?usp=sharing

I'll follow it and associate the different CLs with this bug.

Any feedback is welcomed!
Project Member Comment 8 by bugdroid1@chromium.org, Nov 18 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2031b1892af1ca54c5b85660f6f369979e548fdb

commit 2031b1892af1ca54c5b85660f6f369979e548fdb
Author: rego <rego@igalia.com>
Date: Wed Nov 18 17:58:07 2015

[css-grid] Store lines instead of tracks in GridResolvedPosition

Due to the new feature that allows to create implicit tracks before the
explicit ones, we will need to use lines instead of tracks in the
code to be able to implement it properly.

This is just a first simple patch using lines instead of tracks in
GridResolvedPosition. It modifies the code that was using it, as it was
considering that the resolvedFinalPosition was a track index and
not a line index.

So if we've an item positioned like:
    grid-column: 2 / 5;
    grid-row: 1 / span 2;

Before we were storing this information on the GridSpan:
    * columns:
        * resolvedInitialPosition: 1
        * resolvedFinalPosition:  3
    * rows:
        * resolvedInitialPosition: 0
        * resolvedFinalPosition:  1

And now we're storing:
    * columns:
        * resolvedInitialPosition: 1
        * resolvedFinalPosition:  4
    * rows:
        * resolvedInitialPosition: 0
        * resolvedFinalPosition:  2

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1451883002

Cr-Commit-Position: refs/heads/master@{#360361}

[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/layout/LayoutGrid.h
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/paint/GridPainter.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/style/GridCoordinate.h
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp
[modify] http://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb/third_party/WebKit/Source/core/style/GridResolvedPosition.h

Project Member Comment 9 by bugdroid1@chromium.org, Nov 23 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ac0ca9794efed848b7546eeb54fdf7d7850c680

commit 1ac0ca9794efed848b7546eeb54fdf7d7850c680
Author: rego <rego@igalia.com>
Date: Mon Nov 23 16:25:34 2015

[css-grid] Refactor GridSpan to avoid pointers

Add new boolean to know if a GridSpan is definite or indefinite.
That way we don't need to use pointers and we can always have two
GridSpans in GridCoordinate, if the position is "auto" the GridSpan will
be marked as indefinite. This will allow in a follow-up patch to avoid
repeated calls to methods that resolve positions.

Most operations in GridSpan are restricted to definite GridSpans (access
to positions, iterator, etc.). For indefinite GridSpans we only need to
know that they're indefinite we shouldn't use the rest of the data.

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1459373002

Cr-Commit-Position: refs/heads/master@{#361119}

[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/paint/GridPainter.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/style/GridCoordinate.h
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp
[modify] http://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680/third_party/WebKit/Source/core/style/GridResolvedPosition.h

Project Member Comment 10 by bugdroid1@chromium.org, Nov 26 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7e24b5853a11308a12b302b34c9f624f2acd367

commit a7e24b5853a11308a12b302b34c9f624f2acd367
Author: rego <rego@igalia.com>
Date: Thu Nov 26 09:32:20 2015

[css-grid] Avoid duplicated calls to resolution code

We were calling GridResolvedPosition::resolveGridPositionsFromStyle()
several times per item.

We can store the GridCoordinates in
LayoutGrid::populateExplicitGridAndOrderIterator()
and reuse them in the placement code.
Once LayoutGrid::placeItemsOnGrid() is over,
all the items will have a definite position in both axis.

No new tests, no change of behavior.

BUG= 444011 

GridResolvedPosition::resolveGridPositionsFromStyle

Review URL: https://codereview.chromium.org/1465153004

Cr-Commit-Position: refs/heads/master@{#361855}

[modify] http://crrev.com/a7e24b5853a11308a12b302b34c9f624f2acd367/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Project Member Comment 11 by bugdroid1@chromium.org, Nov 27 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24b7188272ac4e05a9831da49224ddcb5caf0336

commit 24b7188272ac4e05a9831da49224ddcb5caf0336
Author: rego <rego@igalia.com>
Date: Fri Nov 27 14:24:41 2015

[css-grid] Simplify GridResolvedPosition interface

Move static methods from GridResolvedPosition public interface as they
were only used internally.
Also move some static methods from GridSpan that were only used by
GridResolvedPosition.
Last, remove unused method in GridCoordinate.

Based on a previous patch by Sergio Villar <svillar@igalia.com>
https://codereview.chromium.org/1424913009/

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1477203002

Cr-Commit-Position: refs/heads/master@{#361998}

[modify] http://crrev.com/24b7188272ac4e05a9831da49224ddcb5caf0336/third_party/WebKit/Source/core/style/GridCoordinate.h
[modify] http://crrev.com/24b7188272ac4e05a9831da49224ddcb5caf0336/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp
[modify] http://crrev.com/24b7188272ac4e05a9831da49224ddcb5caf0336/third_party/WebKit/Source/core/style/GridResolvedPosition.h

Project Member Comment 12 by bugdroid1@chromium.org, Dec 6 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ad965028770aec5042ea3587843aec66027e1da

commit 3ad965028770aec5042ea3587843aec66027e1da
Author: rego <rego@igalia.com>
Date: Sun Dec 06 22:41:22 2015

[css-grid] Get rid of GridResolvedPosition

GridResolvedPosition was a small class just wrapping a size_t.
In the future it should actually wrap an integer
(as we want to support implicit tracks before the explicit grid).

The class itself is not providing any benefit,
so we can get rid of it and store directly 2 size_t in GridSpan.

This will make simpler future changes related to this task.

We keep the class just as a utility for the methods
that deal with the positions resolution.
But it should be renamed in a follow-up patch.

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1500433003

Cr-Commit-Position: refs/heads/master@{#363384}

[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/paint/GridPainter.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/style/GridCoordinate.h
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp
[modify] http://crrev.com/3ad965028770aec5042ea3587843aec66027e1da/third_party/WebKit/Source/core/style/GridResolvedPosition.h

Project Member Comment 13 by bugdroid1@chromium.org, Dec 7 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/302423d65cd31d4ba94cbc177612bf26f3305af6

commit 302423d65cd31d4ba94cbc177612bf26f3305af6
Author: rego <rego@igalia.com>
Date: Mon Dec 07 12:25:50 2015

[css-grid] Simplify method to resolve auto-placed items

Refactor the method to resolve auto-placed items,
for which we're only interested in knowing the span size.

Adapt the calls to use the span size instead of a GridSpan.

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1496863004

Cr-Commit-Position: refs/heads/master@{#363457}

[modify] http://crrev.com/302423d65cd31d4ba94cbc177612bf26f3305af6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/302423d65cd31d4ba94cbc177612bf26f3305af6/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp
[modify] http://crrev.com/302423d65cd31d4ba94cbc177612bf26f3305af6/third_party/WebKit/Source/core/style/GridResolvedPosition.h

Project Member Comment 14 by bugdroid1@chromium.org, Jan 5 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7433487e4082c928a06ddff51fe5c963a28da3ef

commit 7433487e4082c928a06ddff51fe5c963a28da3ef
Author: rego <rego@igalia.com>
Date: Tue Jan 05 12:55:40 2016

[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

Review URL: https://codereview.chromium.org/1529083006

Cr-Commit-Position: refs/heads/master@{#367511}

[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-negative-position-resolution.html
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-spanning-resolution.html
[add] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/layout/LayoutGrid.h
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/paint/GridPainter.cpp
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/style/GridCoordinate.h
[modify] http://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp

Comment 15 by r...@igalia.com, Jan 7 2016
Status: Fixed
Comment 16 by r...@igalia.com, Jan 7 2016
Status: Assigned
Initial support is ready, but we still have to do some refactorings before closing the bug.
Comment 17 by svil...@igalia.com, Jan 20 2016
Should we close this rego?
Comment 18 by r...@igalia.com, Jan 21 2016
Not yet, as I said in comment #16 I want to do some extra refactorings on the code that are part of the plan described in comment #7.

So, if you don't mind let's keep this open for a few more time. :-)
Project Member Comment 20 by bugdroid1@chromium.org, Feb 1 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2b19519116145ad2e1baa24a3965726dc0eabc6

commit d2b19519116145ad2e1baa24a3965726dc0eabc6
Author: rego <rego@igalia.com>
Date: Mon Feb 01 11:21:37 2016

[css-grid] Rename GridCoordinate to GridArea

As the comment in GridCoordinate says,
it actually represents a grid area as it stores
the initial and final positions in both axis (columns and rows).

Someone can think about a grid coordinate just like a single cell.
However this class was representing an area of several cells.

On top of that the "grid area" concept is defined in the spec:
https://drafts.csswg.org/css-grid/#grid-area-concept

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1648333002

Cr-Commit-Position: refs/heads/master@{#372638}

[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/core.gypi
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.h
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/layout/LayoutGrid.h
[rename] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/style/GridArea.h
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/style/GridPositionsResolver.cpp
[modify] http://crrev.com/d2b19519116145ad2e1baa24a3965726dc0eabc6/third_party/WebKit/Source/core/style/StyleGridData.h

Project Member Comment 21 by bugdroid1@chromium.org, Mar 18 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1f4acccc0a6f34ae712788f7b18c61876644463

commit f1f4acccc0a6f34ae712788f7b18c61876644463
Author: rego <rego@igalia.com>
Date: Fri Mar 18 10:57:57 2016

[css-grid] Rename GridSpan properties

GridSpan was using old names initialResolvedPosition and
finalResolvedPosition.
This patch rename them to startLine and endLine.

Some reasons for this refactoring:
* "position" is a vague term not defined in the spec.
* GridSpan is currently storing grid lines. A grid "line" is defined
  in the spec: https://drafts.csswg.org/css-grid/#grid-line-concept
* The spec uses the concepts "start" and "end" lines too.

No new tests, no change of behavior.

BUG= 444011 

Review URL: https://codereview.chromium.org/1808193002

Cr-Commit-Position: refs/heads/master@{#381930}

[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp
[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/style/GridArea.h
[modify] https://crrev.com/f1f4acccc0a6f34ae712788f7b18c61876644463/third_party/WebKit/Source/core/style/GridPositionsResolver.cpp

Comment 22 by r...@igalia.com, Mar 18 2016
Status: Fixed
With the last refactorings there're no more planned work on the placement code in the short term.
Sign in to add a comment