Bug 181677 – [css-grid] Spanning Grid item has too much space at the bottom / is too high

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

[Tobi Reif]

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

[Tobi Reif]

*** Bug 181645 has been marked as a duplicate of this bug. ***

[Tobi Reif]

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

[Tobi Reif]

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.

[Tobi Reif]

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."

[Tobi Reif]

Another reduced test case, based on one by Manuel Rego:

http://jsbin.com/dusetovifa/1/edit?html,css,output

[Tobi Reif]

(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/ .)

[Tobi Reif]

The spec could be clearer it seems:

https://github.com/w3c/csswg-drafts/issues/2201

[Manuel Rego Casasnovas]

Created attachment 331909 [details]
Patch

[Javier Fernandez]

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.

[Manuel Rego Casasnovas]

Created attachment 331910 [details]
Patch

[Manuel Rego Casasnovas]

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.

[WebKit Commit Bot]

Comment on attachment 331910 [details]
Patch

Clearing flags on attachment: 331910

Committed r227288: <https://trac.webkit.org/changeset/227288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36730534>

[Tobi Reif]

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?

[Manuel Rego Casasnovas]

(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.

[Tobi Reif]

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)

[Tobi Reif]

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!

[Tobi Reif]

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?

[Sergio Villar Senin]

(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.