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 Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, donggwan.kim, hyatt, jfernandez, ojan, rego, rniwa, svillar, tony, xan.lopez

Priority:

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

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	Details Formatted Diff Diff
Applied suggested changes. (3.62 KB, patch) 2014-03-28 17:24 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
Finally decided to implement it as a reftest. (4.09 KB, patch) 2014-04-02 03:50 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Applying suggested changes. (5.61 KB, patch) 2014-04-03 05:22 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
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.