Closed
Bug 1218178
Opened 8 years ago
Closed 7 years ago
[css-grid] Implement the "transferred size" piece for "Implied Minimum Size of Grid Items"
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Depends on 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
7.04 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
93.64 KB,
patch
|
Details | Diff | Splinter Review | |
6.34 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
Follow-up from bug 1176775
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Just to double check I understood the spec(s) correctly, does this look right to you?
Flags: needinfo?(dholbert)
Comment 3•8 years ago
|
||
I think the grid-item sizes there all look correct (though the rules of influence here can be deceptively complex, so I might be missing something). (BTW, I focused on the grid *item* sizes, and didn't really think about the grid containers' sizes in the testcase-screenshot. I recall less about how grid container intrinsic-sizing works, and I'm not sure that's what you're focusing on here. Let me know if you think that needs some sanity-checking as well, though.)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 4•8 years ago
|
||
Sorry, I should have been more clear. The top part tests the resulting column size when there's a single item (image) with 'min-width:auto' and the other styles you see in the test. The width of the grid container (black border) wraps that column's size, so the width of that box is what's interesting. (You can ignore the vertical dimension here.) The bottom part tests the resulting row height when there's an image with 'min-height:auto' and the other styles you see in the test. It's the height of the container that's interesting here. (You can ignore the horizontal dimension here.)
Comment 5•8 years ago
|
||
RE the width of the container being "interesting" for the top part -- by that, you mean that the container's width indicates what we think the grid item's "min-width" is, correct? (I initially glossed over the "grid-template-columns:minmax(auto,0)", having forgotten the exact meaning of the syntax, but on re-examination I think that this is what you're trying to get at with that CSS?) (and similar for the bottom part)
Assignee | ||
Comment 6•8 years ago
|
||
Exactly. It's the item's "Implied Minimum Size" per 4.4 that decides the column width (top) and row height (bottom).
Comment 7•8 years ago
|
||
Great, thanks. There are two more values that I think you should be testing here, in "coltest" (the first part) (and probably similar in the second part, too) (1) "min-height: 4px" (2) "min-height: 4px; max-width: 30px" As described in the "content size" definition on section 4.4, the "min-height" value should be converted through the aspect-ratio (producing 48px) & should provide a lower bound for the "content size". So, I'd expect case (1) to produce a grid that is 48px wide. The content size is *further* clamped by the "max" value in the same dimension (if definite), and case (2) should test that we're doing that -- I'd expect that one to produce a 30px-wide grid.
Comment 8•8 years ago
|
||
(I guess comment 7 is technically not about the "transferred size" -- it's about a similar piece of the "content size". If you'd like to handle that piece in a followup, feel free; but it might make sense to lump it in here.)
Comment 9•8 years ago
|
||
Renderings still look good to me, looking w/ fresh eyes in light of comment 5 / comment 6. (One possible test-bug that I noticed: the very last example in this testcase has "width:1px; min-height:30px" on its grid-item -- notice that it's explicitly setting "min-height". It's in a section that's ostensibly testing "min-height:auto" resolution, but it does not have "min-height:auto". Seems like that might be a mistake, but maybe you included it for completeness or as a bonus or something.)
Assignee | ||
Comment 10•7 years ago
|
||
It looks like we need to implement two separate behaviors here: stretch-with-preserved-ratio for 'normal' (the default), and and stretch-without-preserved-ratio for 'stretch'. https://github.com/w3c/csswg-drafts/issues/523 The CSSWG resolution says: Stretching image grid items in both dimensions ---------------------------------------------- - RESOLVED: The normal value of align-self and justify-self preserves aspect of image but stretch causes it to stretch to fit containing block. Default value continues to be normal. https://lists.w3.org/Archives/Public/www-style/2016Oct/0068.html
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8679407 -
Attachment is obsolete: true
Attachment #8679409 -
Attachment is obsolete: true
Attachment #8803655 -
Flags: review?(dholbert)
Assignee | ||
Comment 12•7 years ago
|
||
I originally had some changes in this method which I later reversed. I did some code cleanup though which I think is worth keeping. Regarding the removed "If we're a flex item..." comment: I think it's not very informative and it should be fairly obvious from the isGridItem/isFlexItem bools, and their use, that we're doing things "a bit differently" for those.
Attachment #8803656 -
Flags: review?(dholbert)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8803657 -
Flags: review?(dholbert)
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f3ffbd9fa09&exclusion_profile=false (the linux64 orange is due to CI changes and looks unrelated to my changes)
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8803657 [details] [diff] [review] part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio. FYI, I've fixed an indentation issue in this patch locally: stretchI = inlineAxisAlignment == NS_STYLE_ALIGN_NORMAL || - inlineAxisAlignment == NS_STYLE_ALIGN_STRETCH; + inlineAxisAlignment == NS_STYLE_ALIGN_STRETCH; stretchB = blockAxisAlignment == NS_STYLE_ALIGN_NORMAL || - blockAxisAlignment == NS_STYLE_ALIGN_STRETCH; + blockAxisAlignment == NS_STYLE_ALIGN_STRETCH;
Assignee | ||
Comment 17•7 years ago
|
||
(Fixing an edge case bug I discovered while writing some reftests for <canvas> grid items.)
Attachment #8803657 -
Attachment is obsolete: true
Attachment #8803657 -
Flags: review?(dholbert)
Attachment #8806213 -
Flags: review?(dholbert)
Comment 18•7 years ago
|
||
Comment on attachment 8803655 [details] [diff] [review] part 1 - [css-grid] Implement the transferred size part of Automatic Minimum Size for grid items. Review of attachment 8803655 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +5053,5 @@ > nscoord bSizeTakenByBoxSizing = > GetBSizeTakenByBoxSizing(boxSizing, aFrame, horizontalAxis, > aFlags & IGNORE_PADDING); > > + nscoord minContentSize = result; Observation: this assignment/naming is a bit mysterious -- i.e. it's not clear why "result" actually contains the min-content size here. (It looks like it probably *is* the min-content size, but only because |result| hasn't been further-refined based on other constraints yet.) Perhaps it'd be better to rewrite the code above this point to use "minContentSize" instead of "result", and then declare "nscoord result = minContentSize;" somewhere around here? (That could happen in a followup patch/bug.) @@ +5088,5 @@ > + if (MOZ_UNLIKELY(aFlags & nsLayoutUtils::MIN_INTRINSIC_ISIZE)) { > + // This is the "transferred size" piece of: > + // https://www.w3.org/TR/css-flexbox-1/#min-width-automatic-minimum-size > + // https://drafts.csswg.org/css-grid/#min-size-auto > + result = std::min(result, minContentSize); Please add something like this at the beginning of this comment: // This flag means we're resolving min-[width|height]: auto. (Is that strictly true? Hopefully it is -- otherwise, this new code isn't quite right.) That makes the following references to "transferred size" a bit clearer. (I didn't initially recall that MIN_INTRINSIC_ISIZE was directly part of "min-{...}: auto" -- so this min-size-auto-specific transferred-size code initially seemed out of place to me.)
Attachment #8803655 -
Flags: review?(dholbert) → review+
Comment 19•7 years ago
|
||
Comment on attachment 8803656 [details] [diff] [review] part 2 - Minor code cleanup (idempotent). Review of attachment 8803656 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8803656 -
Flags: review?(dholbert) → review+
Comment 20•7 years ago
|
||
Comment on attachment 8806213 [details] [diff] [review] part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio. Review of attachment 8806213 [details] [diff] [review]: ----------------------------------------------------------------- r=me, though this might not be quite right yet. I'll let you decide whether to address the "normal" thing (my second point below) here or in a followup. ::: layout/base/nsLayoutUtils.cpp @@ +5528,5 @@ > + !aFrame->StyleMargin()->HasInlineAxisAuto(aWM)) { > + auto inlineAxisAlignment = > + aWM.IsOrthogonalTo(aFrame->GetParent()->GetWritingMode()) ? > + stylePos->UsedAlignSelf(aFrame->StyleContext()->GetParent()) : > + stylePos->UsedJustifySelf(aFrame->StyleContext()->GetParent()); I'm pretty sure you actually want aFrame->GetParent()->StyleContext() here. (You want the parent frame's style context -- not the style context's parent.) Otherwise you'll run into issues like the one I fixed in bug 1306213. If aFrame is a nsTableOuterFrame (for a table inside of a grid), then aFrame->GetParent()->StyleContext() would give you the nsTableFrame's style context! And that's not what you want here (for resolving "align-self:auto"). You want to be using the grid's style context (the parent frame's style context). @@ +5690,5 @@ > } > > if (aIntrinsicRatio != nsSize(0, 0)) { > + if (stretchI || stretchB) { > + // Stretch within the CB size with preserved intrinsic ratio. Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values? Right now, it looks like we'll enter this code even if we have "align-self:stretch; justify-self: stretch". And I don't think we're supposed to... This is supposed to be a special thing that only happens for the "normal" value, IIUC (and stretch is allowed to deform away from the aspect ratio).
Attachment #8806213 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #18) > > + nscoord minContentSize = result; > > Observation: this assignment/naming is a bit mysterious -- i.e. it's not > clear why "result" actually contains the min-content size here. (It looks > like it probably *is* the min-content size, but only because |result| hasn't > been further-refined based on other constraints yet.) Well, it is the min-content size at that point for the MIN_INTRINSIC_ISIZE case, which is the only use. > Perhaps it'd be better to rewrite the code above this point to use > "minContentSize" instead of "result", and then declare "nscoord result = > minContentSize;" somewhere around here? (That could happen in a followup > patch/bug.) So this is probably not a good idea. I don't know what else to call it though, let me know if you have a suggestion. > > + if (MOZ_UNLIKELY(aFlags & nsLayoutUtils::MIN_INTRINSIC_ISIZE)) { > > + // This is the "transferred size" piece of: > > + // https://www.w3.org/TR/css-flexbox-1/#min-width-automatic-minimum-size > > + // https://drafts.csswg.org/css-grid/#min-size-auto > > Please add something like this at the beginning of this comment: > // This flag means we're resolving min-[width|height]: auto. > (Is that strictly true? Hopefully it is -- otherwise, this new code isn't > quite right.) We pass MIN_INTRINSIC_ISIZE also for min-width/height:min-content etc: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/generic/nsGridContainerFrame.cpp#3833 I'll investigate if this is a problem and file a follow-up bug if needed...
Assignee | ||
Comment 22•7 years ago
|
||
> I'm pretty sure you actually want aFrame->GetParent()->StyleContext() here. Good catch, I've fixed this locally, and also the same bug in the existing block-axis code! > Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values? Yeah, I think I misunderstood the CSSWG minutes the first time around but after reading it again I think you're right that's their intention (no actual spec text for this yet though). I'd prefer to fix this in a follow-up bug though, if you don't mind, because bug 1300369 stomps on this code a bit (for clamping) so I'd rather do the fix on top of that.
Comment 23•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #21) > Well, it is the min-content size at that point for the MIN_INTRINSIC_ISIZE > case, which is the only use. Yeah, but that only-use is much further down. So the declaration here (which lacks a MIN_INTRINSIC_ISIZE check) is still hand-wavy & mysterious. Two suggestions: (1) Perhaps add a comment above its decl saying something like: // NOTE: This is only the minContentSize if we've been passed MIN_INTRINSIC_ISIZE // (which is fine, because this should only be used inside a check for that flag). ...and/or: (2) You could make this validity more explicit by only giving this variable a value *inside* of a a dedicated 1-liner MIN_INTRINSIC_ISIZE check. Then, if we accidentally add bogus uses of this variable in the future (outside of a MIN_INTRINSIC_ISIZE check), the compiler and/or valgrind+ASAN will help us with uninitialized-usage warnings. But maybe that's overkill. (In reply to Mats Palmgren (:mats) from comment #22) > > Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values? > > Yeah [...] I'd prefer to fix this in a follow-up bug though, if you don't mind Sure, sounds good.
Comment 24•7 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/28dad79c4333 part 1 - [css-grid] Implement the transferred size part of Automatic Minimum Size for grid items. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/60d6fd3a6aa8 part 2 - Minor code cleanup (idempotent). r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9d3d21f20d part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/30c652ff1d1e part 4 - [css-grid][css-align] Add more reftests with image grid items.
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28dad79c4333 https://hg.mozilla.org/mozilla-central/rev/60d6fd3a6aa8 https://hg.mozilla.org/mozilla-central/rev/4e9d3d21f20d https://hg.mozilla.org/mozilla-central/rev/30c652ff1d1e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•