Closed Bug 1430757 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect)

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tobi, Assigned: mats)

References

Details

(Keywords: testcase)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

Open (the work-in-progress demo)
https://tobireif.com/demos/grid/ , 
set the viewport width to approximately 900px.
Scroll down. Right-click on the content -> "Inspect element". Select the parent with class "page". For that element turn on the Grid highlighter. 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") / that it is too high, 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/ .




Actual results:

There is superfluous space at the bottom of the spanning Grid item "main" /  the spanning Grid item "main" is too high.


Expected results:

The should be no superfluous space at the bottom of the spanning Grid item "main".
Attached image screenshot_firefox.png
Duplicate of this bug: 1430580
Reported the issue for some of the other browsers as well:

https://bugs.chromium.org/p/chromium/issues/detail?id=802021

https://bugs.webkit.org/show_bug.cgi?id=181677
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
Component: Untriaged → Layout
Product: Firefox → Core
(In reply to Tobi Reif from comment #6)
> 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."

That's a bug in the spec.  It really should point out that the grid gaps
between the tracks spanned by the item should also be subtracted.
Firefox has the exact same bug as Chrome and that's because we both
implement what the spec actually says...
Assignee: nobody → mats
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Spanning Grid item has too much space at the bottom / is too high → [css-grid] Spanning Grid item has too much space at the bottom / is too high
Thanks for checking out my report!

From my point of view it's obvious that the spanning item should not have that additional gap-wide space at its end. It spans cells with a gap between each cell, and then it should end :)

But if will get fixed/clarified/pointed out in the spec then that's great!

"It really should point out that the grid gaps between the tracks spanned by the item should also be subtracted."
Perhaps that text could get submitted at https://github.com/w3c/csswg-drafts/labels/css-grid-1 ?

At https://bugs.chromium.org/p/chromium/issues/detail?id=802021 , Manuel Rego cc'd (you and) Tab Atkins, I guess that there will be some decision there regarding a possible change in the spec.

I hope that the issue that I see on my page
https://tobireif.com/non_site_stuff/unfinished_grid_demo_copy_for_grid_span_bug_reports/
(when viewed at eg 900px)
and the (same) issue shown by the test-cases can get fixed in Firefox (etc), no matter whether the spec will be changed or not :)
The spec could be clearer it seems:

https://github.com/w3c/csswg-drafts/issues/2201
Attached patch fixSplinter Review
Thank you!
Attachment #8943685 - Flags: review?(dholbert)
Comment on attachment 8943685 [details] [diff] [review]
fix

Review of attachment 8943685 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +4535,5 @@
> +      const LineRange& range =
> +        mAxis == eLogicalAxisInline ? item.mArea.mCols : item.mArea.mRows;
> +      const auto span = range.Extent();
> +      if (span > 1) {
> +        spaceToFill -= mGridGap * (span - 1);

IMO this would be easier to understand if rephrased as:

  const auto spannedGaps = range.Extent() - 1;
  if (spannedGaps > 0) {
    spaceToFill -= mGridGap * spannedGaps;
  }

But what you've got is fine too -- r=me regardless.
Attachment #8943685 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f688ac39fb7
Subtract the size of the grid gaps the item spans from "space to fill" when calculating the "Find the Size of an 'fr'" for a grid item.  r=dholbert
(In reply to Daniel Holbert [:dholbert] from comment #15)
> IMO this would be easier to understand if rephrased as:
> 
>   const auto spannedGaps = range.Extent() - 1;

Sure, I added a "MOZ_ASSERT(range.Extent() >= 1)" before that line too, for clarity.
Flags: in-testsuite+
Thanks! Yeah, as I wrote my suggestion, I was slightly worried about the edge case of 0-extent-ranges, but was hoping those wouldn't get to this point in the code. :D
https://hg.mozilla.org/mozilla-central/rev/9f688ac39fb7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I just checked - it now works in Nightly! Thanks all!
Attached image screenshot_nightly.png
It works - there's no superfluous space at the bottom of the spanning grid item.
Also works regarding the test-case.
You need to log in before you can comment on or make changes to this bug.