[css-grid] Spanning Grid item has too much space at the bottom / is too high
Reported by
t...@tobireif.com,
Jan 15 2018
|
|||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36 Steps to reproduce the problem: Open (the work-in-progress demo) https://tobireif.com/demos/grid/ , set the viewport width to approximately 900px. Scroll down. Right-click -> Inspect -> in the dev tools select the div with class "page", leave the pointer over that element. Notice that below the text "No HTML code needs to be changed" there's too much space inside that Grid item (element "main"), and that it's not cased eg by "margin" or "padding" values. (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/ . What is the expected behavior? The should be no superfluous space at the bottom of the spanning Grid item "main". What went wrong? The is superfluous space at the bottom of the spanning Grid item "main". Did this work before? N/A Does this work in other browsers? No Same issue in Safari, Firefox. Seems to be OK in Edge. Chrome version: 63.0.3239.132 Channel: stable OS Version: OS X 10.13.2 Flash Version: The attached simplified test-case is by fantasai - thanks!
,
Jan 15 2018
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.) Chrome: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284631/snapshots/z11754b1173ad93e2863 Firefox: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284747/snapshots/z1ce3ac3f5b88fe30c52 Safari: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284674/snapshots/z1343e3cf89987fc0db2 Seems OK in Edge?: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10284744/snapshots/z261445e0327d014027f
,
Jan 15 2018
Typo: The is superfluous space at the bottom of the spanning Grid item "main". -> There is superfluous space at the bottom of the spanning Grid item "main".
,
Jan 15 2018
I still need to take a deeper look, but on a first sight the test case says: "The size and contents of the four orange boxes below must be identical." Which is not true even in Edge, the first 2 grids are different than the other 2. The reason is that "auto 1fr" is different from "auto auto" when there are spanning items (BTW, I've recently opened an issue about this https://github.com/w3c/csswg-drafts/issues/2177).
,
Jan 15 2018
,
Jan 15 2018
The test-case if by fantasai, I hope she can elaborate on its text. The issue report itself is valid, I think: When you open https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/ , set the window width to ~900px, and make the Grid boxes appear using the dev tools, you can see that there's too much space at the bottom of the spanning element "main" which is not caused by margin or padding - the superfluous space in the Grid item / larger-than-content height of the Grid item seems to be Grid-related.
,
Jan 16 2018
The test-case has an issue, here's the new bug report: https://bugs.chromium.org/p/chromium/issues/detail?id=802228
,
Jan 16 2018
(Please feel free to close this report.)
,
Jan 16 2018
Bug we don't need a new bug report, the test case is not 100% right but it's useful. Here we've some info already so I'll keep this bug report. I'm attaching a reduced test case to show the problem. Basically "grid-template-rows: auto 1fr; grid-row-gap: 10px;" should behave the same than "grid-template-rows: auto 10px 1fr;".
,
Jan 16 2018
Issue 802228 has been merged into this issue.
,
Jan 16 2018
Don't worry, I know what I'm doing 😀 The original test-case is incorrect and thus it was confusing everyone (I also had questions about its assertion), and it was (as seen in the Twitter thread) diverting attention from the original issue. Thus I filed a fresh report without the incorrect and thus confusing test-case file. I found a Grid issue in my (work-in-progress) demo page. I hope that this issue will get fixed, in all browsers where it occurs. No matter whether a reduced test-case is used or not (as long as it's correct), and no matter which report URL you use 😀 Off-topic for this page, but: Is Opera affected as well? Is Edge OK?
,
Jan 16 2018
Opera should be affected as they use Chromium/Blink (as any other browser using Chromium underneath). But once it's fixed her it'll fixed for Opera and other too. It seems Edge is ok, but I'm not 100% convinced about the bug. I still need time to debug it properly and understand the issue.
,
Jan 16 2018
"I'm not 100% convinced about the bug" I hope we agree that (on my page, at ~900px viewport width, in Chrome etc) there's superfluous space at the bottom of the spanning Grid item "main" / that the spanning Grid item "main" is too high. And that it's not caused by margin or padding. And that the issue thus seems Grid-related. Right? "I still need time to debug it properly and understand the issue." Thank you for investigating!
,
Jan 16 2018
The bug reports for some of the other browsers: https://bugzilla.mozilla.org/show_bug.cgi?id=1430757 https://bugs.webkit.org/show_bug.cgi?id=181677
,
Jan 16 2018
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.
,
Jan 16 2018
Sounds good! Thanks! I assume it makes both render correctly: 1) https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=320224 and also 2) https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/ (the layout that appears when the page is viewed at ~900px viewport width)
,
Jan 16 2018
@tobi, yeah it'd fix both things. BTW, notice that the issue is also present in the inline (columns) direction: http://jsbin.com/fidewij/1/edit?html,css,output Again it works fine in Edge but wrong in the rest of browsers.
,
Jan 16 2018
"yeah it'd fix both things" That's good to hear 😁 I can't confirm that the assertion in that latest test-case is true in Edge / they're different in Edge: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10304317/snapshots/z2798e93ba6dfd21a8f2 My version: http://jsbin.com/kuzuqadaji/edit?html,css,output The two boxes are different in Chrome: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10304406/snapshots/z59fdc0a1c8715992e24 ... and get rendered correctly (both boxes look the same) in Edge: https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10304376/snapshots/z0bb0da46b9d871c9501
,
Jan 16 2018
(my version, updated: http://jsbin.com/dusetovifa/1/edit?html,css,output ) (different size in Chrome:) https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10304481/snapshots/z582d3eb69c8beec4d08 (same size = correct in Edge:) https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10304476/snapshots/z77180b7e6678cd0161c
,
Jan 17 2018
You need to install Ahem font in my example to make them look the same in Edge: https://www.w3.org/Style/CSS/Test/Fonts/Ahem/
,
Jan 17 2018
I don't have Windows - for testing on Windows I'm using https://crossbrowsertesting.com/ . It'd be impractical to install the font each time 😁 I could install it in a VM, but IMHO in general it's better when test-cases don't require the tester to have specific fonts installed. (except perhaps when the test-case is part of a test-suite with instructions for testers) Or we could @font-face the Ahem font in the test-case. In any case: Does my version make sense as is? http://jsbin.com/dusetovifa/1/edit?html,css,output (when the specified Ahem is loaded, and also when it's not loaded / a fallback/default font is loaded) I had linked it at https://bugzilla.mozilla.org/show_bug.cgi?id=1430757 and https://bugs.webkit.org/show_bug.cgi?id=181677 . Please let me know in case it's incorrect as test-case for this reported issue.
,
Jan 17 2018
(Oh, I meant as additional test-case, not as THE ONE 😁)
,
Jan 17 2018
Yes your version is fine, anyway don't worry too much about the test. Once this is fixed, it'll include a WPT test that we could share with other browser vendors (using Ahem font is very common on those cases). About WebKit I'll take care of porting the patch, once it lands here so no worries either. Just give us some time for the patch to get reviewed and landed, probably that would happen in the coming days.
,
Jan 17 2018
"About WebKit I'll take care of porting the patch, once it lands here so no worries either." I'm not worrying 😁 Good to hear that you're going to port the patch. As soon as it works in WebKit you (or someone else) can close https://bugs.webkit.org/show_bug.cgi?id=181677 .
,
Jan 18 2018
Gecko has the exact same bug. I tend to think we both implement what the spec actually says correctly, and that the bug is in the spec. Rego, will you file a spec bug to make the text clearer?
,
Jan 18 2018
I'm not sure if it's a spec bug. The text in (https://drafts.csswg.org/css-grid/#algo-find-fr-size) is pretty clear: > 1. Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks. We were not removing the size of the gutters (when the grid container has a content based size), but it seems that we should: > Note: Gutters are treated as empty fixed-size tracks for the purpose of the track sizing algorithm. Maybe this shouldn't be a note though... but yeah I can open an issue to discuss this.
,
Jan 18 2018
Opened the issue on CSSWG Github for further discussion: https://github.com/w3c/csswg-drafts/issues/2201
,
Jan 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea343b082054dcdb48e3a2c10abb62b8c52257d6 commit ea343b082054dcdb48e3a2c10abb62b8c52257d6 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Fri Jan 19 12:33:56 2018 [css-grid] Fix for flexible tracks, gutters and spanning items In IndefiniteSizeStrategy::FindUsedFlexFraction() we were not subtracting the size of the gutters when we call FindFrUnitSize(). If an item spans several tracks, we cannot pass the MaxContentForChild() directly, we need to subtract the gutters as they are treated as fixed size tracks in the algorithm. The spec text is pretty clear regarding this (https://drafts.csswg.org/css-grid/#algo-find-fr-size): "Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks." Gutters are treated as fixed-size tracks for the purpose of the track sizing algorithm, so we need to subtract them from the leftover space while finding the size of an "fr". BUG= 802021 TEST=external/wpt/css/css-grid/layout-algorithm/grid-find-fr-size-gutters-001.html Change-Id: If066852826a0ba0072a9fd996f11c0d653610b97 Reviewed-on: https://chromium-review.googlesource.com/868659 Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com> Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Reviewed-by: Sergio Villar <svillar@igalia.com> Cr-Commit-Position: refs/heads/master@{#530491} [add] https://crrev.com/ea343b082054dcdb48e3a2c10abb62b8c52257d6/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/layout-algorithm/grid-find-fr-size-gutters-001.html [add] https://crrev.com/ea343b082054dcdb48e3a2c10abb62b8c52257d6/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/layout-algorithm/grid-find-fr-size-gutters-002.html [modify] https://crrev.com/ea343b082054dcdb48e3a2c10abb62b8c52257d6/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp
,
Jan 19 2018
This should be fixed now.
,
Jan 19 2018
Thank you!
,
Jan 20 2018
I can confirm that it's fixed: I checked the test-case and the page, in Canary. Thanks all!
,
Mar 19 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by t...@tobireif.com
, Jan 15 2018147 KB
147 KB View Download