[css-grid] percentage lengths for grid item isn't calculated correctly

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla45
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 affected, firefox45 fixed, firefox62 affected)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8603876 [details]
testcase

The expected size for the green box is 75x50 pixels.
(Assignee)

Comment 2

4 years ago
Created attachment 8604050 [details] [diff] [review]
fix
Attachment #8603877 - Attachment is obsolete: true
Attachment #8604050 - Flags: review?(dholbert)
Comment on attachment 8604050 [details] [diff] [review]
fix

>diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp
>--- a/layout/generic/nsGridContainerFrame.cpp
>+++ b/layout/generic/nsGridContainerFrame.cpp
[...]
>-    nsHTMLReflowState childRS(pc, aReflowState, child, cb.Size(wm));
>+    nsHTMLReflowState childRS(pc, aReflowState, child, cb.Size(wm),
>+                              cb.Width(wm), cb.Height(wm));

Just to make sure I understand -- the idea here is that grid items need to use their *grid area* as their containing block -- not the grid container -- right?  (And by default, nsHTMLReflowState assumes it should use the parent reflow-state's size as the containing block, and that's not what we want.)

If so: please update the documentation for the nsHTMLReflowState constructor that you're invoking -- quoting MXR:
> 618    * @param aContainingBlockWidth An optional width, in app units, that is used
> 619    *        by absolute positioning code to override default containing block
> 620    *        width.
> 621    * @param aContainingBlockHeight An optional height, in app units, that is
> 622    *        used by absolute positioning code to override default containing
> 623    *        block height.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h?rev=9f3b7f39745b#618

Right now, this documentation basically says these args are *only relevant for abspos code*. That's no longer the case, it seems.
Comment on attachment 8604050 [details] [diff] [review]
fix

Optional nit:
>Bug 1163435 part 1 - Propagate an explicit CB width/height to the reflow state to resolve percentage lengths for grid items properly.  r=dholbert

Maybe worth adding a second line (or just rewording) to clarify what "properly" actually means here. e.g. "Grid items are supposed to use their 'grid area' -- not their grid container -- as their containing block."

r=me with the nsHTMLReflowState documentation updated, anyway.
Attachment #8604050 - Flags: review?(dholbert) → review+
Comment on attachment 8604051 [details] [diff] [review]
tests

Drive-by nits on tests:

>+++ b/layout/reftests/css-grid/grid-item-sizing-percent-001-ref.html
>@@ -0,0 +1,96 @@
>+<!DOCTYPE html>
>+<!--
>+     Any copyright is dedicated to the Public Domain.
>+     http://creativecommons.org/publicdomain/zero/1.0/
>+-->
>+<html><head>
>+  <title>CSS Test: Testing grid item 'px' sizes</title>

