Bug 128372

Summary: [CSS Grid Layout] handle undefined RemainingSpace in computeUsedBreadthOfGridTracks algorithm
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, hyatt, jfernandez, kling, kondapallykalyan, rego, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731, 103473    
Attachments:
Description Flags
Patch
none
Patch rebased and applied suggested changed. none

Javier Fernandez
Reported 2014-02-07 03:26:04 PST
The spec defines a different code path for the computeUsedBreadthOfGridTracks algorithm http://dev.w3.org/csswg/css-grid/#function-ComputeUsedBreadthOfGridTracks. The current code has a FIXME that mentions this missing piece in our implementation. Basically the track breadth is different when the available size is undefined and thus, cannot be taken into account during the computations. The available size is undefined whenever the height is auto or the grid element has a shrink-to-fit behavior. It would be also a good idea to rename the function to match the specs so the function name will start with compute instead of computed.
Attachments
Patch (83.55 KB, patch)
2014-02-09 14:49 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch rebased and applied suggested changed. (83.56 KB, patch)
2014-02-21 14:18 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2014-02-09 14:49:30 PST
Created attachment 223649 [details] Patch
Dave Hyatt
Comment 2 2014-02-18 11:52:36 PST
Comment on attachment 223649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223649&action=review r=me, although concerned about the one function I called out. > Source/WebCore/rendering/RenderGrid.cpp:269 > +static bool gridElementIsShrinkToFit(const RenderStyle& style) > +{ > + return style.isFloating() || style.position() == AbsolutePosition; > +} This looks suspicious to me. Are you wanting to catch everybody that just sizes intrinsically? This is not enough if so.
Dave Hyatt
Comment 3 2014-02-18 11:54:04 PST
Comment on attachment 223649 [details] Patch Also, do you not have a renderer? Checking styles for floating and absolute is weird, since you don't know for sure that they applied. The RenderObject bits are better, i.e., isFloating() and isPositioned. We even have a helper function to test both at once.
Dave Hyatt
Comment 4 2014-02-18 11:54:34 PST
Comment on attachment 223649 [details] Patch I'm actually going to minus this, since I want clarification on this suspicious function.
Javier Fernandez
Comment 5 2014-02-19 03:59:14 PST
(In reply to comment #2) > (From update of attachment 223649 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223649&action=review > > r=me, although concerned about the one function I called out. > > > Source/WebCore/rendering/RenderGrid.cpp:269 > > +static bool gridElementIsShrinkToFit(const RenderStyle& style) > > +{ > > + return style.isFloating() || style.position() == AbsolutePosition; > > +} > > This looks suspicious to me. Are you wanting to catch everybody that just sizes intrinsically? This is not enough if so. Well, the patch tries to implement what it's stated in the Grid Layout specifications: http://www.w3.org/TR/css-grid-1/#RemainingSpace-definition "The max of zero and the AvailableSpace less the sum of all Grid track UsedBreadth values. This is undefined if AvailableSpace is undefined (i.e. the Grid element is shrink-to-fit or the height is auto.)" It wasn't obvious how to implement this shrink-to-fit behavior. so I assumed that it was triggered whenever a floating or positioned object is rendered. Would you mind suggesting some alternative approaches ?
Javier Fernandez
Comment 6 2014-02-19 04:00:37 PST
(In reply to comment #3) > (From update of attachment 223649 [details]) > Also, do you not have a renderer? Checking styles for floating and absolute is weird, since you don't know for sure that they applied. The RenderObject bits are better, i.e., isFloating() and isPositioned. We even have a helper function to test both at once. Yeah, you're totally right on this. If it happens that floating or positioned objects are the ones triggering the shrink-to-fit effect, then the patch should use the helper function you mentioned. However, let's clarify that first.
Javier Fernandez
Comment 7 2014-02-21 14:18:33 PST
Created attachment 224907 [details] Patch rebased and applied suggested changed.
Dave Hyatt
Comment 8 2014-02-24 13:51:49 PST
Comment on attachment 224907 [details] Patch rebased and applied suggested changed. r=me
WebKit Commit Bot
Comment 9 2014-02-24 14:23:55 PST
Comment on attachment 224907 [details] Patch rebased and applied suggested changed. Clearing flags on attachment: 224907 Committed r164609: <http://trac.webkit.org/changeset/164609>
WebKit Commit Bot
Comment 10 2014-02-24 14:23:58 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.