Closed Bug 1398483 Opened 7 years ago Closed 6 years ago

[css-flexbox] Implement flexbox layout for the row-gap and column-gap properties

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox63 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: mihir)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P1][designer-tools])

Attachments

(3 files)

testcase 1
844 bytes, text/html
Details
reference case 1
893 bytes, text/html
Details
Bug 1398483 - Implement column and row gap for flexbox.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
https://logs.csswg.org/irc.w3.org/css/2017-08-04/#e847329
"Resolved: column-gap and row-gap apply to flex, grid, and multicol"

https://github.com/w3c/csswg-drafts/issues/1696

These are now defined in the Box Alignment spec:
https://www.w3.org/TR/css-align-3/#gaps

There is no spec text in the flexbox spec yet though...
Perhaps it's actually fully defined for flexbox in css-align:
https://www.w3.org/TR/css-align-3/#gap-flex
status-firefox57: affected → ---
status-firefox57: --- → wontfix
Priority: -- → P3
Flags: needinfo?(dholbert)
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1][designer-tools]
Any update? I don’t think we should let this go too long.
Yeah, sorry -- I'll take a look at this tomorrow.
Note that we need the CSSWG to make up its mind up about how to
resolve percentages with an indefinite percentage basis before
can implement this (and bug 1434478) correctly.
https://github.com/w3c/csswg-drafts/issues/2297
(That issue was first resolved as invalid, but then reopened,
so I'm assuming it's still undecided how it should be handled.)

While we can certainly start working on these things before then,
it'd be nice to have that decision so we don't have to waste time
on tweaking the code later...
In particular, writing reftests for one alternative and then
having to tweak them later is usually rather time consuming
(more so than tweaking the code probably).
(In reply to Jen Simmons [:jensimmons] from comment #2)
> Any update? I don’t think we should let this go too long.

Hmm -- from my testing, it doesn't look like any other browsers implement "column-gap"/"row-gap" for flexbox yet, actually. (Not that that should stop us from implementing it - but that lowers it on my priority queue for the moment, I think.  There are other flexbox bugs that I feel more inclined to catch up on, rather than implementing something that nobody else has implemented yet.)

Also: beyond comment 5, the current spec text isn't precise enough to be implementable. A few specific issues:
 1) It's not clear how "row-gap" and "align-content:space-around" interact.  The spec says "indicates minimum spacing between adjacent flex lines", but that's still quite vague.  Example: suppose we've got 100px of packing space to distribute around 2 flex lines. On its own, "align-content:space-around" would put 25px at the start, 50px between the lines, and 25px at the end.  If we add "row-gap:60px", then clearly we end up with 60px rather than 50px between the lines -- but should we still have 25px of space at the beginning?  Or should the 60px be subtracted out first, stealing some (maybe all) of the beginning/end packing space?

 2) It's not clear how these "gap" properties and margins interact...
   2a) In the main axis, do the gaps overlay (include) the items' margins, or do they add extra space *in addition to* the margins (in the same way that 'justify-content' packing space does)?
   2b) In the cross axis, clearly flex lines don't have their own margins, so it seems clearer there that the gaps are extra space beyond the items' margins (modulo my uncertainty in question (1)).  Maybe this is a reason that we should make them extra space beyond the items' margins in the main axis, too?  That way, "margin: 10px; column-gap: 5px; row-gap: 5px" will produce something with consistent spacing, as an author might expect...

I'll file spec bug(s) about the above & maybe start working on this after that, but for now I don't think this is actionable.
Flags: needinfo?(dholbert)
Attached file testcase 1 — 
Here's the testcase I used for testing other browsers. I see no gaps in:
 Chrome Version 66.0.3343.3 (Official Build) dev (64-bit)
 Edge 16
 Safari 11

