New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 802021 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

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

Reported by t...@tobireif.com, Jan 15 2018

Issue description

UserAgent: 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!
 
testcase_by_fantasai.html
1.5 KB View Download

Comment 1 by t...@tobireif.com, Jan 15 2018

screenshot_chrome.png
147 KB View Download

Comment 3 by t...@tobireif.com, 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".

Comment 4 Deleted

Comment 5 by r...@igalia.com, Jan 15 2018

Cc: svil...@igalia.com jfernan...@igalia.com r...@igalia.com
Components: -Blink>Layout Blink>Layout>Grid
Status: Untriaged (was: Unconfirmed)
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).

Comment 6 by r...@igalia.com, Jan 15 2018

Summary: [css-grid] Spanning Grid item has too much space at the bottom / is too high (was: Spanning Grid item has too much space at the bottom / is too high)

Comment 7 by t...@tobireif.com, 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.

Comment 8 by t...@tobireif.com, 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

Comment 9 by t...@tobireif.com, Jan 16 2018

(Please feel free to close this report.)

Comment 10 by r...@igalia.com, Jan 16 2018

Status: Available (was: Untriaged)
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;".
bug-802021.html
882 bytes View Download

Comment 11 by r...@igalia.com, Jan 16 2018

 Issue 802228  has been merged into this issue.

Comment 12 by t...@tobireif.com, 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?

Comment 13 by r...@igalia.com, 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.

Comment 14 by t...@tobireif.com, 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!

Comment 16 by r...@igalia.com, Jan 16 2018

Owner: r...@igalia.com
Status: Started (was: Available)
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 17 by t...@tobireif.com, 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)

Comment 18 by r...@igalia.com, 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.

Comment 19 by t...@tobireif.com, 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

Comment 21 by r...@igalia.com, 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/

Comment 22 by t...@tobireif.com, 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.

Comment 23 by t...@tobireif.com, Jan 17 2018

(Oh, I meant as additional test-case, not as THE ONE 😁)

Comment 24 by r...@igalia.com, 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.

Comment 25 by t...@tobireif.com, 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 .

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?

Comment 27 by r...@igalia.com, Jan 18 2018

Cc: mpalmg...@mozilla.com tabatkins@chromium.org
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.

Comment 28 by r...@igalia.com, Jan 18 2018

Opened the issue on CSSWG Github for further discussion:
https://github.com/w3c/csswg-drafts/issues/2201
Project Member

Comment 29 by bugdroid1@chromium.org, 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

Comment 30 by r...@igalia.com, Jan 19 2018

Status: Fixed (was: Started)
This should be fixed now.

Comment 31 by t...@tobireif.com, Jan 19 2018

Thank you!

Comment 32 by t...@tobireif.com, Jan 20 2018

I can confirm that it's fixed: I checked the test-case and the page, in Canary.

Thanks all!

Comment 33 by e...@chromium.org, Mar 19 2018

Cc: e...@chromium.org pbomm...@chromium.org
 Issue 822645  has been merged into this issue.

Sign in to add a comment