Bug 179959 – [css-grid] Centered auto margin not recalculated when overflow changes on display: grid item

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Layout and Rendering

[Javier Fernandez]

Created attachment 327473 [details]
Test case to reproduce the bug

Steps to reproduce the problem:
With an element that is `display: grid` and has a scrollbar, and an element that is `margin: auto` for left & right

1. On grid element, change overflow to `hidden`
2. Wait at least 1 frame draw
3. Change grid element overflow back to `auto`

Margin auto should be recalculated and there shouldn't be horizontal overflow if none existed before. However, The element with margin auto retains it's previous size and causes a horizontal scrollbar to appear, even if one did not exist before.

[Javier Fernandez]

Created attachment 327486 [details]
Patch

[EWS Watchlist]

Comment on attachment 327486 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-container-scroll-accounts-for-auto-margin.html

[EWS Watchlist]

Created attachment 327491 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 327486 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-container-scroll-accounts-for-auto-margin.html

[EWS Watchlist]

Created attachment 327492 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 327486 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-container-scroll-accounts-for-auto-margin.html

[EWS Watchlist]

Created attachment 327493 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Javier Fernandez]

This is actually a bug originally reported in Blink, so I assumed it was affecting the Grid WebKit implementation, since the code is practically the same for this logic. 

The original problem in Blink was that we are using the resolved 'auto' margins if any style change implies a full layout of the grid, but the grid item with 'auto' margins doesn't need a layout. In this specific scenario we were wrongly used the previously computed 'auto' margins during the tracks sizing algorithm.

The test case in the Blink report consists of changing the overflow property from 'auto', to hidden and then get it back to 'auto'. 

However, the same test case doesn't trigger the bug in the WK implementation. The reason is that in WK such style changes implies a layout, not only for the grid but also on all its children. If the children need layout we compute again the 'auto' margins, which must be 0 during the track sizing algorithm, according to the Grid spec.

Even though the bug is not reproducible with the described test case, we indeed need that change on our 'auto' margin logic. There may be other situations were the grid needs to be laid out again while the children don't. 

However, I think it's better to close this bug and land the patch in a different bug.