Closed Bug 1316029 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

Attachments

(2 files, 1 obsolete file)

... for example <iframe>
(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)
(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)
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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f250ead28c69
https://hg.mozilla.org/mozilla-central/rev/410135b3dd56
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.