New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 688640 link

Starred by 19 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 79180



Sign in to add a comment

[css-grid] Increase the maximum row number at grid

Reported by utasirob...@gmail.com, Feb 4 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce the problem:
open this testcase: https://jsfiddle.net/0jcrxm45/1/
Clearly visible if rows are broken after 1000.
This is a big problem, when we try to make masonry layout by using grid.

Here is a sample: https://jsfiddle.net/utasir/0yp0o80z/8/

Normally this shows 100 random pics as masonry, but now it is cut off.

What is the expected behavior?
accepted rows can be 2^32 pieces, or more :)

What went wrong?
The number of rows is limited.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 55.0.2883.87  Channel: canary
OS Version: 10.0
Flash Version: Shockwave Flash 24.0 r0

 

Comment 1 by woxxom@gmail.com, Feb 4 2017

Bisect:
444816 (good) - 444825 (bad), 57.0.2987.0
https://chromium.googlesource.com/chromium/src/+log/cc273dda..a397e24f?pretty=fuller
Suspecting r444818 "[css-grid] Limit the track range to [-999, 999]"
Not sure why it affects v55 mentioned in the report, but maybe it was back-merged?
This is a real-life testcase, a masonry/pinterest gallery with a lazy loads. normally it loads 50pictures, and when scroll down by a scroll hyjack it loads 50pics again and again. Currently it does not what we need whilst 1000 lines are not enough loading 50pics either. So the suggested way minimum 256 000 lines, that should be enough for 58-60 lazy loads by this realistic testcase.

Note: this is the official suggested way currently at w3c by tabatkins to make masonry layout.
gridmax.html
1.1 KB View Download
Probably better way to limit the row-spanning itself instead of the whole grid if it is causing some stacking problems. The grid can be flowed "unlimited".

Comment 4 by r...@igalia.com, Feb 6 2017

Cc: svil...@igalia.com jfernan...@igalia.com r...@igalia.com
Components: -Blink>Layout Blink>Layout>Grid
Labels: -OS-Windows OS-All
Status: Untriaged (was: Unconfirmed)
So far due to memory issues we need to cap the size of the grid,
it'd be nice to see how the discussion in GitHub evolves:
https://github.com/w3c/csswg-drafts/issues/1009

Also we should keep an eye on the Firefox bug too:
https://bugzilla.mozilla.org/show_bug.cgi?id=1336679
Can you comment the GitHub with this memory issues?

I have an idea, maybe I can reflect there with it: 

1000x1000= 1000000 means 1 million cells. Can the row get priority by this way?:

If rows = X then the allowed cols = sqrt(1000000-X). This is inside the 1million cells limit.
Or I am in a wrong way?
No , it was wrong. sorry,
so shortly:

1000rows 1000columns
10000rows 100colums
100000rows 10columns
1000000rows 1 columns
rows = 10^X -> cols = 10^(6-X) this is correct and keeps 1million cells.
by this priority way.

Comment 9 by e...@chromium.org, Feb 7 2017

Status: Available (was: Untriaged)
We haven't chosen to do that (neither Firefox) because that would be super-confusing for users. A fixed limit is much more understandable than the variable one you suggest.

Also that won't help, because we would hit the same out-of-memory issues we have seen before if we try to allocate, say 100000 rows even if we have just 10 columns.

The solution is not to create those weird sizing rules, but implement a more memory efficient representation of the grid, something that is already in our TODOs. Grid is going to be shipped, so the focus was features and stability. Once we get this then we can move to performance, something that indeed requires use cases like the one you're describing.

Comment 11 by r...@igalia.com, Feb 7 2017

Blocking: 79180
Summary: [css-grid] Increase the maximum row number at grid (was: [grid] Increase the maximum row number at grid )
The reference bugs were closed:

https://github.com/w3c/csswg-drafts/issues/1009 by tabatkins
https://bugzilla.mozilla.org/show_bug.cgi?id=1336679 by me

whilst masonry/pinterest layout was moved to css-grid-2.

However I suggest increasing the 1000x1000 value to 10000x10000 because it is really small and not enough for a "one screen masonry" either. That requires 4000x1440 or so.

Comment 13 by a...@scirra.com, May 26 2017

We just ran in to this limit with our PWA at editor.construct.net. Kind of amazed there's an arbitrary limit imposed! We use a CSS grid to display user content, and the user can add 1000 rows of content. A user just did that and filed a bug with us, but we can't fix it!

Comment 14 by svil...@igalia.com, May 26 2017