(Note that the Chrome version I'm testing is the "dev" channel, which IIRC comes from periodic snapshots of their trunk.)

Restoring needinfo=me to remind myself to file spec bugs on this (which I'll aim to do first thing next week).
Flags: needinfo?(mats)
Flags: needinfo?(mats) → needinfo?(dholbert)
I filed https://github.com/w3c/csswg-drafts/issues/2336 with a summary of the things I found in the spec that need clarification.
Flags: needinfo?(dholbert)
Keywords: site-compat
Replying to my comment 6 / comment 8:  based on fantasai's response on my github issue, I think these gaps are separate from (but preallocated before) other sorts of "distribute-the-rest-of-the-space" things.

So if "justify-content" or "flex:1" or "margin:auto" would previously have had X pixels to distribute, now they'll have X pixels minus the column-gaps... I think.
[Sorry, just noticed some typos after posting, so I'll repost with those fixed for clarity (and autohide the broken version]

Handy-wavy summary of the code changes that I think need to happen here:

MAIN-AXIS STUFF (accounting for gaps between flex items [in the main axis]):
 (1) In the nsFlexContainer::GetMinISize and GetPrefISize() "axisTracker.IsRowOriented()" case, we should add space for 'column-gap' between each item.

 (2) In "AddItem", here...
 https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#942 
...we should be adding a copy of 'column-gap' to mTotalOuterHypotheticalMainSize (note "Outer" not "Inner", because the "inner" one just tracks content-box sizes that can be flexed/shrunk).  (Using column-gap here if aAxisTracker.IsRowOriented() is true -- or we'd want to use the 'row-gap' if it's false).

 (3) In FlexLine::PositionItemsInMainAxis() (which walks across the items, packing space, etc.), we need to also step across a copy of the 'column-gap' between each item (if aAxisTracker.IsRowOriented() is true -- or we'd use the 'row-gap' if it's false).

CROSS-AXIS STUFF (accounting for gaps between flex lines [in the cross axis a multi-line flexbox]):
 (4) In the CrossAxisPositionTracker constructor, when we figure out how much packing space we have, we need to subtract a copy of the row-gap between each flex line (if aAxisTracker.IsRowOriented() is true -- or we'd subtract the column-gap if it's false).
https://dxr.mozilla.org/mozilla-central/rev/62c2508f27a865dd0ac5ca3f0485cb20afafbbd0/layout/generic/nsFlexContainerFrame.cpp#3046-3053

 (5) Similar to (3) -- when we call crossAxisPosnTracker.TraverseLine(...) and crossAxisPosnTracker.TraversePackingSpace(), to step over a flex line and cross-axis packing space: we need to *also* step across some space for the 'row-gap' (if aAxisTracker.IsRowOriented() is true -- or we'd use the column-gap if it's false).


Note: It might be OK to fold in the stepping in comment (3) and (5) into their respective "TraversePackingSpace()" functions (probably still representing the packing space separately from the gap, but traversing it at the same time).
[I'm probably going to use this as an intern "good first/second bug" in a week or two, BTW]
Assignee: nobody → iyermihir
Status: NEW → ASSIGNED
Attached file reference case 1 — 
Here's a reference case for the testcase, BTW (using CSS Grid instead of CSS Flexbox -- so that "column-gap" and "row-gap" having a visual effect, per bug 1398482).
Attachment #8987637 - Flags: review?(dholbert)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259418

This looks really good!

Haven't reviewed thoroughly yet, but here are my notes from my first look, to get you started:

::: commit-message-6e8e8:1
(Diff revision 1)
> +Bug 1398483 - Implement column and row gap for flexbox. Includes reftests.

Probably drop the "includes reftests", to make the commit message a bit shorter, and because it's implied.

(It's typically assumed that every commit will include tests.  If the tests are in their own dedicated commit, as they sometimes are, then *that* commit would mention them in its commit message, but otherwise they're not worth calling out as part of the [brief] commit message.)

::: layout/generic/nsFlexContainerFrame.cpp:956
(Diff revision 1)
>                 nscoord aItemInnerHypotheticalMainSize,
> -               nscoord aItemOuterHypotheticalMainSize)
> +               nscoord aItemOuterHypotheticalMainSize,
> +               const FlexboxAxisTracker& aAxisTracker,
> +               const nsStylePosition* stylePos)

It doesn't look like these new args (aAxisTracker and aStylePos) are used here at all, so you can revert this change, I think.

::: layout/generic/nsFlexContainerFrame.cpp:2267
(Diff revision 1)
>      MOZ_ASSERT(mNumAutoMarginsInMainAxis == 0,
>                 "miscounted the number of auto margins");
>    }
>  
> +  // Advances past the gap space (if any) between two flex items
> +  void TraverseGap(nscoord aGapSize) {mPosition += aGapSize;}

Nit: please add a space just inside each curly-brace.

::: layout/generic/nsFlexContainerFrame.cpp:2298
(Diff revision 1)
>                             bool aIsCrossSizeDefinite,
> -                           const FlexboxAxisTracker& aAxisTracker);
> +                           const FlexboxAxisTracker& aAxisTracker,
> +                           const nscoord aCrossGapSize);
> +
> +  // Advances past the gap (if any) between two flex lines
> +  void TraverseGap() {mPosition += mCrossGapSize;}

