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.
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.
> 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.
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)
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.
Created attachment 226176 [details] Patch
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.
Created attachment 228099 [details] Applied suggested changes.
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.
Created attachment 228381 [details] Finally decided to implement it as a reftest.
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.
Created attachment 228436 [details] Using flex track sizes to actually notice the shrink-to-fit effect.
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
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 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
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
Created attachment 228493 [details] Using a relative positioned container.
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 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.
Created attachment 228497 [details] Applying suggested changes.
Comment on attachment 228497 [details] Applying suggested changes. Awesome, thanks.
Comment on attachment 228497 [details] Applying suggested changes. Clearing flags on attachment: 228497 Committed r166717: <http://trac.webkit.org/changeset/166717>
All reviewed patches have been landed. Closing bug.