[css-grid] Implement Automatic Minimum Size clamping for grid items with an intrinsic size but without an intrinsic ratio

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

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

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
... for example <iframe>
Assignee

Comment 3

3 years ago
(Sorry for the whackamole game with fixing these stretch/clamp bugs.  I think I'm done now.  :-) )
This logic is getting a bit opaque :-/  Could you add a code-comment to explain what we're doing here?

In particular -- the only comment in this area of code is "apply 'stretch' as needed".  But I don't think that's always accurate, in the now-generalized version of this code after the patch here.
Flags: needinfo?(mats)
Assignee

Comment 5

3 years ago
(updated the code comments)
Attachment #8808699 - Attachment is obsolete: true
Attachment #8808699 - Flags: review?(dholbert)
Flags: needinfo?(mats)
Attachment #8808831 - Flags: review?(dholbert)
Comment on attachment 8808831 [details] [diff] [review]
Implement Automatic Minimum Size clamping for grid items with an intrinsic size but without an intrinsic ratio.

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

::: layout/generic/nsFrame.cpp
@@ +4728,5 @@
> +      stretch = inlineAxisAlignment == NS_STYLE_ALIGN_NORMAL ||
> +                inlineAxisAlignment == NS_STYLE_ALIGN_STRETCH;
> +    }
> +    if (stretch || (aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize)) {
> +      auto iSize = std::max(nscoord(0), aCBSize.ISize(aWM) -

Maybe name this "iSizeToFillCB", to be clearer about what it aims to represent?

That makes the conditional assignment that follows this a little more human-readable, too.
Attachment #8808831 - Flags: review?(dholbert) → review+
Comment on attachment 8808831 [details] [diff] [review]
Implement Automatic Minimum Size clamping for grid items with an intrinsic size but without an intrinsic ratio.

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

::: layout/generic/nsFrame.cpp
@@ +4815,5 @@
> +          stretch = blockAxisAlignment == NS_STYLE_ALIGN_NORMAL ||
> +                    blockAxisAlignment == NS_STYLE_ALIGN_STRETCH;
> +        }
> +        if (stretch || (aFlags & ComputeSizeFlags::eBClampMarginBoxMinSize)) {
> +          auto bSize = std::max(nscoord(0), cbSize -

(and similarly "bSizeToFillCB" or somesuch)

Comment 8

3 years ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f250ead28c69
[css-grid] Implement Automatic Minimum Size clamping for grid items with an intrinsic size but without an intrinsic ratio.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/410135b3dd56
[css-grid] Reftests for Automatic Minimum Size clamping for grid items with an intrinsic size but without an intrinsic ratio.
Assignee

Updated

3 years ago
Flags: in-testsuite+

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f250ead28c69
https://hg.mozilla.org/mozilla-central/rev/410135b3dd56
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.