1473047 - [css-flexbox] Need to resolve block-axis column-gap/row-gap percentages, when determining positions of flex items & flex lines

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load https://jsfiddle.net/81tboure/

EXPECTED RESULTS: Gap between blue and magenta box, 50% of the height of the black bordered area (pushing the magenta box outside the black border)

ACTUAL RESULTS: No gap.


The working group went back and forth on this behavior, but I think they settled on "percentages don't contribute to sizing during intrinsic sizing calculations, i.e. for determining the size of the (flex) container, but they do resolve against the container for layout-of-the-children purposes after we've determined that size.

Mats has a testcase showing this working correctly (in Firefox Nightly at least) for css grid:
https://github.com/w3c/csswg-drafts/issues/509#issuecomment-386332974
https://jsfiddle.net/aLx0sv3p/

Comment 1

[Mihir Iyer [:mihir]]

Attachment: Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

Review commit: https://reviewboard.mozilla.org/r/259202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259202/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

https://reviewboard.mozilla.org/r/259202/#review267626

::: layout/generic/nsFlexContainerFrame.cpp:1062
(Diff revision 1)
>     */
>    nscoord GetMainGapSize() const {
>      return mMainGapSize;
>    }
>  
> +  inline void SetMainGapSize (nscoord aMainGapSize) {mMainGapSize = aMainGapSize;}

Two nits:
 (1) this line is too long (80 character limit)
 (2) this line needs a space between the curly-braces and their contents.

Let's address (1) by renaming the arg to just "aNewSize", which saves you a bunch of characters and lets you add spaces to address (2) while still staying under 80 characters.

::: layout/generic/nsFlexContainerFrame.cpp:2315
(Diff revision 1)
> +  inline void SetCrossGapSize(nscoord aCrossGapSize) {mCrossGapSize = aCrossGapSize;}
> +

Please fix as noted above (add a space just inside each curly brace, and rename aCrossGapSize to aNewSize).

::: layout/generic/nsFlexContainerFrame.cpp:4861
(Diff revision 1)
> +  // Recalculate the gap sizes now that the container size has been determined.
> +  nsStyleCoord mainGap;
> +  nsStyleCoord crossGap;
> +  if (aAxisTracker.IsRowOriented()) {
> +    mainGap = aReflowInput.mStylePosition->mColumnGap;
> +    crossGap = aReflowInput.mStylePosition->mRowGap;
> +  } else {
> +    mainGap = aReflowInput.mStylePosition->mRowGap;
> +    crossGap = aReflowInput.mStylePosition->mColumnGap;
> +  }

A few issues here:
 (1) Nit: right now, this code would copy these nsStyleCoord values into the local variables, and that might be kinda expensive if these happen to represent calc() expressions or something else nontrivial. So, to the extent that we capture these in local variables, we should rewrite this to use "const nsStyleCoord&" references (but don't do that just yet, read on:)

 (2) I think we only ever need to bother re-resolving *one* of our gaps here -- the one that is in the block dimension.  And I think that happens to *always* be the row-gap. Regardless of whether we're row- or column-oriented, the "row-gap" is the gap that's in our block dimension.  Plus: we only need to re-resolve it if it has a percent value *and* we lack a precomputed/specified size in the block axis.

I think we should probably do something like the following here:

  if (aReflowInput.ComputedBSize() == NS_INTRINSICSIZE &&
      aReflowInput.mStylePosition->mRowGap.HasPercent()) {
      // Re-resolve the row-gap (the gap in the block-axis), now that we've
      // definitely got a size for the flex container in that axis.
      ...This code should use main or cross size, and update main or cross gap.
      ...(using cross axis iff IsRowOriented(); otherwise using main axis).
    }

(See https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsGridContainerFrame.cpp#6023-6026 for where this happens in grid.)

Note: if we do have to update the main-gap-size for every line (i.e. if we're column-oriented), then we could either do so in a new dedicated (short) loop over all the lines here, or we could keep it how you've got it with an update to the loop below - either way.  But if we do it in the existing loop below, I think it'd be nice to structure it so that it's optional, because it's unlikely that we need to re-resolve it.

::: layout/generic/nsFlexContainerFrame.cpp:4888
(Diff revision 1)
> +    nscoord oldMainGapSize = line->GetMainGapSize();
> +    line->SetMainGapSize(finalMainGapSize);
>      line->PositionItemsInMainAxis(justifyContent,
>                                    aContentBoxMainSize,
>                                    aAxisTracker);
> +    line->SetMainGapSize(oldMainGapSize);

Is there a reason we need to restore the oldMainGapSize here?  (Isn't it better to leave this with the more-accurate final resolved gap size?)

Comment 3

[Mihir Iyer [:mihir]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259202/diff/1-2/

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

https://reviewboard.mozilla.org/r/259202/#review267656

::: commit-message-0d72c:1
(Diff revision 2)
> +Bug 1473047 - Resolve block-axis col/row-gap percentages after intrinsic size calculated.

Two nits:
 (1) s/Resolve/Re-resolve/
 (2) Drop the mention of "col" here, since we don't re-resolve that.

::: layout/generic/nsFlexContainerFrame.cpp:4861
(Diff revision 2)
> +  // Recalculate the gap sizes if nevessary now that the container size has
> +  // been determined.

s/nevessary/necessary

::: layout/generic/nsFlexContainerFrame.cpp:4871
(Diff revision 2)
> +    if (rowIsCross) {
> +        crossAxisPosnTracker.SetCrossGapSize(newBlockGapSize);

Too much indentation on this inner line here -- deindent by 2 spaces.

Comment 5

[Mihir Iyer [:mihir]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259202/diff/2-3/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

https://reviewboard.mozilla.org/r/259202/#review267658

::: commit-message-0d72c:1
(Diff revisions 2 - 3)
> -Bug 1473047 - Resolve block-axis col/row-gap percentages after intrinsic size calculated.
> +Bug 1473047 - Re-resolve block-axis row-gap percentages after intrinsic size calculated.

Sorry, one final nit that I didn't realize until seeing the final formulation here: could you wrap "block-axis" in parens here?

Or alternately, just drop "block-axis" and replace s/intrinsic size/intrinsic block size/

Right now, "block-axis row-gap" is redundant & hence a bit confusing (it sounds like there could be inline-axis row-gaps, but there cannot).

Comment 7

[Mihir Iyer [:mihir]]

Comment on attachment 8996406 [details]
Bug 1473047 - Re-resolve row-gap percentages after intrinsic block size calculated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259202/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/347512fd214e
Re-resolve row-gap percentages after intrinsic block size calculated. r=dholbert

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/347512fd214e

Comment 10

[Daniel Holbert [:dholbert]]

Note: this bug's code-changes are being reverted in bug 1639627, in accordance with a CSSWG resolution.