Bug 181677 - [css-grid] Spanning Grid item has too much space at the bottom / is too high
Summary: [css-grid] Spanning Grid item has too much space at the bottom / is too high
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: InRadar
: 181645 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-01-16 05:27 PST by Tobi Reif
Modified: 2018-02-01 03:01 PST (History)
8 users (show)

See Also:


Attachments
screenshot of https://tobireif.com/demos/grid/ at medium sized window (233.01 KB, image/png)
2018-01-16 05:27 PST, Tobi Reif
no flags Details
Patch (19.32 KB, patch)
2018-01-22 03:15 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (19.37 KB, patch)
2018-01-22 03:33 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobi Reif 2018-01-16 05:27:45 PST
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
Comment 1 Tobi Reif 2018-01-16 05:28:32 PST
*** Bug 181645 has been marked as a duplicate of this bug. ***
Comment 2 Tobi Reif 2018-01-16 07:08:31 PST
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
Comment 3 Tobi Reif 2018-01-16 07:26:33 PST
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.
Comment 4 Tobi Reif 2018-01-16 08:55:52 PST
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."
Comment 5 Tobi Reif 2018-01-16 13:54:59 PST
Another reduced test case, based on one by Manuel Rego:

http://jsbin.com/dusetovifa/1/edit?html,css,output
Comment 7 Tobi Reif 2018-01-18 03:28:48 PST
The spec could be clearer it seems:

https://github.com/w3c/csswg-drafts/issues/2201
Comment 8 Manuel Rego Casasnovas 2018-01-22 03:15:10 PST
Created attachment 331909 [details]
Patch
Comment 9 Javier Fernandez 2018-01-22 03:28:54 PST
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.
Comment 10 Manuel Rego Casasnovas 2018-01-22 03:33:12 PST
Created attachment 331910 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2018-01-22 03:33:58 PST
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 12 WebKit Commit Bot 2018-01-22 04:33:39 PST
Comment on attachment 331910 [details]
Patch

Clearing flags on attachment: 331910

Committed r227288: <https://trac.webkit.org/changeset/227288>
Comment 13 WebKit Commit Bot 2018-01-22 04:33:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-01-22 04:34:30 PST
<rdar://problem/36730534>
Comment 15 Tobi Reif 2018-01-23 01:08:26 PST
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?
Comment 16 Manuel Rego Casasnovas 2018-01-23 03:04:21 PST
(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.
Comment 17 Tobi Reif 2018-01-23 03:19:47 PST
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)
Comment 18 Tobi Reif 2018-01-24 13:03:17 PST
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!
Comment 19 Tobi Reif 2018-02-01 02:58:39 PST
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?
Comment 20 Sergio Villar Senin 2018-02-01 03:01:29 PST
(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.