New issue
Advanced search Search tips

Issue 910953 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

[css-grid] Using `span` in grid-row-end results in incorrect placement of grid item

Reported by uhyohyo...@gmail.com, Dec 2

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.110 Safari/537.36

Example URL:
https://codepen.io/uhyo/pen/RqvmQO

Steps to reproduce the problem:
Open https://codepen.io/uhyo/pen/RqvmQO
Or open a page with below content:

<style>
.grid {
  display: grid;
  grid-template-columns: 100px 100px 100px;
  grid-template-rows: repeat(, 100px);
  grid-auto-flow: row dense;
  gap: 10px;
}

.grid > div {
  width: 100px;
  min-height: 100px;
  border: 1px solid black;
}
.span {
  background-color: #cccccc;
  grid-column: 1;
  grid-row-start: auto;
  grid-row-end: span 2;
</style>
<div class="grid">
  <div>1</div>
  <div>2</div>
  <div>3</div>
  <div class="span">4</div>
  <div>5</div>
  <div>6</div>
  <div>7</div>
  <div>8</div>
  <div>9</div>
  <div>10</div>
  <div>11</div>
</div>

What is the expected behavior?
The box with label "4" should be placed on the second row.

What went wrong?
The box with label "4" is actually on the third row.
I'm not sure which behavior is spec-compliant, but at least Chrome disagrees with Firefox's behavior and Firefox's behavior seems more rational.

Does it occur on multiple sites: N/A

Is it a problem with a plugin? No 

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 70.0.3538.110  Channel: stable
OS Version: Ubuntu 16.04
Flash Version:
 
Labels: Needs-Triage-M70
Cc: phanindra.mandapaka@chromium.org
Components: Blink>CSS
Labels: -Pri-2 -Type-Compat hasbisect-per-revision RegressedIn-70 Triaged-ET ReleaseBlock-Stable Target-71 Target-72 Target-73 M-71 FoundIn-71 FoundIn-70 FoundIn-73 FoundIn-72 OS-Mac OS-Windows Pri-1 Type-Bug-Regression
Owner: svil...@igalia.com
Able to reproduce the issue on the reported chrome 67.0.3396.99,latest canary 69.0.3486.0 using Windows 10, Mac 10.13.6 and Ubuntu 17.10. Below is the bisect information for same.

Bisect Info:
================
Good build: 70.0.3502.0
Bad build:  70.0.3503.0

CHANGELOG URL:
You are probably looking for a change made after 577854 (known good), but no later than 577855 (first known bad).
https://chromium.googlesource.com/chromium/src/+log/448c3822b5021ccdb4e3a1ff89f02c13c7829266..b9351e20a47f21d28527b55d49e2191f0a08845f

Suspect: https://chromium.googlesource.com/chromium/src/+/b9351e20a47f21d28527b55d49e2191f0a08845f

Reviewed-on: https://chromium-review.googlesource.com/1104166 
Sergio Villar Senin:Please confirm the issue and help in re-assigning if it is not related to your change. Adding RBS label for M-71 feel free to change it if not required.

Thanks..!
Status: Assigned (was: Unconfirmed)
++Correction..
Labels: -M-71 -Target-71 M-72
As this is regressed in M70, not a blocker for M71. Pls target fix for M72.
Friendly ping to get an update as per C#4 & it is marked as RB stable.
Thanks..!

Gentle ping to get an update as it is marked as RBS.
Thanks..!
Cc: jfernan...@igalia.com r...@igalia.com
rego, could you find someone to look into this please?
Components: -Blink>CSS Blink>Layout>Grid
Owner: r...@igalia.com
We'll take a look, sorry but it was not tagged as Grid so we were not notified.
First of all, there are some errors in the example; the repeat(, 100px) syntax is invalid. Anyway, these errors doesn't affect the issue, which is definitively legit. 

Attached a simpler test case to reproduce the issue.

Additionally, it seems that without "dense", Chrome renders the test case like Firefox. So, I'm not totally sure the root cause of the issue is the use of 'span' as 'grid-row-end' value.
span-in-row-end-issue.html
552 bytes View Download
Attaching an even more reduced test case.
bug-auto-placement.html
314 bytes View Download
Status: Started (was: Assigned)
Summary: [css-grid] Using `span` in grid-row-end results in incorrect placement of grid item (was: Using `span` in grid-row-end results in incorrect placement of grid item )
Just for future reference I'm explaining my findings on this topic.

The reason for this regression is the new implementation of NextEmptyGridArea()
that is not working like the previous one.
In the previous code we have the following lines in CheckEmptyCells() [1]
  // Ignore cells outside current grid as we will grow it later if needed.
  size_t max_rows = std::min(row_index_ + row_span, matrix_.size());
  size_t max_columns = std::min(column_index_ + column_span, matrix_[0].size());

The example has an item with span 2, that before placing any item
already creates a grid with 2 tracks (as it follows the auto-placement algorithm [2]).
Then when we call NextEmptyGridArea() and the first track is already busy
we return nullptr.
Then we create a new area outside of the current grid,
so we do it in tracks 3rd and 4th (leaving empty 2nd track which is wrong).
The previous code was returning an empty area in tracks 2nd and 3rd
even when 3rd track didn't exist yet as it could be created on demand when required.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1104166/21/third_party/blink/renderer/core/layout/grid.cc
[2] https://drafts.csswg.org/css-grid/#auto-placement-algo
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59d4c1b34bfde4fbc31f7a40ab7d0e7df58ffd67

commit 59d4c1b34bfde4fbc31f7a40ab7d0e7df58ffd67
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Wed Dec 19 12:14:23 2018

[css-grid] Fix auto-placement on implicit tracks

This fixes a regression introduced in r577855
due to the new implementation of ListGridIterator::NextEmptyGridArea()
which was not taking into account some special situations
in the previous code.

When we call NextEmptyGridArea() it was returning null
when it couldn't find any empty grid area,
while the code in LayoutGrid was expecting an area anyway
even if it's outside the bounds of the current grid tracks.
This patch changes that code so it always returns an area,
and also removes some unneeded code in LayoutGrid
(of course it still keeps the code that checks that
we can add new tracks in the direction of the new area,
otherwise it discards it).

Let's explain what the new test is checking to understand the problem.
In the new tests cases the number of tracks of the grid is determined
by the largest span among all the auto-placed items. In the tests
we have an item that spans 2 tracks, so the grid is considered
to have 2 tracks as minimum. Then the first cell is occupied by an item,
so when we look for an empty area for the item spanning 2 tracks
we were not finding it. Then we were creating an new area outside
of the grid limits and leaving the second track empty (which is wrong).
Instead of that we're now returning an area taking
the 2nd and 3rd tracks, even when the 3rd track doesn't exist yet
as it's going to be created on demand.

BUG= 910953 ,688640
TEST=external/wpt/css/css-grid/placement/grid-auto-placement-implicit-tracks-001.html

Change-Id: If4eeb46a9d2a909b0772e75be90ff6dabd49c387
Reviewed-on: https://chromium-review.googlesource.com/c/1383055
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Sergio Villar <svillar@igalia.com>
Commit-Queue: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#617806}
[modify] https://crrev.com/59d4c1b34bfde4fbc31f7a40ab7d0e7df58ffd67/third_party/blink/renderer/core/layout/grid.cc
[modify] https://crrev.com/59d4c1b34bfde4fbc31f7a40ab7d0e7df58ffd67/third_party/blink/renderer/core/layout/layout_grid.cc
[add] https://crrev.com/59d4c1b34bfde4fbc31f7a40ab7d0e7df58ffd67/third_party/blink/web_tests/external/wpt/css/css-grid/placement/grid-auto-placement-implicit-tracks-001.html

Labels: Merge-Request-72
Project Member

Comment 16 by sheriffbot@chromium.org, Dec 20

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Seems like this has been around for a while. Why is this critical for M72? How safe is this merge?
Cc: e...@chromium.org
This was working fine, we broke it in M70 when we changed the internal structure of grid and it has been broken in M70, M71 and M72 now.
I don't know how critical it is, maybe someone with more experience like eae@ can comment on the topic.
About how safe it is, it's also hard to say, the patch is fixing the problem and we have a bunch of tests for that code (but it's true that the tests were not catching this regression) so you cannot be never 100% sure that no other regressions would be added because of this.

Given that this has been broken awhile and the complexity of the change I would lean towards not down integrating this into M72.
Thanks eae@, it sounds good to me.

Regarding the specific test case, as a workaround one option would be to define the explicit grid (bigger than the span in the item).
For example adding "grid-template-rows: 100px 100px 100px;" in the codepen (https://codepen.io/uhyo/pen/RqvmQO) would do the trick.
Labels: -Merge-Review-72 Merge-Rejected-72
Let's target M73. 
Status: Fixed (was: Started)

Sign in to add a comment