Closed Bug 1315857 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
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
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 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.)
Attachment #8808452 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/5b67107f5aa4
https://hg.mozilla.org/mozilla-central/rev/db459d693928
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.