Bug 103473

Summary: [CSS Grid Layout] Make sure grid element's shrink-to-fit behavior is correct
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, donggwan.kim, hyatt, jfernandez, ojan, rego, rniwa, svillar, tony, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128372    
Bug Blocks: 60731    
Attachments:
Description Flags
Test case
none
Patch
none
Applied suggested changes.
none
Finally decided to implement it as a reftest.
none
Using flex track sizes to actually notice the shrink-to-fit effect.
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Using a relative positioned container.
none
Applying suggested changes. none

Description Julien Chaffraix 2012-11-27 17:51:27 PST
Created attachment 176376 [details]
Test case

Per "Defining the Shrink-to-fit Behavior of Grid Elements", grid elements should shrink to fit using the grid track breadth as their minimum and preferred logical width.

Attached is a very simple reproduction, shamelessly borrowed from Xan.
Comment 1 Ojan Vafai 2012-11-27 22:14:20 PST
As I read the spec, it's only defining how shrink-to-fit should work on grid elements, not *when* we should shrink-to-fit them. By default the grid's width should size to it's container like any block element. It shrink-to-fits if you float it, absolutely position it, make it display:inline-grid or it's container is an inline-block.

I don't see anything in the spec saying that grids should shrink-to-fit by default.

I'm not convinced we get any of those cases correct. So layout tests, at the least, would be great.
Comment 2 Julien Chaffraix 2012-12-11 18:45:59 PST
> I don't see anything in the spec saying that grids should shrink-to-fit by default.

Agreed, I misread the part where they define shrink-to-fit.
Comment 3 Julien Chaffraix 2012-12-14 12:17:13 PST
Bug 104818 landed a test that makes use of the shrink-to-fit behavior on the grid element but it only covers a simple case (fixed grid + paddings & borders)
Comment 4 Javier Fernandez 2014-03-07 14:53:20 PST
The patch landed by Bug 128372 implements the shrink-to-fit behavior for floating and out-of-flow positioned grid elements. 

I'm going to implement a Layout Test to verify it works as expected.
Comment 5 Javier Fernandez 2014-03-07 15:43:04 PST
Created attachment 226176 [details]
Patch
Comment 6 Sergio Villar Senin 2014-03-10 05:00:37 PDT
Comment on attachment 226176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226176&action=review

Looks good to me, but I'd prefer Hyatt or any other rendering expert to take a look at it.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit-expected.txt:3
> +The following grids should be 400px * 400px, except the first one which uses 'relative' positioning.

This comment looks weird because we don't print any size information.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:56
> +<p>The following grids should be 400px * 400px, except the first one which uses 'relative' positioning.</p>

See comment in -expected.txt

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:58
> +<div class="grid" id="regularGrid" data-expected-height="400" data-expected-width="769">

The value for the width may differ depending on the platform because it's the width of the DRT viewport. Better set a fixed size for <body> (like 800x600) and use that value here.
Comment 7 Javier Fernandez 2014-03-28 17:24:43 PDT
Created attachment 228099 [details]
Applied suggested changes.
Comment 8 Sergio Villar Senin 2014-04-02 02:49:28 PDT
Comment on attachment 228099 [details]
Applied suggested changes.

Looks good. Before landing please remove the color/background stuff as it adds nothing to the test. Also I think the test case would much more descriptive if you set the width of the regular grid to a different value, like 500px.
Comment 9 Javier Fernandez 2014-04-02 03:50:31 PDT
Created attachment 228381 [details]
Finally decided to implement it as a reftest.
Comment 10 Sergio Villar Senin 2014-04-02 04:19:38 PDT
Comment on attachment 228381 [details]
Finally decided to implement it as a reftest.

View in context: https://bugs.webkit.org/attachment.cgi?id=228381&action=review

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit-expected.html:7
> +

Don't need to define & use this.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit-expected.html:10
> +    width: 100%;

Don't need to define width.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:52
> +}

See the comment bellow about grid items.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:62
> +  <div class="four"></div>

Do we need grid items?

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:69
> +  <div class="four"></div>

Ditto.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:76
> +  <div class="four"></div>

Ditto.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:83
> +  <div class="four"></div>

Ditto.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:86
> +

Remove empty lines.
Comment 11 Javier Fernandez 2014-04-02 15:57:00 PDT
Created attachment 228436 [details]
Using flex track sizes to actually notice the shrink-to-fit effect.
Comment 12 Build Bot 2014-04-02 16:26:22 PDT
Comment on attachment 228436 [details]
Using flex track sizes to actually notice the shrink-to-fit effect.

Attachment 228436 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6154428658745344

New failing tests:
fast/css-grid-layout/grid-element-shrink-to-fit.html
Comment 13 Build Bot 2014-04-02 16:26:27 PDT
Created attachment 228443 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Build Bot 2014-04-02 18:43:45 PDT
Comment on attachment 228436 [details]
Using flex track sizes to actually notice the shrink-to-fit effect.

Attachment 228436 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5648287969312768

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
fast/css-grid-layout/grid-element-shrink-to-fit.html
Comment 15 Build Bot 2014-04-02 18:43:49 PDT
Created attachment 228454 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 16 Javier Fernandez 2014-04-03 02:43:18 PDT
Created attachment 228493 [details]
Using a relative positioned container.
Comment 17 Sergio Villar Senin 2014-04-03 04:30:46 PDT
Comment on attachment 228493 [details]
Using a relative positioned container.

View in context: https://bugs.webkit.org/attachment.cgi?id=228493&action=review

Some style comments before the final review.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit-expected.html:26
> +

All these three are identical. Use a single one and rename it to something meaningful.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit-expected.html:94
> +     <div class="secondRowSecondColumn" style="top: 50px; left: 50%"></div>

You're always applying the same style for the same elements. You can just move it to the CSS declarations.

> LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:16
> +

Remove this empty declaration.
Comment 18 Sergio Villar Senin 2014-04-03 04:55:40 PDT
Comment on attachment 228493 [details]
Using a relative positioned container.

r=me with comments. Ah remove the margin thing as I think it isn't needed.
Comment 19 Javier Fernandez 2014-04-03 05:22:34 PDT
Created attachment 228497 [details]
Applying suggested changes.
Comment 20 Sergio Villar Senin 2014-04-03 07:20:30 PDT
Comment on attachment 228497 [details]
Applying suggested changes.

Awesome, thanks.
Comment 21 WebKit Commit Bot 2014-04-03 07:51:21 PDT
Comment on attachment 228497 [details]
Applying suggested changes.

Clearing flags on attachment: 228497

Committed r166717: <http://trac.webkit.org/changeset/166717>
Comment 22 WebKit Commit Bot 2014-04-03 07:51:27 PDT
All reviewed patches have been landed.  Closing bug.