IMO reference cases should have a different title from the testcase. (Otherwise it's harder to tell which is which when switching between them in tabs)

>+<!--
>+<div id="abs" style="float:left">
>+<div class="grid"><div id="t1" class="item"></div></div>
[...]
>+</div>
>+-->

What's up with this huge commented-out part in each of the test files?

If that's intended to be uncommented, pending some other bugfix, maybe add a comment referencing that bug number? Otherwise it seems like mysterious cruft that'll easily end up abandoned & forgotten.
(Also, the reference case has "percent" in its filename, but "px" in its <title> -- that seems like a typo.

Also, since this reference case is used for both a percent testcase and a px testcase, it seems a bit odd to have it declare one or the other in its filename. These might really want to be named something like:
  grid-item-sizing-001-pct.html
  grid-item-sizing-001-px.html
  grid-item-sizing-001-ref.html
or -001a, -001b, -001-ref. Maybe not worth worrying about, though.)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1217206
(Assignee)

Updated

3 years ago
Blocks: 1000592

Comment 9

3 years ago
Looks good. This is the behavior as resolved by the CSSWG at the May NYC f2f. It was re-opened for discussion at the Sept Paris f2f, left at an impasse, however it was widely acknowledged that implementer experience here is encouraged. 

Key thing is that we did get agreement between Mozilla, Microsoft, and explicit abstention from Apple[1] who had agreed with this behavior at the May NYC f2f, and thus we can reasonably move forward with that.

https://lists.w3.org/Archives/Public/www-style/2015Sep/0038.html
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1163565
(Assignee)

Comment 11

3 years ago
Created attachment 8678153 [details] [diff] [review]
fix

Note: this is with the fix from bug 1163565 folded in.

This patch makes grid/flex items use the size in the same axis as
the percentage basis.  Also for abs.pos. boxes with a grid/flex
container as their Absolute Containing Block.

(This change passed reftests, so it seems we don't have any Flexbox tests
 for the abs.pos. case that use percentage in the block axis?)

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Just to make sure I understand -- the idea here is that grid items need to
> use their *grid area* as their containing block -- not the grid container --
> right?

Yes.

> (And by default, nsHTMLReflowState assumes it should use the parent
> reflow-state's size as the containing block, and that's not what we want.)

I think the given aAvailableSpace is used(?)

> If so: please update the documentation for the nsHTMLReflowState constructor

Fixed.
Attachment #8604050 - Attachment is obsolete: true
Attachment #8678153 - Flags: review?(dholbert)
(Assignee)

Comment 12

3 years ago
Created attachment 8678155 [details] [diff] [review]
tests

(In reply to Daniel Holbert [:dholbert] from comment #6)
> What's up with this huge commented-out part in each of the test files?

It was to comment out the tests for bug 1163565 that uncommented
this block.  Not a problem now that the bugs have merged.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Also, since this reference case is used for both a percent testcase and a px
> testcase, it seems a bit odd to have it declare one or the other in its
> filename.

Meh.  I think it's fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf40a4f8221c
Attachment #8604051 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1176775
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
OK, I missed that you were talking about percent *margin and padding* w.r.t. changing the flexbox percentage basis here (as opposed to width/height, which is what this bug was originally about).

Marking my confused comment 13 as "obsolete" & thinking about this again, with that in mind.
I'm not clear why this & bug 1163565 were merged.  They're related, but I'd somewhat prefer that we landed this bug's original change separately from bug 1163565's changes, since what bug 1163565 covered was contentious (& seems like it could change in the future), as shown in the discussion that tantek linked to in comment 9.

Whereas, this bug's original (1-line) change is uncontroversial & orthogonal to that discussion.

Not a big enough deal for me to "r-", but if you agree, I'd suggest considering re-splitting the patches if not the bugs.
Comment on attachment 8678153 [details] [diff] [review]
fix

r=me, modulo comment 17 and the following nit:

>+++ b/layout/generic/nsGridContainerFrame.cpp
>@@ -2795,20 +2795,21 @@ nsGridContainerFrame::ReflowChildren(Gri
>     LogicalSize childCBSize = cb.Size(wm).ConvertTo(childWM, wm);
>     // XXX temporary workaround to avoid being INCOMPLETE until we have
>     // support for fragmentation (bug 1144096)
>+    LogicalSize percentBasis(childCBSize);
>     childCBSize.BSize(childWM) = NS_UNCONSTRAINEDSIZE;

Please move this new line up 2 lines -- insert it before the "XXX" comment.  (The XXX comment is referring to the next line -- the NS_UNCONSTRAINEDSIZE assignment -- and your new line splits those two things apart right now.)
Attachment #8678153 - Flags: review?(dholbert) → review+
Attachment #8678297 - Attachment is obsolete: true
Two more notes:
 (1) FWIW, this will make us violate (or at least go beyond the requirements of) the current flexbox spec ED, w.r.t. % margins/padding. It only calls for this behavior on "flex items" (in-flow children): https://drafts.csswg.org/css-flexbox-1/#item-margins  So until the spec is updated to call for this behavior on abspos children [not sure if/how-soon that'll happen], we'll be non-conforming on this point.  I'll defer to tantek's judgement that this is fine.

 (2) I expect there to be a small web-compat impact from the flexbox change, with bugs being filed about the behavior-change. (I'm basing this expectation on the handful of bugs that were filed after bug 851379 landed, which I've clustered as dupes of invalid-bug 958714).
(Assignee)

Comment 20

3 years ago
Right, this is a change for Flexbox abs.pos. items, which previously used the ISize
and now use BSize to resolve vertical percent.  (I think that means we don't have
any tests for that because I haven't noticed any failures from this change.)

Comment 21

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #19)
> Two more notes:
>  (1) FWIW, this will make us violate (or at least go beyond the requirements
> of) the current flexbox spec ED, w.r.t. % margins/padding. It only calls for
> this behavior on "flex items" (in-flow children):

I don't see how this is stating this for "only" in flow flex items. It just states flex items, not in-flow flex items nor does the section regarding abspos specify that they should resolve % margin/padding any differently.
(In reply to Greg Whitworth from comment #21)
> > this behavior on "flex items" (in-flow children):
> 
> I don't see how this is stating this for "only" in flow flex items. It just
> states flex items, not in-flow flex items

Flex items are *defined* as being the in-flow children of a flex container.  See first two sentences of https://drafts.csswg.org/css-flexbox-1/#flex-items

(The spec is careful to refer to abspos children as "absolutely-positioned child of a flex container" or "Absolutely-Positioned Flex Children", and avoids calling them "flex items", since they don't flex.)

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9dbec0f99fd0
https://hg.mozilla.org/mozilla-central/rev/a8b188bf416f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 27

3 years ago
Sorry, but I'm not familiar with your release process, when could I expect it to be released to Stable Firefox?

Updated

2 years ago
Blocks: 1312610

Comment 28

2 months ago
The width of outer grid container is calculated wrong: https://codepen.io/KES777/pen/KxXdgV

it have less width than summ of all columns

Updated

2 months ago
status-firefox62: --- → affected
You need to log in before you can comment on or make changes to this bug.