1315857 - [css-grid] Stretch/clamp intrinsic ratio grid items that has a definite size in one axis

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Stretch/clamp intrinsic ratio grid items that has a definite size in one axis.

Comment 2

[Mats Palmgren (inactive)]

I forgot to say that I'm intentionally ignoring clamping in the final else-branch
in this patch for now, because I couldn't figure out if 'hasIntrinsicI/BSize' is ever false.

Perhaps we could refactor this code to set a 150/300px intrinsic size up front instead:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/base/nsLayoutUtils.cpp#5592

Comment 3

[Mats Palmgren (inactive)]

Attachment: reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c15cf010f79de129fad1b84ce9285bb1838e0cc

Comment 4

[Mats Palmgren (inactive)]

Fwiw, we have zero reftests+crashtests that exercise the "hasIntrinsicI/BSize = false" path.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f3c251f54ca2192914e23c2ab83d3a48c876a18&exclusion_profile=false

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8808452 [details] [diff] [review]
Stretch/clamp intrinsic ratio grid items that has a definite size in one axis.

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

Commit message nit:
> Bug 1315857 - [css-grid] Stretch/clamp intrinsic ratio grid items that has a definite size in one axis.  r=dholbert

s/has/have/  ("items...that have")

::: layout/generic/nsFrame.cpp
@@ +5250,5 @@
> +        } else if (hasIntrinsicISize) {
> +          if (!((aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize) &&
> +                intrinsicISize > iSize)) {
> +            iSize = intrinsicISize;
> +          } // else iSize is the CB iSize

Perhaps tweak this to clarify what you're expressing. (I think you're intending to say (a) what iSize currently contains, (b) that this value is correct for our circumstances & doesn't need updating. Right now, this dual-meaning is sort of implied but isn't entirely clear.)

Maybe apply this tweak:
  s/iSize is/iSize should be (& is already)/

(If you do make this tweak, note that this patch has 4 instances of this comment which all would want the same tweak.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b67107f5aa4
[css-grid] Stretch/clamp intrinsic ratio grid items that have a definite size in one axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/db459d693928
[css-grid] Reftests for stretching/clamping intrinsic ratio grid items that has a definite size in one axis.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b67107f5aa4
https://hg.mozilla.org/mozilla-central/rev/db459d693928