If you have one, please attach a proposed -ref.html for attachment 8763367 [details]. I'll work one up based on my understanding of correct behavior.

Thanks for the offer to help, but I have mostly complete patches to fix this bug.

I was about to fill a bug but then I found this :). I think there are a couple of cases in grid-repeat-auto-fill-fit-001.html that are wrong. In particular there are 2 float grids with a min-width:50px which are not respecting what the specs say. The current code creates a grid with 2 columns but I think it should have 3 because we have an indefinite size (and max size) but a definite min-size. In that case in order to fulfill the requirement we need at least 3 columns.

I'm specifically talking about these cases

Thanks Sergio, the upcoming patch will fix those cases and more.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f33db4430317&selectedJob=27339333 (the clip-path-*.html failures are an unrelated trunk issue)

Comment on attachment 8790692 [details] [diff] [review] fix Review of attachment 8790692 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.cpp @@ +5992,5 @@ > + auto GetDefiniteSize = [] (const nsStyleCoord& aCoord, > + nscoord aMin, > + nscoord* aResult) { > + MOZ_ASSERT(aMin >= 0, "aMin is supposed to be a size"); > + if (aCoord.IsCoordPercentCalcUnit() && !aCoord.HasPercent()) { I think this check is equivalent to "aCoord.ConvertsToLength()"? (It looks like you're filtering for lengths & calcs-that-only-involve-lengths, and it looks to me like ConvertsToLength is a more direct way to check for that.) @@ +5993,5 @@ > + nscoord aMin, > + nscoord* aResult) { > + MOZ_ASSERT(aMin >= 0, "aMin is supposed to be a size"); > + if (aCoord.IsCoordPercentCalcUnit() && !aCoord.HasPercent()) { > + nscoord res = nsRuleNode::ComputeCoordPercentCalc(aCoord, 0); ...and I think this is equivalent to aCoord->ToLength() (given that we've already screened for percentages), right? ConvertsToLength() and ToLength() are defined here: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h#178 (There are static versions with the actual logic, which then get called by the simpler member-functions.) @@ +6002,5 @@ > + }; > + LogicalSize sz(state.mWM, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE); > + LogicalSize min(state.mWM, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE); > + LogicalSize max(state.mWM, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE); > + nscoord& minI = min.ISize(state.mWM); Could you add some comments explaining what these variables represent, and what this code is doing? (In particular, one question: why are we using NS_UNCONSTRAINEDSIZE as our minimum lengths [with special-cases for treating it as 0 in the lambda], instead of just directly using 0 as our minimum? There's probably a reason, I'm just not seeing it right away.

OK, I'll use ConvertsToLength() and ToLength() instead, thanks. (I went back and forth here for a while, regarding whether to treat calc(Apx + B%) as indefinite or as Apx, before deciding. I think I'll tweak this again in the future depending on bug 1302541 is resolved, and in that case let the B% contribute to the SumOfPercentages part.) > ... NS_UNCONSTRAINEDSIZE ... instead of just directly using 0 as our minimum? I think it was for this check: https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/layout/generic/nsGridContainerFrame.cpp#976 but I think we could just change that to "aMinSize == 0" instead.

Yup, I think that change would make sense. Thanks!

Using zero for the min-size also simplifies the call from Reflow nicely. Hmm, I wonder why I didn't do this in the first place? oh well... https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c1138f8068c

Comment on attachment 8792346 [details] [diff] [review] fix Review of attachment 8792346 [details] [diff] [review]: ----------------------------------------------------------------- The first two chunks make sense, in light of the end of comment 10 -- but they don't really seem related to the patch's commit message (or the main intrinsic-sizing change in this patch). Perhaps it'd be best to split this into two pieces, where: * part 1 contains the first two chunks, PLUS a much-simplified version of the third chunk where we simply simply use a zero-sized LogicalSize instead of "indefinite" for the minimum size. (I suspect, but am not sure, that this patch as a whole wouldn't change behavior.) * part 2 would actually adjust the IntrinsicISize impl to do what this patch's commit message says (take [min-/max-]width into account) ::: layout/generic/nsGridContainerFrame.cpp @@ +5992,5 @@ > + nscoord& minI = min.ISize(state.mWM); > + GetDefiniteSize(state.mGridStyle->MinISize(state.mWM), 0, &minI); > + if (!GetDefiniteSize(state.mGridStyle->ISize(state.mWM), minI, > + &sz.ISize(state.mWM))) { > + GetDefiniteSize(state.mGridStyle->MaxISize(state.mWM), minI, I'm still not understanding the logic behind these GetDefiniteSize() calls. With the patch right now, we: - Unconditionally resolve the min-ISize (to 0 if indefinite) - Attempt to resolve the ISize (and min-clamp it). - If that succeeds, we leave the max ISize at NS_UNCONSTRAINEDSIZE. (why???) - If that fails, we resolve the max ISize. I'm particularly confused about the part that I flagged with "why???". If we have width:100px, but max-width:50px, it doesn't make sense to me that we'd just proceed with our 100px width and an indefinite max-width... @@ +5997,5 @@ > + &max.ISize(state.mWM)); > + } > + if (state.mRowFunctions.mHasRepeatAuto && > + !(state.mGridStyle->mGridAutoFlow & NS_STYLE_GRID_AUTO_FLOW_ROW)) { > + // Our block-size may affect the number of columns. Can you clarify this comment a bit? (and how it relates to the two-part condition that it's inside) (Is that condition the only scenario in which our block size can impact the number of columns?) @@ +6002,5 @@ > + nscoord& minB = min.BSize(state.mWM); > + GetDefiniteSize(state.mGridStyle->MinBSize(state.mWM), 0, &minB); > + if (!GetDefiniteSize(state.mGridStyle->BSize(state.mWM), minB, > + &sz.BSize(state.mWM))) { > + GetDefiniteSize(state.mGridStyle->MaxBSize(state.mWM), minB, (This BSize logic is the same as the ISize logic above, so the same question applies here -- why do we only conditionally care about the max-bsize property?)

(In reply to Daniel Holbert [:dholbert] from comment #13) > With the patch right now, we: > - Unconditionally resolve the min-ISize (to 0 if indefinite) > - Attempt to resolve the ISize (and min-clamp it). > - If that succeeds, we leave the max ISize at NS_UNCONSTRAINEDSIZE. > (why???) > - If that fails, we resolve the max ISize. Er, sorry, my indentation was off on the last line there, making the implied-logical-nesting wrong. Just disregard the last line, really, because the line before it (about leaving the max ISize at NS_UNCONSTRAINEDSIZE) is the part I'm really concerned with.

Right, this is idempotent.

Comment on attachment 8792695 [details] [diff] [review] part 1 Review of attachment 8792695 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me on part 1

You were right there was a bug lurking here, good catch! I don't think it would occur in the inline axis though since IntrinsicISize isn't called when there is a specified inline size. It does occur with a specified bsize + max-bsize though. I've added reftests to cover that.

Comment on attachment 8792707 [details] [diff] [review] fix Review of attachment 8792707 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: layout/generic/nsGridContainerFrame.cpp @@ +6004,5 @@ > + &max.ISize(state.mWM)); > + } > + if (state.mRowFunctions.mHasRepeatAuto && > + !(state.mGridStyle->mGridAutoFlow & NS_STYLE_GRID_AUTO_FLOW_ROW)) { > + // Only 'grid-auto-flow:row' can create new implicit columns, so that's Two things: (1) Comment typo s/:row/:column/ (right?) (2) This comment doesn't explain the mRowFunctions.mHasRepeatAuto check -- it only explains the mGridAutoFlow check. Could you broaden the comment to explain that first condition, too?

Comment on attachment 8792708 [details] [diff] [review] reftests Review of attachment 8792708 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by note on the reftests: ::: layout/reftests/css-grid/reftest.list @@ +124,5 @@ > == grid-repeat-auto-fill-fit-007.html grid-repeat-auto-fill-fit-007-ref.html > == grid-repeat-auto-fill-fit-008.html grid-repeat-auto-fill-fit-008-ref.html > == grid-repeat-auto-fill-fit-009.html grid-repeat-auto-fill-fit-009-ref.html > +== grid-repeat-auto-fill-fit-010.html grid-repeat-auto-fill-fit-010-ref.html > +== grid-repeat-auto-fill-fit-011.html grid-repeat-auto-fill-fit-010-ref.html Nit: it's always a bit confusing (IMO) to have a test numbered -011 but no reference named -011-ref.html. So: I'd prefer these were numbered -010a and -010b (since they both match -010-ref) instead of -010 and -011. But, not a big deal; fine if you want to leave it as-is too.

> (right?) Right, thanks.

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a25ca70d82f part 1 - [css-grid] Simplify handling of min-size for repeat track calculation. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/941f9bfc8b66 part 2 - [css-grid] Take any specified [min-/max-]width/height into account when calculating the number of auto-fill/fit tracks for intrinsic sizing. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/8c51ff54a0ea part 3 - [css-grid] More reftests for auto-fill/fit intrinsic sizing.

https://hg.mozilla.org/mozilla-central/rev/8a25ca70d82f https://hg.mozilla.org/mozilla-central/rev/941f9bfc8b66 https://hg.mozilla.org/mozilla-central/rev/8c51ff54a0ea