[css-grid] Increase the maximum row number at grid
Reported by
utasirob...@gmail.com,
Feb 4 2017
|
|||||
Issue descriptionUserAgent: 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
,
Feb 5 2017
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.
,
Feb 6 2017
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".
,
Feb 6 2017
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
,
Feb 6 2017
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?
,
Feb 6 2017
No , it was wrong. sorry,
,
Feb 6 2017
so shortly: 1000rows 1000columns 10000rows 100colums 100000rows 10columns 1000000rows 1 columns
,
Feb 6 2017
rows = 10^X -> cols = 10^(6-X) this is correct and keeps 1million cells. by this priority way.
,
Feb 7 2017
,
Feb 7 2017
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.
,
Feb 7 2017
,
Mar 31 2017
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.
,
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!
,
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.
,
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>?
,
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.
,
May 26 2017
Issue 726708 has been merged into this issue.
,
Nov 10 2017
Issue 783736 has been merged into this issue.
,
Dec 11 2017
Issue 793279 has been merged into this issue.
,
Mar 21 2018
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.
,
Mar 22 2018
Issue 824394 has been merged into this issue.
,
Mar 22 2018
Issue 824398 has been merged into this issue.
,
Apr 16 2018
any update on this?
,
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.
,
May 11 2018
Issue 841762 has been merged into this issue.
,
Jul 6
,
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
,
Oct 26
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.
,
Oct 29
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?
,
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 |
|||||
Comment 1 by woxxom@gmail.com
, Feb 4 2017