1280798 - [css-grid] Wrong intrinsic grid container size in some repeat(auto-fill/fit) cases

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: testcases

Comment 1

[Brad Werth [:bradwerth]]

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.

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Sergio Villar]

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.

Comment 4

[Sergio Villar]

Attachment: Patch for current tests

I'm specifically talking about these cases

Comment 5

[Mats Palmgren (inactive)]

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

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

Comment 7

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 8

[Mats Palmgren (inactive)]

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

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Mats Palmgren (inactive)]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[Mats Palmgren (inactive)]

Attachment: fix

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 13

[Daniel Holbert [:dholbert]]

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?)

Comment 14

[Daniel Holbert [:dholbert]]

(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.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 1

Right, this is idempotent.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8792695 [details] [diff] [review]
part 1

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

Thanks! r=me on part 1

Comment 17

[Mats Palmgren (inactive)]

Attachment: fix

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 18

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 19

[Daniel Holbert [:dholbert]]

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 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Mats Palmgren (inactive)]

> (right?)

Right, thanks.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 23

[Carsten Book [:Tomcat]]

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