[css-grid] Using `span` in grid-row-end results in incorrect placement of grid item
Reported by
uhyohyo...@gmail.com,
Dec 2
|
||||||||||||
Issue descriptionUserAgent: 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:
,
Dec 3
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..!
,
Dec 3
++Correction..
,
Dec 3
As this is regressed in M70, not a blocker for M71. Pls target fix for M72.
,
Dec 10
Friendly ping to get an update as per C#4 & it is marked as RB stable. Thanks..!
,
Dec 17
Gentle ping to get an update as it is marked as RBS. Thanks..!
,
Dec 17
rego, could you find someone to look into this please?
,
Dec 17
We'll take a look, sorry but it was not tagged as Grid so we were not notified.
,
Dec 17
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.
,
Dec 17
,
Dec 18
,
Dec 18
Attaching an even more reduced test case.
,
Dec 18
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
,
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
,
Dec 19
,
Dec 20
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
,
Dec 20
Seems like this has been around for a while. Why is this critical for M72? How safe is this merge?
,
Dec 20
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.
,
Dec 20
Given that this has been broken awhile and the complexity of the change I would lean towards not down integrating this into M72.
,
Dec 20
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.
,
Dec 21
Let's target M73.
,
Dec 31
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by phanindra.mandapaka@chromium.org
, Dec 2