Bug 163535 - [css-grid] Different width of grid container between initial load and refresh
Summary: [css-grid] Different width of grid container between initial load and refresh
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2016-10-17 00:18 PDT by Javier Fernandez
Modified: 2016-10-18 05:36 PDT (History)
12 users (show)

See Also:


Attachments
Patch (6.93 KB, patch)
2016-10-17 00:38 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (793.23 KB, application/zip)
2016-10-17 01:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1015.50 KB, application/zip)
2016-10-17 01:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.74 MB, application/zip)
2016-10-17 01:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (9.33 MB, application/zip)
2016-10-17 01:50 PDT, Build Bot
no flags Details
Patch (16.62 KB, patch)
2016-10-17 03:21 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (16.38 KB, patch)
2016-10-17 05:23 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (16.46 KB, patch)
2016-10-17 06:52 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (16.46 KB, patch)
2016-10-17 10:18 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2016-10-17 00:18:06 PDT
If you open the following Layout Test [1]:
LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html

The first time the width of the first grid container is 110px, but if you resize the window or reload the page, the width is 120px.

We should investigate why we're getting different results depending on the layout.

I tried to create a reduced test case but it seems that I cannot reproduce it without the JavaScript involved on the test.

I managed to reproduce it inside JS Bin too (resize the output frame to see it):
http://jsbin.com/milawa/edit?html,output

[1] https://trac.webkit.org/browser/trunk/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
Comment 1 Javier Fernandez 2016-10-17 00:38:08 PDT
Created attachment 291795 [details]
Patch
Comment 2 Build Bot 2016-10-17 01:37:06 PDT
Comment on attachment 291795 [details]
Patch

Attachment 291795 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2302965

New failing tests:
fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
Comment 3 Build Bot 2016-10-17 01:37:10 PDT
Created attachment 291798 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Build Bot 2016-10-17 01:38:00 PDT
Comment on attachment 291795 [details]
Patch

Attachment 291795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2302969

New failing tests:
fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
Comment 5 Build Bot 2016-10-17 01:38:03 PDT
Created attachment 291799 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-10-17 01:43:16 PDT
Comment on attachment 291795 [details]
Patch

Attachment 291795 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2302970

New failing tests:
fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
Comment 7 Build Bot 2016-10-17 01:43:20 PDT
Created attachment 291801 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2016-10-17 01:50:03 PDT
Comment on attachment 291795 [details]
Patch

Attachment 291795 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2302978

New failing tests:
fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
Comment 9 Build Bot 2016-10-17 01:50:07 PDT
Created attachment 291802 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Javier Fernandez 2016-10-17 03:21:15 PDT
Created attachment 291805 [details]
Patch

Fixed layout tests.
Comment 11 Manuel Rego Casasnovas 2016-10-17 05:07:14 PDT
Comment on attachment 291805 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        algorithm depend on these override sizes.

Typo: s/depend/depends/

> Source/WebCore/ChangeLog:22
> +        clear these override sizes and performs a layout of the affected grid

Typo: s/performs/perform/

> Source/WebCore/rendering/RenderGrid.cpp:465
> +    // We need to clear both own and containingBlock override sizes to
> +    // ensure we get the same result when grid's intrinsic size is
> +    // computed again in the updateLogicalWidth call bellow.

We do this only for orthogonal items, shouldn't we say that on the comment.

> Source/WebCore/rendering/RenderGrid.cpp:471
> +    // We do cache orthogonal items during the placeItemsOnGrid call, which is
> +    // executed later. However, we are
> +    // only interested on running this logic when we are performing a
> +    // relayout, hence we have already cached
> +    // the orthogonal items.

Mmm, you don't use cached orthogonal items here.
Is this comment accurate?

> LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html:19
> +.i1 {
> +  color: magenta;
> +  background: cyan;
> +}

Nit: Maybe you can use firstRowFirstColumn class which is more descriptive
and already has a background color.
And you just add here the color for the text.

> LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html:20
> +.i2 {

Ditto but using firstRowSecondColumn.

> LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html:21
> +  color: red;

It's weird to use "red" if there's not a fail in a test.

> LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html:30
> +        document.body.offsetLeft;
> +        document.body.offsetLeft;

Do you need to repeat twice this line?
Comment 12 Manuel Rego Casasnovas 2016-10-17 05:12:12 PDT
For future reference this has been already solved in Blink:
https://codereview.chromium.org/2361373002/
Comment 13 Javier Fernandez 2016-10-17 05:21:09 PDT
Comment on attachment 291805 [details]
Patch

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

>> LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html:30
>> +        document.body.offsetLeft;
> 
> Do you need to repeat twice this line?

Well, perhaps it's too much, but I wanted to be really sure that many layouts doesn't produce different results. I guess we can live with just one extra layout, but I guess we can keep as it is, so we performs the same test than Blink.
Comment 14 Javier Fernandez 2016-10-17 05:23:41 PDT
Created attachment 291810 [details]
Patch

Applied suggsted changes.
Comment 15 Javier Fernandez 2016-10-17 06:52:58 PDT
Created attachment 291814 [details]
Patch

Properly forcing relayout in the test.
Comment 16 Javier Fernandez 2016-10-17 10:18:38 PDT
Created attachment 291829 [details]
Patch

Patch for landing.
Comment 17 WebKit Commit Bot 2016-10-18 05:36:10 PDT
Comment on attachment 291829 [details]
Patch

Clearing flags on attachment: 291829

Committed r207460: <http://trac.webkit.org/changeset/207460>
Comment 18 WebKit Commit Bot 2016-10-18 05:36:16 PDT
All reviewed patches have been landed.  Closing bug.