Nit: please add a space just inside each curly-brace.

::: layout/generic/nsFlexContainerFrame.cpp:2602
(Diff revision 1)
>      return;
>    }
>    MOZ_ASSERT(!IsEmpty() || aLineInfo,
>               "empty lines should take the early-return above");
>  
> -  // Subtract space occupied by our items' margins/borders/padding, so we can
> +  // Subtract space occupied by our items' margins/borders/padding/gaps, so 

nit: there's a trailing space character on this line (MozReview handily highlights this in red).

Please drop this trailing space character.

::: layout/generic/nsFlexContainerFrame.cpp:2943
(Diff revision 1)
> +  // Subtract space required for row/col gap from the remaining packing space
> +  mPackingSpaceRemaining -= aLine->GetMainGapSize() * (aLine->GetNumItems()-1);

nit: add spaces around the "-", for consistency.

Also, this change will also push this line over 80 characters, so probably wrap (and indent 2 spaces with respect to the previous line's indentation) either after the "-=" or after the "*".

::: layout/generic/nsFlexContainerFrame.cpp:3154
(Diff revision 1)
>      mPackingSpaceRemaining -= line->GetLineCrossSize();
>      numLines++;
>    }
>  
> +  // Subtract space required for row/col gap from the remaining packing space
> +  mPackingSpaceRemaining -= aCrossGapSize * (numLines-1);

Add spaces around "-".

::: layout/generic/nsFlexContainerFrame.cpp:3951
(Diff revision 1)
> -    curLine->AddItem(item.release(), shouldInsertAtFront,
> +    curLine->AddItem(item.release(),
> +                     shouldInsertAtFront,
>                       itemInnerHypotheticalMainSize,
> -                     itemOuterHypotheticalMainSize);
> +                     itemOuterHypotheticalMainSize,
> +                     aAxisTracker,
> +                     StylePosition());

s/StylePosition()/aReflowInput.mStylePosition/

StylePosition() has a nonzero (but small) duration/cost. So we look it up once just before the start of the frame's Reflow method, and cache it on ReflowInput, and use that already-known pointer, where possible, rather than calling StylePosition() again.

