Bug 103573

Summary: [CSS Grid Layout] Support implicit rows and columns
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 103332    
Bug Blocks: 60731    
Attachments:
Description Flags
Test case
none
Proposed implementation: Requires auto sizing to pass EWS. none

Julien Chaffraix
Reported 2012-11-28 16:19:43 PST
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.
Attachments
Test case (671 bytes, text/html)
2012-11-28 16:19 PST, Julien Chaffraix 		no flags
Details
Proposed implementation: Requires auto sizing to pass EWS. (17.81 KB, patch)
2013-01-30 12:06 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Xan Lopez
Comment 1 2012-12-08 08:24:05 PST
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.
Julien Chaffraix
Comment 2 2013-01-30 12:06:31 PST
Created attachment 185533 [details] Proposed implementation: Requires auto sizing to pass EWS.
Ojan Vafai
Comment 3 2013-01-30 13:18:04 PST
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?
Julien Chaffraix
Comment 4 2013-01-30 13:32:28 PST
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.
Ojan Vafai
Comment 5 2013-01-30 14:54:11 PST
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.
Build Bot
Comment 6 2013-01-31 03:34:19 PST
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
Julien Chaffraix
Comment 7 2013-01-31 09:36:04 PST
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.
WebKit Review Bot
Comment 8 2013-01-31 16:49:38 PST
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>
WebKit Review Bot
Comment 9 2013-01-31 16:49:43 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.