Closed
Bug 1302541
Opened 8 years ago
Closed 8 years ago
[css-grid] Wrong intrinsic grid container size with percentage grid gaps
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: testcase)
Attachments
(6 files)
3.08 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.51 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.54 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.10 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.69 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
45.57 KB,
patch
|
Details | Diff | Splinter Review |
I think percentage grid gaps should contribute to the grid container's intrinsic size just like percentages normally do, i.e. using the equation: Size = SumOfCoordSizes / (1 - SumOfPercentages). Here's a testcase: https://people.mozilla.org/~mpalmgren/tests/grid/percent-intrinsic-sizing.html (last two boxes are grids) and here's the suggested rendering: https://people.mozilla.org/~mpalmgren/tests/grid/percent-intrinsic-sizing.png The CSS Grid spec needs to be amended to allow that though: https://github.com/w3c/csswg-drafts/issues/472
Assignee | ||
Comment 1•8 years ago
|
||
Tentatively marking this as blocking release; at least until the spec issue is sorted out.
Blocks: 1217086
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mats
Assignee | ||
Comment 2•8 years ago
|
||
The CSSWG was kind enough to "leave open" how percentages work under intrinsic sizing, explicitly mentioning "back-computing" is allowed, with the requirement that grid-gap works the same as empty tracks: https://github.com/w3c/csswg-drafts/issues/472#issuecomment-248655700
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8799916 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8799917 -
Flags: review?(dholbert)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8799918 -
Flags: review?(dholbert)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8799919 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8799920 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8648102e4fbe239040d69f426f056ff477ca2b5a&exclusion_profile=false
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8799918 [details] [diff] [review] part 3 - [css-grid] Back-compute percentages for the intrinsic inline size. > + size += std::max(length, trackSize); FYI, I believe this line could be simplified to "size += trackSize" if we make a small change to how we initialize tracks with a min-sizing calc() size. When the percentage base is indefinite, we should set mBase to the calc() length part instead of zero (but continue to treat the track as "auto" as before). I'll investigate and do this in a follow-up bug.
Updated•8 years ago
|
Attachment #8799916 -
Flags: review?(dholbert) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8799917 [details] [diff] [review] part 2 - [css-grid] Propagate track state bits and store the union of all tracks in each axis. Review of attachment 8799917 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.cpp @@ +1811,5 @@ > nscoord mGridGap; > // The first(last)-baseline for the first(last) track in this axis. > nscoord mBaseline[2]; // index by BaselineSharingGroup > + // The union of the track min/max-sizing state bits in this axis. > + TrackSize::StateBits mState; Maybe name this mStateUnion*, so that it's harder to mix up this member-var (in Tracks) with the identically-named member-var in TrackSize? (And also since this particular variable isn't meant to track a single "state" -- as its comment says, it's a union of multiple states.) * (or mUnitedStates? :D )
Attachment #8799917 -
Flags: review?(dholbert) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8799918 [details] [diff] [review] part 3 - [css-grid] Back-compute percentages for the intrinsic inline size. Review of attachment 8799918 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with the following minor concern addressed/explained: ::: layout/generic/nsGridContainerFrame.cpp @@ +4873,5 @@ > + } > + size += gridGapCount * gridGapLength; > + } > + > + return std::max(0, nsLayoutUtils::AddPercents(size, percent)); Why the "std::max" here? |size| should already be nonnegative (since it's the sum of nonnegative things), and AddPercents promises to return a strictly larger value than it was given. (And I don't think we need to bother concerning ourselves with integer overflow, except to be sure it doesn't cause hangs/crashes.)
Attachment #8799918 -
Flags: review?(dholbert) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8799919 [details] [diff] [review] part 4 - [css-grid] Back-compute percentages for the intrinsic block size. Review of attachment 8799919 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below: ::: layout/generic/nsGridContainerFrame.cpp @@ +2003,5 @@ > } > > /** > + * Calculate our track sizes. If the given aContentBox block-axis size is > + * unconstrained it is assigned to the resulting intrinsic size. Two nits: 1) add comma after "unconstrained", for readability. 2) s/intrinsic size/intrinsic block-axis size/, for clarity about what flavor of "intrinsic size" we're copying from here.
Attachment #8799919 -
Flags: review?(dholbert) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8799920 [details] [diff] [review] part 5 - [css-grid] Back-compute percentages when calculating the number of auto-fill/fit tracks. Review of attachment 8799920 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: layout/generic/nsGridContainerFrame.cpp @@ +1131,4 @@ > // Note that the repeat() track size is included in |sum| in this loop. > nscoord sum = 0; > + float percentSum = 0.0f; > + nscoord percentBasis = aSize; Can we declare |percentBasis| as "const" here? Looks like it never gets modified.
Attachment #8799920 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #12) > Why the "std::max" here? It can be negative since we sum coord sizes separately from percentages, e.g. grid: 0 0 / 0; grid-gap: calc(10% - 1px) leads to |size| = -1px
Assignee | ||
Comment 16•8 years ago
|
||
(other nits addressed as suggested)
Comment 17•8 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d7ffc24c84 part 1 - [css-grid] Add a track state bit for percentage min-sizing that is treated as 'auto'. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/c761406e4c14 part 2 - [css-grid] Propagate track state bits and store the union of all tracks in each axis. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/948ea4b490cf part 3 - [css-grid] Back-compute percentages for the intrinsic inline size. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/0adfd4435d28 part 4 - [css-grid] Back-compute percentages for the intrinsic block size. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/50173aed1530 part 5 - [css-grid] Back-compute percentages when calculating the number of auto-fill/fit tracks. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/aff267eaa3b4 part 6 - [css-grid] Add more reftests for percentage track sizes and grid-gaps.
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1d7ffc24c84 https://hg.mozilla.org/mozilla-central/rev/c761406e4c14 https://hg.mozilla.org/mozilla-central/rev/948ea4b490cf https://hg.mozilla.org/mozilla-central/rev/0adfd4435d28 https://hg.mozilla.org/mozilla-central/rev/50173aed1530 https://hg.mozilla.org/mozilla-central/rev/aff267eaa3b4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 19•7 years ago
|
||
https://github.com/w3c/csswg-drafts/issues/509
You need to log in
before you can comment on or make changes to this bug.
Description
•