::: layout/generic/nsFlexContainerFrame.cpp:4122
(Diff revision 1)
>  void
>  FlexLine::PositionItemsInMainAxis(uint8_t aJustifyContent,
>                                    nscoord aContentBoxMainSize,
> -                                  const FlexboxAxisTracker& aAxisTracker)
> +                                  const FlexboxAxisTracker& aAxisTracker,
> +                                  const nsStylePosition* aStylePos)
>  {

aStylePos is unused here, so you can revert the changes to this function signature.

(I'm guessing you used it originally to lookup the gap size, before you were caching the gap on the FlexLine.)

::: layout/generic/nsFlexContainerFrame.cpp:4333
(Diff revision 1)
> +  // Calculate gap size for main and cross axis
> +  nscoord crossGapSize;
> +  nscoord mainGapSize;
> +  if (axisTracker.IsRowOriented()) {
> +    mainGapSize = nsLayoutUtils::ResolveGapToLength(stylePos->mColumnGap,

Nit: please swap the ordering of the declarations here (declaring mainGapSize first), since the main axis is the "primary" axis.

::: layout/generic/nsFlexContainerFrame.cpp:4355
(Diff revision 1)
>    DoFlexLayout(aPresContext, aDesiredSize, aReflowInput, aStatus,
>                 contentBoxMainSize, availableBSizeForContent,
> -               struts, axisTracker);
> +               struts, axisTracker, crossGapSize, mainGapSize);
>  
>    if (!struts.IsEmpty()) {
>      // We're restarting flex layout, with new knowledge of collapsed items.
>      aStatus.Reset();
>      DoFlexLayout(aPresContext, aDesiredSize, aReflowInput, aStatus,
>                   contentBoxMainSize, availableBSizeForContent,
> -                 struts, axisTracker);
> +                 struts, axisTracker, crossGapSize, mainGapSize);

As above: since the main axis is the "primary" axis, let's pass in mainGapSize first and crossGapSize second (and update the DoFlexLayout function signature to expect them in that order, too).

::: layout/generic/nsFlexContainerFrame.cpp:4804
(Diff revision 1)
> +  const nsStylePosition* stylePos = StylePosition();
> +
>    lineIndex = 0;
>    for (FlexLine* line = lines.getFirst(); line; line = line->getNext(),
>                                                  ++lineIndex) {
>      // Main-Axis Alignment - Flexbox spec section 9.5
>      // ==============================================
>      line->PositionItemsInMainAxis(justifyContent,
>                                    aContentBoxMainSize,
> -                                  aAxisTracker);
> +                                  aAxisTracker,
> +                                  stylePos);

(No need for these changes anymore, as noted in PositionItemsInMainAxis.)

::: layout/generic/nsFlexContainerFrame.cpp:5251
(Diff revision 1)
> +  }
> +
>    const bool useMozBoxCollapseBehavior =
>      ShouldUseMozBoxCollapseBehavior(StyleDisplay());
>  
> +  bool hasCols = false;

We can probably name this something clearer than "hasCols".  (This name is partly confusing because flex containers don't really have "columns" per se.  They have items, which are within flex lines, and either the items or the lines are analogous to columns for the purpose of row/column-gap, but aren't really columns.)

How about:

  // The loop below sets aside space for a {column,row}-gap before each item,
  // besides the 1st. This bool helps us handle that special-case.
 bool onFirstChild = true;

And then inside the loop:
  if (!onFirstChild) {
    containerISize += mainGapSize;
  }
  onFirstChild = false;

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001-ref.html:14
(Diff revision 1)
> +-->
> +<html>
> +<head>
> +  <title>Reference: Testing horizontal and vertical multi-line flex container with the space-around property and auto margins</title>
> +  <link rel="author" title="Mihir Iyer" href="mailto:miyer@mozilla.com">
> +  <link rel="help" href="">

Drop the empty <link rel="help"> tag here -- it's not needed in the reference case.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001.html:14
(Diff revision 1)
> +-->
> +<html>
> +<head>
> +  <title>CSS Test: Testing horizontal and vertical multi-line flex containers with the space-around property and auto margins</title>
> +  <link rel="author" title="Mihir Iyer" href="mailto:miyer@mozilla.com">
> +  <link rel="help" href="">

In both testcases here, this help="" attribute needs a spec link. Probably this one:
 https://drafts.csswg.org/css-align/#gap-flex

(This is used to automatically link up specs & the tests that cover them, and also for human readers to figure out what spec text is related to a given test that they're trying to understand.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-002.html:7
(Diff revision 1)
> +<!--
> +     Any copyright is dedicated to the Public Domain.
> +     http://creativecommons.org/publicdomain/zero/1.0
> +-->
> +<!--
> +     Testcase for the correctness of gaps in vertical writing mode  multi-line

nit: you've got two spaces after "mode" here -- get rid of one of those spaces.

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list:101
(Diff revision 1)
>  == flexbox-collapsed-item-baseline-001.html flexbox-collapsed-item-baseline-001-ref.html
>  == flexbox-collapsed-item-horiz-001.html flexbox-collapsed-item-horiz-001-ref.html
>  == flexbox-collapsed-item-horiz-002.html flexbox-collapsed-item-horiz-002-ref.html
>  == flexbox-collapsed-item-horiz-003.html flexbox-collapsed-item-horiz-003-ref.html
>  
> +# Tests for "row-gap and column-gap"

Nit:
s/"row-gap and column-gap"/
 "row gap" and "column-gap"/

(Give each term its own quotes.)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259790

::: layout/generic/nsFlexContainerFrame.h:261
(Diff revision 2)
> -                    const FlexboxAxisTracker& aAxisTracker);
> +                    const FlexboxAxisTracker& aAxisTracker,
> +                    nscoord aCrossGapSize,
> +                    nscoord aMainGapSize);

swap Cross <--> Main variables here, so that Main is listed first.

(You already fixed this up in the implementation & in callers, but the old ordering is still present in the declaration here.)

::: layout/generic/nsFlexContainerFrame.cpp:990
(Diff revision 2)
> +    // If the item added was not the first item in the line, we add in any gap
> +    // space as needed.
> +    if (mNumItems >= 2) {
> +      mTotalOuterHypotheticalMainSize =
> +        AddChecked(mTotalOuterHypotheticalMainSize, mMainGapSize);

Please update the code-comment above this member-variable's declaration to indicate that it includes the sizes of main-axis {row,column}-gaps between items.

And while you're at it, please also update the documentation for its getter, GetTotalOuterHypotheticalMainSize().

(Both have documentation that says something very specific right now & which does *not* mention gaps.)

::: layout/generic/nsFlexContainerFrame.cpp:1101
(Diff revision 2)
>  
>    nscoord mLineCrossSize;
>    nscoord mFirstBaselineOffset;
>    nscoord mLastBaselineOffset;
> +
> +  // Maintain size of gaps in main axis

s/size of gaps/size of each {row,column}-gap/

(This makes it clearer that
 (a) this is the **per-gap** size, not the total size
 (b) this is about "gaps" from the row/column-gap properties, not from e.g. packing space

::: layout/generic/nsFlexContainerFrame.cpp:2940
(Diff revision 2)
> +  // Subtract space required for row/col gap from the remaining packing space
> +  mPackingSpaceRemaining -=
> +    aLine->GetMainGapSize() * (aLine->GetNumItems() - 1);

(Do we know aLine->GetNumItems() is nonzero here? If it could be zero, then this subtraction is problematic...)

::: layout/generic/nsFlexContainerFrame.cpp:3151
(Diff revision 2)
> +  // Subtract space required for row/col gap from the remaining packing space
> +  mPackingSpaceRemaining -= aCrossGapSize * (numLines - 1);
> +

numLines could be 0 here, I think, so this subtraction is slightly problematic.  This needs to be wrapped in "if (numLines > 0)", I think?

(Or if we always have positive numLines here, maybe add an assertion to that effect here?)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259790

> (Do we know aLine->GetNumItems() is nonzero here? If it could be zero, then this subtraction is problematic...)

For this zero edge-case handling, it might be simplest to add a aLine->GetSumOfGaps() function, which would just return
 mNumItems == 0 ? 0 : (mNumItems - 1) * mMainGapSize;

You'd probably want to use that function in FlexLine::ResolveFlexibleLengths, too (where you compute  spaceAvailableForFlexItemsContentBoxes)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259790

> numLines could be 0 here, I think, so this subtraction is slightly problematic.  This needs to be wrapped in "if (numLines > 0)", I think?
> 
> (Or if we always have positive numLines here, maybe add an assertion to that effect here?)

Sorry -- I'm actually 99% sure we should always have at least 1 line here.

(Justification: Flexbox is "single-line" or "multi-line" -- where multi = 1 and maybe more, and nsFlexContainerFrame::GenerateFlexLines has a comment a few lines in saying we always have at least one (possibly-empty) line.)

So: rather than adding handling for an impossible special-case here, let's just add a strong assertion to make our assumption explicit. e.g.
  MOZ_ASSERT(numLines > 1,
             "GenerateFlexLines should've produced at least 1 line");
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259826

::: layout/generic/nsFlexContainerFrame.cpp:3942
(Diff revision 3)
>        // rather than (possibly-overflowing) normal addition, to be sure we don't
>        // make the wrong judgement about whether the item fits on this line.
>        nscoord newOuterSize =
>          AddChecked(curLine->GetTotalOuterHypotheticalMainSize(),
>                     itemOuterHypotheticalMainSize);
> +      newOuterSize = AddChecked(newOuterSize, aMainGapSize);

Let's add a comment before this newOuterSize tweak -- maybe something like:
          // Account for gap between previous line's previous item & this item:


This (a) makes it clearer which gap we're accounting for here (the one before, not after) and (b) reinforces that this is *skipped* for the first item (which is how we end up accounting for N-1 gaps among N items here).  (This skipping happens via a curLine->IsEmpty() check a bit further up.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001-ref.html:27
(Diff revision 3)
> +    .rowiNoWrap {
> +      flex-flow: row nowrap;
> +    }

This rule (for "rowiNoWrap") has a typo in the class name -- there's a stray "i", and it should be "rowNoWrap".

(The test still happens to work even with the typo, because this class happens to be setting the default values -- i.e. flex containers default to "row nowrap". Anyway, it still seems reasonable to keep this [typo-fixed] CSS, to make things clearer in comparison to the columnNoWrap version alongside it.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001.html:12
(Diff revision 3)
> +     Testcase for the correctness of gaps in horizontal and vertical multi-line
> +     flex containers.
> +-->
> +<html>
> +<head>
> +  <title>CSS Test: Testing horizontal and vertical multi-line flex containers with the space-around property and auto margins</title>

The title here should mention row-gap and column-gap, since that's the primary thing you're testing here.

Maybe just "CSS Test: Testing row-gap and column-gap in multi-line flex containers"

(This is kind of the same content that you have in the <!-- ... --> HTML comment at the top -- feel free to remove that comment if you like, since it's a bit redundant given a similar-length <title> tag.  Or feel free to leave it around, no big deal. I think I added those <!-- --> comments in some of my older tests when I had a larger high-level test-overview than what I could express in a concise <title>.)

(Also: looks like you've got a similar <title> in the reference case, too, so you'd want to update that as well.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001.html:20
(Diff revision 3)
> +      width: 200px;
> +      height: 200px;

Could you make the container have a different height vs. width, so that we can tell if we happen to be using the wrong dimension to resolve percentages?  (This applies to both -001 and -002 tests.)

Sorry, I know this may invalidate some of the hardcoded math in the testcases. :-/  It's worth it for the robustness of being sure we're handling percentages correctly, though.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-002.html:7
(Diff revision 3)
> +     Testcase for the correctness of gaps in vertical writing mode  multi-line
> +     flex containers.

(There's a stray double space here after "mode".)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-002.html:12
(Diff revision 3)
> +     Testcase for the correctness of gaps in vertical writing mode  multi-line
> +     flex containers.
> +-->
> +<html>
> +<head>
> +  <title>CSS Test: Testing vertical writing mode multi-line flex containers</title>

As with the other testcase, the title here should mention row-gap and column-gap.
Attachment #8987637 - Flags: review?(dholbert) → review-
(I feel like I've now grokked the whole patch, so I think this will be r+ with that last round of review comments addressed! I marked it r- for now since I think it's worth a final once-over review pass on the fixups, though.)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259832

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-001.html:20
(Diff revision 3)
> +      width: 200px;
> +      height: 200px;

Also: when tweaking the size, try to keep the whole thing smaller than 600x600 pixels, per guideline at https://web-platform-tests.org/writing-tests/reftests.html

So in particular, you could increase the container width up to ~298px right now (which makes them 300px wide with), but no more than that, because you've got these 2 containers stacked horizontally, and 2 x 300px = 600 which is the maximum area that we're supposed to depend on having for reftests.

Alternately, if you want them to be wider, you could increase the width by more than that and just get rid of "float: left", so that then they'd stack vertically and would still fit into a 600x600 area.
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259826

> Let's add a comment before this newOuterSize tweak -- maybe something like:
>           // Account for gap between previous line's previous item & this item:
> 
> 
> This (a) makes it clearer which gap we're accounting for here (the one before, not after) and (b) reinforces that this is *skipped* for the first item (which is how we end up accounting for N-1 gaps among N items here).  (This skipping happens via a curLine->IsEmpty() check a bit further up.)

Just noticed that I had a typo in my suggested text here :D  ("previous line's previous item" should've just said "this lines's previous item")

In case you happened to use this exact suggested text, please correct that typo. (& sorry)
Comment on attachment 8987637 [details]
Bug 1398483 - Implement column and row gap for flexbox.

https://reviewboard.mozilla.org/r/252864/#review259854

r=me, one final nit on one of the changes herE:

::: layout/generic/nsFlexContainerFrame.cpp:902
(Diff revisions 2 - 4)
>      mLastBaselineOffset(nscoord_MIN),
>      mMainGapSize(aMainGapSize)
>    {}
>  
> -  // Returns the sum of our FlexItems' outer hypothetical main sizes.
> +  nscoord GetSumOfGaps() const {
> +    return mNumItems <= 0 ? 0 : (mNumItems - 1) * mMainGapSize;

mNumItems is unsigned, so the "less than" part of the "<=0" comparison feels a bit bizarre.

I see two easy options to address this; take your pick:

 (1) realistically, you could just make this "!=" 0 and it'd be fine.
 (2) Though if you want to be robust against hypothetical-future-refactorings-of-this-variable-to-allow-negative-values [which I'm guessing is where you were coming from with the <= 0 comparison], then it'd perhaps be clearer to just make this "mNumItems > 0 ?"  (or >=1) and swap the order of the ternary expression's cases.  That's equivalently robust to what you've got, without being as visibly awkward RE checking if an unsigned value is less than 0. :)

(To be clear, I don't think we'd do any sort of refactoring-to-allow-negative-values for this variable -- this is a count, which is by definition >=0 -- so the above options are equivalently fine from my perspective.)
Attachment #8987637 - Flags: review?(dholbert) → review+
When you think this is ready to land, just add "checkin-needed" to the "keyword" field (in the "tracking" section at the top of this bug), and someone will come along and trigger autoland for it (within a couple hours, typically).

Alternately, feel free to just ping me and I can trigger autoland, too.
Flags: needinfo?(miyer)
Flags: needinfo?(miyer)
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c1bb0f3ff98
Implement column and row gap for flexbox. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c1bb0f3ff98
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1472533
Really exciting to see this happening, I have some questions about this patch
(I'm checking this in Firefox Nightly 63.0a1 (2018-07-02)).

1) Should the intrinsic height of the flex container be affected by the row-gap.
   E.g.:
     <div style="display: flex; border: solid thick; font: 50px/1 Monospace;
                 row-gap: 50px; width: 100px;">
       <div style="background: cyan;">i1</div>
       <div style="background: magenta;">i2</div>
     </div>

   Here the height of the flex container is 100px, so the 2nd item "i2" overflows it.
   Shouldn't it be 150px so it includes the row-gap too?

2) Percentage row gaps and indefinite heights.
   E.g.

     <div style="display: flex; border: solid thick; font: 50px/1 Monospace;
                 row-gap: 50%; flex-direction: column;">
       <div style="background: cyan;">i1</div>
       <div style="background: magenta;">i2</div>
     </div>

   The spec says (https://drafts.csswg.org/css-align/#column-row-gap):
   "percentages resolve against zero for determining intrinsic size contributions,
    but resolve against the box’s content box when laying out the box’s contents."

   So in this case, the intrinsic height of the flex container should be 100px,
   and then the 50% row gap should be resolved against 100px during layout,
   so it'd be a 50px gap.
   However the height of the flex container shouldn't change, it should be still 100px
   and we'll have overflow (the output would be similar to point 1)).
Depends on: 1473044
Blocks: 1473044
No longer depends on: 1473044
Thanks for the feedback!

(In reply to Manuel Rego Casasnovas from comment #31)
> 1) Should the intrinsic height of the flex container be affected by the
> row-gap.

I agree with you, if we make that example multi-line (adding flex-wrap:wrap) so that there are multiple rows between which to put row gaps. Sample with that change (and a larger row-gap for emphasis): https://jsfiddle.net/6kwz1m58/1

> 2) Percentage row gaps and indefinite heights.

Hmm, it looks like you're right here as well -- this is analogous to Mats' "css grid is awesome" testcase from this comment:
 https://github.com/w3c/csswg-drafts/issues/509#issuecomment-386332974

(Link to that live grid testcase: https://jsfiddle.net/aLx0sv3p/ )

In Firefox Nightly, we render that testcase with gaps (which cause overflow), as shown in his screenshot there, so we should probably do the same in flexbox as well.  (I notice Chrome doesn't show any gaps in that grid testcase - do you know if that's changing soon?)
Flags: needinfo?(rego)
(I filed bug 1473044 and bug 1473047 for rego's two points here.)
(In reply to Daniel Holbert [:dholbert] from comment #32)
> In Firefox Nightly, we render that testcase with gaps (which cause
> overflow), as shown in his screenshot there, so we should probably do the
> same in flexbox as well.  (I notice Chrome doesn't show any gaps in that
> grid testcase - do you know if that's changing soon?)

Yes, we're working on that, but we'd like to switch both percentage row tracks
and percentage row gutters behavior at the same time:
* https://github.com/w3c/csswg-drafts/issues/509
* https://github.com/w3c/csswg-drafts/issues/1921

As doing it only for percentage row gutters like Firefox seems kind of strange,
the behavior of percentages would be quite different comparing gutters and tracks.
Mats seems supportive about issue #1921 too, so I guess he'd update Firefox
on that regard too.
Flags: needinfo?(rego)
Keywords: site-compat
(In reply to Rachel Andrew from comment #35)
> I've been working on the documentation for this issue:
> 
> Updated:
> https://developer.mozilla.org/en-US/docs/Web/CSS/gap
> https://developer.mozilla.org/en-US/docs/Web/CSS/row-gap
> https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap
> 
> Also updated the compat data which is on the tables on those pages.

Looks good!

> 
> Added a note about the properties to:
> https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Alignment/
> Box_Alignment_in_Flexbox#The_gap_properties

I made a tiny tweak: s/gaps on either side of the item/gaps between adjacent items/, to make it clearer that there are no gaps at start/end.

> Minor updates to:
> https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/
> https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Aligning_Items_in_a_Flex_Container

Looks good!
Flags: needinfo?(iyermihir)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: