Bug 163535

Summary: [css-grid] Different width of grid container between initial load and refresh
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Javier Fernandez <jfernandez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kondapallykalyan, rego, rniwa, simon.fraser, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=628565
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch none

Javier Fernandez
Reported 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
Attachments
Patch (6.93 KB, patch)
2016-10-17 00:38 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (16.38 KB, patch)
2016-10-17 05:23 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (16.46 KB, patch)
2016-10-17 06:52 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (16.46 KB, patch)
2016-10-17 10:18 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2016-10-17 00:38:08 PDT
Created attachment 291795 [details] Patch
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Javier Fernandez
Comment 10 2016-10-17 03:21:15 PDT
Created attachment 291805 [details] Patch Fixed layout tests.
Manuel Rego Casasnovas
Comment 11 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?
Manuel Rego Casasnovas
Comment 12 2016-10-17 05:12:12 PDT
For future reference this has been already solved in Blink: https://codereview.chromium.org/2361373002/
Javier Fernandez
Comment 13 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.
Javier Fernandez
Comment 14 2016-10-17 05:23:41 PDT
Created attachment 291810 [details] Patch Applied suggsted changes.
Javier Fernandez
Comment 15 2016-10-17 06:52:58 PDT
Created attachment 291814 [details] Patch Properly forcing relayout in the test.
Javier Fernandez
Comment 16 2016-10-17 10:18:38 PDT
Created attachment 291829 [details] Patch Patch for landing.
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2016-10-18 05:36:16 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.