We're working on a new internal representation for the grid which would allow bigger grids. It's difficult to get a right balance between memory efficient representations and speed.

Comment 15 by phistuck@gmail.com, May 26 2017

#13 - are you referring to the event list where you can add events? That seems like a real table. Using grid layout for that seems a bit weird to me. Perhaps just switch to <table>?

Comment 16 by a...@scirra.com, May 26 2017

Table layout is only really suitable for tabular data lists. We did originally prototype that view using both block layout, table layout and CSS grid. CSS grid was by far the best - easiest to get working and fewest DOM elements required.

Comment 17 by e...@chromium.org, May 26 2017

 Issue 726708  has been merged into this issue.

Comment 18 by r...@igalia.com, Nov 10 2017

 Issue 783736  has been merged into this issue.
Cc: viswatej...@techmahindra.com sc00335...@techmahindra.com
 Issue 793279  has been merged into this issue.
Just ran into that bug too.
So after one year, there is still no solution???
Firefox works fine and doesn't show any sign of struggle.
Please fix this.

Comment 21 by r...@igalia.com, Mar 22 2018

 Issue 824394  has been merged into this issue.
Issue 824398 has been merged into this issue.
any update on this?

Comment 24 by svil...@igalia.com, Apr 16 2018

We're working on this. It'll take some time as the solution is not easy at all. Note that you normally have to balance speed and memory consumption. If we reduce a lot the memory consumption then speed will be reduced too (because the memory structure used to store the grid will be much more complicated). Hopefully we could get to something in the following weeks.
 Issue 841762  has been merged into this issue.
Cc: susan.boorgula@chromium.org
 Issue 860463  has been merged into this issue.
Project Member

Comment 27 by bugdroid1@chromium.org, Jul 25

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

commit b9351e20a47f21d28527b55d49e2191f0a08845f
Author: Sergio Villar Senin <svillar@igalia.com>
Date: Wed Jul 25 13:23:14 2018

[css-grid] New list-based data structure for the Grid

The data structure used to store the grid items is currently a matrix,
a Vector of Vectors. That's pretty convenient because it's quite easy
to use, to implement and also provides O(1) access to grid cells and
easy row/column traversals. However the problem is that we need to
allocate the whole size of the grid even if we have just a small
number of items on it (sparse matrix).

We're replacing it by a new data structure based on doubly linked
list. We'd be only creating the grid rows/columns/cells required to
store the actual grid items inside the grid. The drawback of this new
implementation is that O(1) access to individual cells is no longer
possible. However that's not the most usual way to access data on the
grid, most of the times we simply iterate through tracks. It's also a
bit more complex to implement, but the good thing is that it does not
require any change in the Grid class API, so the current layout code
is unaffected by this change.

Last but not least, although this change involves memory saves with
sparse grids it does not completely fix the issue of dealing with
large grids. That's because the track sizing algorithm is still using
Vectors to store the list of tracks, so we have to do large
allocations of those. This should be addressed in follow-up patches
where content sized empty tracks would be ignored and not allocated by
the algorithm.

Bug: 688640
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I4d44914ed976358dcb5ef92cc3ff9df7a720102e
Reviewed-on: https://chromium-review.googlesource.com/1104166
Commit-Queue: Sergio Villar <svillar@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577855}
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/layout/grid.cc
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/layout/grid.h
[add] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/layout/grid_test.cc
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/layout/layout_grid.cc
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/layout/layout_grid.h
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/core/paint/grid_painter.cc
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/platform/wtf/BUILD.gn
[modify] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/platform/wtf/doubly_linked_list.h
[add] https://crrev.com/b9351e20a47f21d28527b55d49e2191f0a08845f/third_party/blink/renderer/platform/wtf/doubly_linked_list_test.cc

I think it's fair to have a row limit. But all other browsers use a limit of at least 10.000 rows. The minimum of 10.000 rows is also in the spec: https://drafts.csswg.org/css-grid/#overlarge-grids, second sentence. With the above mentioned performance improvements, would that open up the door for a limit of 10.000 rows in Chrome?

I think this makes a masonry layout work for most usecases. And allows a pixel perfect layout of 10.000px heigh. 20.000px if you're willing to use steps of 2px. Even larger layouts can work with 3, 4 or 5px steps.
Now that the new structure has been here for some months, and no important issues were detected, perhaps it's time to increase the limit. @rego, @jfernandez WDYT?
Project Member

Comment 30 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

Sign in to add a comment