Created attachment 176596 [details] Test case We don't account for implicit rows and columns in the current code. After bug 102968 we will crash on the following test case as we try to access past the track vector boundary in RenderGrid::layoutGridItems. I will land a work-around as part of bug 103343 as we were hitting this but we still won't properly handle the positioning properly.
This seems simple enough except for the small detail that implicit columns or rows are supposedly created with 'auto' size (minmax(min-content,max-content)), so I think to properly implement this we first need bug #103332.
Created attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS.
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. View in context: https://bugs.webkit.org/attachment.cgi?id=185533&action=review > Source/WebCore/rendering/RenderGrid.cpp:258 > + maximumIndex = std::max(maximumIndex, resolveGridPosition(position) + 1); Can we ever get in this situation other than by not setting grid-rows/grid-columns? If not, then we'd effectively only need to std::max(maximumIndex, 1), right? If there are cases where we need this loop, do we have test coverage for those cases?
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. View in context: https://bugs.webkit.org/attachment.cgi?id=185533&action=review >> Source/WebCore/rendering/RenderGrid.cpp:258 >> + maximumIndex = std::max(maximumIndex, resolveGridPosition(position) + 1); > > Can we ever get in this situation other than by not setting grid-rows/grid-columns? If not, then we'd effectively only need to std::max(maximumIndex, 1), right? If there are cases where we need this loop, do we have test coverage for those cases? Nothing prevents people from putting a grid item in a random grid area, regardless of what is specified on the grid element's. See http://www.w3.org/TR/css3-grid-layout/#implicit-columns-and-rows for a self-contained example of that. The tests are covering this situation but I can add something similar to the specification example if you think this would be less confusing. Also we should probably cache this information as it is tied to changing the grid items' or grid element's style but I didn't want a deep change for now.
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. View in context: https://bugs.webkit.org/attachment.cgi?id=185533&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:258 >>> + maximumIndex = std::max(maximumIndex, resolveGridPosition(position) + 1); >> >> Can we ever get in this situation other than by not setting grid-rows/grid-columns? If not, then we'd effectively only need to std::max(maximumIndex, 1), right? If there are cases where we need this loop, do we have test coverage for those cases? > > Nothing prevents people from putting a grid item in a random grid area, regardless of what is specified on the grid element's. See http://www.w3.org/TR/css3-grid-layout/#implicit-columns-and-rows for a self-contained example of that. > > The tests are covering this situation but I can add something similar to the specification example if you think this would be less confusing. > > Also we should probably cache this information as it is tied to changing the grid items' or grid element's style but I didn't want a deep change for now. Got it.
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. Attachment 185533 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16278123
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. The win failure seems to be a sick bot. The patch does build on win which means it's ready for review.
Comment on attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS. Clearing flags on attachment: 185533 Committed r141505: <http://trac.webkit.org/changeset/141505>
All reviewed patches have been landed. Closing bug.