Closed Bug 1163435 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox45 --- fixed
firefox62 --- affected

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(3 files, 4 obsolete files)

testcase
564 bytes, text/html
Details
fix
7.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
tests
12.43 KB, patch
Details | Diff | Splinter Review
Attached file testcase —
The expected size for the green box is 75x50 pixels.
Attached patch wip (obsolete) — — Splinter Review
Attached patch fix (obsolete) — — Splinter Review
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.)
Blocks: 1000592
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
Attached patch fix — — Splinter Review
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)
Attached patch tests — — Splinter Review
(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
Blocks: 1176775
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).
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.)
(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.)
https://hg.mozilla.org/mozilla-central/rev/9dbec0f99fd0 https://hg.mozilla.org/mozilla-central/rev/a8b188bf416f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Sorry, but I'm not familiar with your release process, when could I expect it to be released to Stable Firefox?
Blocks: css-grid-1
The width of outer grid container is calculated wrong: https://codepen.io/KES777/pen/KxXdgV it have less width than summ of all columns
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: