RESOLVED FIXED 103473
[CSS Grid Layout] Make sure grid element's shrink-to-fit behavior is correct 
https://bugs.webkit.org/show_bug.cgi?id=103473
Summary [CSS Grid Layout] Make sure grid element's shrink-to-fit behavior is correct
Julien Chaffraix
Reported 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.
Attachments
Test case (1.41 KB, text/html)
2012-11-27 17:51 PST, Julien Chaffraix 		no flags
Details
Patch (3.78 KB, patch)
2014-03-07 15:43 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Applied suggested changes. (3.62 KB, patch)
2014-03-28 17:24 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Finally decided to implement it as a reftest. (4.09 KB, patch)
2014-04-02 03:50 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Using flex track sizes to actually notice the shrink-to-fit effect. (5.78 KB, patch)
2014-04-02 15:57 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (571.35 KB, application/zip)
2014-04-02 16:26 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (611.56 KB, application/zip)
2014-04-02 18:43 PDT, Build Bot 		no flags
Details
Using a relative positioned container. (6.02 KB, patch)
2014-04-03 02:43 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Applying suggested changes. (5.61 KB, patch)
2014-04-03 05:22 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 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.
Julien Chaffraix
Comment 2 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.
Julien Chaffraix
Comment 3 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)
Javier Fernandez
Comment 4 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.
Javier Fernandez
Comment 5 2014-03-07 15:43:04 PST
Created attachment 226176 [details] Patch
Sergio Villar Senin
Comment 6 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.
Javier Fernandez
Comment 7 2014-03-28 17:24:43 PDT
Created attachment 228099 [details] Applied suggested changes.
Sergio Villar Senin
Comment 8 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.
Javier Fernandez
Comment 9 2014-04-02 03:50:31 PDT
Created attachment 228381 [details] Finally decided to implement it as a reftest.
Sergio Villar Senin
Comment 10 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.
Javier Fernandez
Comment 11 2014-04-02 15:57:00 PDT
Created attachment 228436 [details] Using flex track sizes to actually notice the shrink-to-fit effect.
Build Bot
Comment 12 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
Build Bot
Comment 13 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
Build Bot
Comment 14 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
Build Bot
Comment 15 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
Javier Fernandez
Comment 16 2014-04-03 02:43:18 PDT
Created attachment 228493 [details] Using a relative positioned container.
Sergio Villar Senin
Comment 17 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.
Sergio Villar Senin
Comment 18 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.
Javier Fernandez
Comment 19 2014-04-03 05:22:34 PDT
Created attachment 228497 [details] Applying suggested changes.
Sergio Villar Senin
Comment 20 2014-04-03 07:20:30 PDT
Comment on attachment 228497 [details] Applying suggested changes. Awesome, thanks.
WebKit Commit Bot
Comment 21 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>
WebKit Commit Bot
Comment 22 2014-04-03 07:51:27 PDT
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.