Created attachment 331382 [details] screenshot of https://tobireif.com/demos/grid/ at medium sized window Open (the work-in-progress demo) https://tobireif.com/demos/grid/ , set the viewport width to approximately 900px. Scroll down. Notice that below the text "No HTML code needs to be changed" there's too much space at the bottom of that Grid item (element "main"), and that it's not caused eg by margin or padding - the superfluous space at the bottom of the Grid item / the larger-than-content height of the Grid item seems to be Grid-related. (Perhaps it's the same size as the Grid gap?) A stable copy of the page (in case the layout at the above URL changed and doesn't show the issue anymore) is at https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/ . There should be no superfluous space at the bottom of the spanning Grid item "main". Screenshots of the issue in several browsers: (Each browser window was set to a medium width so that the layout appears in which the issue occurs.) Safari: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284674/snapshots/z1343e3cf89987fc0db2 Chrome: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284631/snapshots/z11754b1173ad93e2863 Firefox: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284747/snapshots/z1ce3ac3f5b88fe30c52 Seems OK in Edge?: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284744/snapshots/z261445e0327d014027f
*** Bug 181645 has been marked as a duplicate of this bug. ***
Reported the issue for some of the other browsers as well: https://bugs.chromium.org/p/chromium/issues/detail?id=802021 https://bugzilla.mozilla.org/show_bug.cgi?id=1430757
This might be good as reduced test case: https://bugs.chromium.org/p/chromium/issues/attachment?aid=320224 (from https://bugs.chromium.org/p/chromium/issues/detail?id=802021 ) In any case, I hope that the issue I describe in the report can get fixed so that the layout/spacing at https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/ will be correct.
From the Chrome/Chromium/Blink bug report page: https://bugs.chromium.org/p/chromium/issues/detail?id=802021#c16 : Manuel Rego: "Ok, I've been checking the algorithm and the code and I found our bug. The problem is when we were finding the size of an "fr" (https://drafts.csswg.org/css-grid/#algo-find-fr-size). The spec says: "Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks." We were removing the gaps from the "leftover space" when the grid has a definite height, but not when it has an indefinite one. I'll work on a patch to fix this."
Another reduced test case, based on one by Manuel Rego: http://jsbin.com/dusetovifa/1/edit?html,css,output
(The test case https://bugs.chromium.org/p/chromium/issues/attachment?aid=320224 might require the font https://www.w3.org/Style/CSS/Test/Fonts/Ahem/ .)
The spec could be clearer it seems: https://github.com/w3c/csswg-drafts/issues/2201
Created attachment 331909 [details] Patch
Comment on attachment 331909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331909&action=review > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:854 > + - renderGrid()->guttersSize(m_algorithm.grid(), direction, span.startLine(), span.integerSpan(), availableSpace()); nit: I don't think we need the muti-line format here.
Created attachment 331910 [details] Patch
Comment on attachment 331909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331909&action=review >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:854 >> + - renderGrid()->guttersSize(m_algorithm.grid(), direction, span.startLine(), span.integerSpan(), availableSpace()); > > nit: I don't think we need the muti-line format here. ACK.
Comment on attachment 331910 [details] Patch Clearing flags on attachment: 331910 Committed r227288: <https://trac.webkit.org/changeset/227288>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36730534>
I order to verify that it works now, I downloaded the latest build https://s3-us-west-2.amazonaws.com/minified-archives.webkit.org/mac-highsierra-x86_64-release/227382.zip from https://webkit.org/build-archives/ . In MiniBrowser.app , the spanning Grid item is too long. Should it work in that browser? Otherwise - how to test?
(In reply to Tobi Reif from comment #15) > I order to verify that it works now, I downloaded the latest build > https://s3-us-west-2.amazonaws.com/minified-archives.webkit.org/mac- > highsierra-x86_64-release/227382.zip > from > https://webkit.org/build-archives/ . > > In MiniBrowser.app , the spanning Grid item is too long. Should it work in > that browser? Otherwise - how to test? In theory that revision should be enough, but I'm not 100% sure about what's included in those builds. Maybe you need to wait for a fresher one, or wait for the next Safari Technology Preview. At least here in Linux and trunk it's working as intended, and the tests are passing in all platforms.
I'd never doubt that you verified the fix thoroughly! I just need to verify here as well 😁 I'll try again in a newer WebKit nightly. Can anyone confirm that eg tomorrow's MiniBrowser.app (from https://webkit.org/build-archives/ ) should have all the latest code including the fix for this reported issue? (bug 181677)
It works! The latest code is reflected in the browser that opens when I run this command in the dir from https://webkit.org/build-archives/ : $ ./run-webkit-archive (... and not when I run http://MiniBrowser.app from the same zip.) I can verify the fix: both of the following pages get rendered correctly regarding the reported issue: https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/ http://jsbin.com/dusetovifa/1/edit?html,css,output Thanks Manuel Rego et al!
I need to adapt my layouts at https://tobireif.com/demos/grid/ as soon as this fix is in Safari (and mobile Safari) stable. In which Safari version (stable / delivered to users) will this fix be included?
(In reply to Tobi Reif from comment #19) > I need to adapt my layouts at https://tobireif.com/demos/grid/ as soon as > this fix is in Safari (and mobile Safari) stable. In which Safari version > (stable / delivered to users) will this fix be included? Unfortunately Apple never publishes information about future releases.