Last Comment Bug 939901 - Support multi-line flexbox in layout
: Support multi-line flexbox in layout
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
P3 normal with 12 votes (vote)
: mozilla28
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css3-flexbox/...
Depends on: css3-flexbox 702508 939896
Blocks: css3test 995020 946269 946835
  Show dependency treegraph
 
Reported: 2013-11-18 10:41 PST by Daniel Holbert [:dholbert]
Modified: 2014-04-12 11:15 PDT (History)
102 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
28+


Attachments
wip rollup patch (29.28 KB, patch)
2013-11-26 15:32 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 1: Honor 'wrap-reverse' flipping the cross axis, in FlexboxAxisTracker (3.88 KB, patch)
2013-11-26 15:46 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 2: Convert GenerateFlexItems into GenerateFlexLines (7.60 KB, patch)
2013-11-26 16:01 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 2 v2: Convert GenerateFlexItems into GenerateFlexLines [r=mats] (7.61 KB, patch)
2013-11-26 17:10 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
part 2 v3: Convert GenerateFlexItems into GenerateFlexLines [r=mats] (7.61 KB, patch)
2013-11-26 17:19 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
part 3: When generating FlexLines, wrap at page-breaks and at max-main-size (11.77 KB, patch)
2013-12-03 15:26 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 4: Loop over the array of FlexLines (26.39 KB, patch)
2013-12-03 15:55 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 4 ("diff -w" version) (15.76 KB, patch)
2013-12-03 16:03 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 5: honor "align-content", using CrossAxisPositionTracker (11.26 KB, patch)
2013-12-03 17:11 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 6: Update nsFlexContainerFrame::GetMinWidth (intrinsic min-width function) to recognize that multi-line lets us be skinnier (1.38 KB, patch)
2013-12-03 17:52 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 3.5: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after" (2.48 KB, patch)
2013-12-04 10:14 PST, Daniel Holbert [:dholbert]
mats: review-
Details | Diff | Splinter Review
reftests patch (not quite done) (84.40 KB, patch)
2013-12-04 10:25 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 3.5 v1 again, oops [disregard] (2.48 KB, patch)
2013-12-04 21:19 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 3.5 v2: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after" (3.62 KB, patch)
2013-12-04 21:22 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 3.5 v3: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after" (3.62 KB, patch)
2013-12-04 21:25 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 7: Improve flex container baseline calculation from first FlexLine (7.69 KB, patch)
2013-12-04 23:52 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
part 8: reftests (123.89 KB, patch)
2013-12-04 23:58 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
overflow:hidden interfering with flex-wrap:wrap (5.55 KB, text/html)
2013-12-10 13:51 PST, thinsoldier
no flags Details

Description User image Daniel Holbert [:dholbert] 2013-11-18 10:41:45 PST
+++ This bug was initially created as a clone of Bug #702508 +++

Filing this bug on implementing the layout parts of multi-line flexbox. (as opposed to the purely css parsing/computation parts, which are implemented in bug 702508.)

This will layer on top of bug 939896, which refactors the flexbox code to create a FlexLine helper-class.
Comment 1 User image Daniel Holbert [:dholbert] 2013-11-18 10:43:00 PST
[oops, forgot to update summary when cloning bug; fixed now]
Comment 2 User image Daniel Holbert [:dholbert] 2013-11-26 15:32:42 PST
Created attachment 8338884 [details] [diff] [review]
wip rollup patch

(for reference, here's a rollup of the code I have so far. I think this is close to complete, though I haven't written reftests yet so it's probably missing an edge case here or there. However, this passes Opera's contributed multi-line reftests in the csswg testsuite, here:
  http://test.csswg.org/source/contributors/opera/submitted/css3-flexbox/
I tried all the tests with "align-content", "wrap", and "flow" in the names, and the testcases all rendered like the references, to my eye.)
Comment 3 User image Jonathan Watt [:jwatt] 2013-11-26 15:40:24 PST
Sweet! :)
Comment 4 User image Daniel Holbert [:dholbert] 2013-11-26 15:46:01 PST
Created attachment 8338895 [details] [diff] [review]
part 1: Honor 'wrap-reverse' flipping the cross axis, in FlexboxAxisTracker

This patch adjusts the FlexboxAxisTracker to honor the cross-axis reversal that "flex-wrap: wrap-reverse" triggers.
   http://dev.w3.org/csswg/css-flexbox/#valuedef-wrap-reverse

This means now we can actually have bottom-to-top cross axis, so this patch also removes some assertions and comments about bottom-to-top being impossible.
Comment 5 User image Daniel Holbert [:dholbert] 2013-11-26 16:01:28 PST
Created attachment 8338898 [details] [diff] [review]
part 2: Convert GenerateFlexItems into GenerateFlexLines

This patch makes us generate an array of flex lines, per this chunk of the flexbox spec:
 http://dev.w3.org/csswg/css-flexbox/#algo-line-break

For now, this just makes the rest of the flexbox algorithm work with lines[0], so that I don't have to mess with the rest of the code yet. Subsequent patches will make us actually walk over the lines.
Comment 6 User image David 2013-11-26 16:16:15 PST
Well this is very exciting! Great to see movement on it; thanks for your hard work
Comment 7 User image Mats Palmgren (:mats) 2013-11-26 16:35:35 PST
Comment on attachment 8338898 [details] [diff] [review]
part 2: Convert GenerateFlexItems into GenerateFlexLines

r=mats

>+  nsTArray<FlexLine> lines;

Why not nsAutoTArray?
Comment 8 User image Daniel Holbert [:dholbert] 2013-11-26 17:10:09 PST
Created attachment 8338954 [details] [diff] [review]
part 2 v2: Convert GenerateFlexItems into GenerateFlexLines [r=mats]

(In reply to Mats Palmgren (:mats) from comment #7)
> Why not nsAutoTArray?

Because I forgot that such a thing existed. :)

Thanks for the suggestion - switched to nsAutoTArray<FlexLine, 1> in this version. ('1' since single-line is the common & default case).
Comment 9 User image Daniel Holbert [:dholbert] 2013-11-26 17:19:19 PST
Created attachment 8338958 [details] [diff] [review]
part 2 v3: Convert GenerateFlexItems into GenerateFlexLines [r=mats]

(Sorry, forgot to qref before posting previous version, so it had nsAutoTArray<FlexLine> which doesn't compile. This version adds the second template-arg, making it nsAutoTArray<FlexLine, 1>.)
Comment 10 User image Daniel Holbert [:dholbert] 2013-12-03 15:26:39 PST
Created attachment 8342011 [details] [diff] [review]
part 3: When generating FlexLines, wrap at page-breaks and at max-main-size

This patch extends GenerateFlexLines (which was introduced in the previous patch) to make it a bit smarter.

In particular, this makes GenerateFlexLines...
 a) ...take the container's main-size (obtained from the reflow state) as a parameter, instead of figuring it out on its own. This just keeps us from needing to figure out this value independently in two different places. (Note: this is before any child/availableHeight-dependent tweaks to the main size have been resolved yet, because we haven't even generated any FlexItems yet.)

 b) ...take the available height, so that (in a vertical flex container) we can wrap to a new (vertical) FlexLine when we run out of availableHeight, instead of just continuing the current line and running off the page.

 c) ...wrap at the max main-size (e.g. "max-height"), so that we don't let our FlexLines grow indefinitely when the container has "height:auto; max-height: 100px".


ALSO: in support of (a), this patch:
 d) splits ComputeFlexContainerMainSize into two helper-methods:
    - GetFlexContainerMainSizeFromReflowState(), which just gets our computed main-size, but doesn't do any pagination/child-shrinkwrapping stuff
    - ClampFlexContainerMainSize(), which does the rest (the pagination/child-shrinkwrapping).
(Note that the former helper-method there has to be a nsFlexContainerFrame method, since it calls GetEffectiveComputedHeight which is protected. The latter is just a file-scope static helper-method, though.)
Comment 11 User image Daniel Holbert [:dholbert] 2013-12-03 15:55:56 PST
Created attachment 8342023 [details] [diff] [review]
part 4: Loop over the array of FlexLines

Recall that part 2 has:
>+  FlexLine& line = lines[0]; // XXXdholbert Temporary; a later patch will
>+                             // rewrite the relevant code to loop over |lines|
>+                             // where appropriate.

This patch here is that "later patch" which drops this dummy |line| and makes us actually use |lines| everywhere.

A lot of this is indentation-changes (due to adding loops), so I'll attach a "diff -w" version for review purposes.
Comment 12 User image Daniel Holbert [:dholbert] 2013-12-03 16:03:47 PST
Created attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)
Comment 13 User image Daniel Holbert [:dholbert] 2013-12-03 17:11:01 PST
Created attachment 8342060 [details] [diff] [review]
part 5: honor "align-content", using CrossAxisPositionTracker

This patch fleshes out the (useless until now) class "CrossAxisPositionTracker".

This class figures out how much packing space we have to distribute among our lines (or to use as padding before/after/between our lines), according to "align-content"...
  http://dev.w3.org/csswg/css-flexbox/#align-content-property
...and then manages the distribution of it, via its public methods (which we call in Reflow).

As noted in a code-comment: the code here is very similar to the existing MainAxisPositionTracker class, so it might be handy to refer to that code when reviewing this.
Comment 14 User image Daniel Holbert [:dholbert] 2013-12-03 17:16:31 PST
(Note that "mNumPackingSpacesRemaining" is the key variable that determines whether TraversePackingSpace() is a no-op or not.  And we only set mNumPackingSpacesRemaining for "align-content:space-between" and "align-content:space-around", which are the only align-content values that allow packing space between the lines.

For the other align-content values, we just set the correct starting position for the first flex line, and we allow TraversePackingSpace() to be a no-op. (And stretch the lines, in the case of 'stretch'.))
Comment 15 User image Daniel Holbert [:dholbert] 2013-12-03 17:52:19 PST
Created attachment 8342076 [details] [diff] [review]
part 6: Update nsFlexContainerFrame::GetMinWidth (intrinsic min-width function) to recognize that multi-line lets us be skinnier

This updates GetMinWidth (the intrinsic min-width function) to account for multi-line horizontal flex container.  (We can allow them to take all of the line-wrap opportunities, which produces a smaller min-width than a single-line horizontal flex container.)

(In case it's not in your recent memory: GetMinWidth is a function that's called *before* we're reflowed, to negotiate our computed width -- e.g. for use in table-cell width balancing, if this is inside of a table.)
Comment 16 User image Mats Palmgren (:mats) 2013-12-04 07:46:13 PST
Comment on attachment 8342011 [details] [diff] [review]
part 3: When generating FlexLines, wrap at page-breaks and at max-main-size

r=mats, with some minor nits resolved as you see fit.

>layout/generic/nsFlexContainerFrame.cpp
>-nsFlexContainerFrame::ComputeFlexContainerMainSize(
>+nsFlexContainerFrame::GetFlexContainerMainSizeFromReflowState(

Does "FlexContainer" add any value in this method's name?
How about just GetMainSizeFromReflowState?
(and ComputeCrossSize)

>+// Adjusts the content-box main-size of our flex container based on the

Perhaps s/Adjusts/Return/ ? (since Adjusts might suggest that the flex
container frame is mutated in some way)

>+ClampFlexContainerMainSize(const nsHTMLReflowState& aReflowState,
>+                           const FlexboxAxisTracker& aAxisTracker,
>+                           nscoord aUnclampedMainSize,
>+                           nscoord aAvailableHeightForContent,

I think I'd prefer to remove the ForContent suffix, since that's the
name we use in other parts of Layout.  I don't think we have other
kinds of available height (or width), but the suffix introduces an
uncertainty about that.


>layout/generic/nsFlexContainerFrame.h
>   nsresult GenerateFlexLines(nsPresContext* aPresContext,
>                              const nsHTMLReflowState& aReflowState,
>+                             nscoord aContentBoxMainSize,
>+                             nscoord aAvailableHeightForContent,

Same comment as above on the ForContent suffix.
Comment 17 User image Daniel Holbert [:dholbert] 2013-12-04 07:56:20 PST
Thanks for the review!

(In reply to Mats Palmgren (:mats) from comment #16)
> >layout/generic/nsFlexContainerFrame.cpp
> >-nsFlexContainerFrame::ComputeFlexContainerMainSize(
> >+nsFlexContainerFrame::GetFlexContainerMainSizeFromReflowState(
> 
> Does "FlexContainer" add any value in this method's name?
> How about just GetMainSizeFromReflowState?
> (and ComputeCrossSize)

Sure - sounds good.


> >+// Adjusts the content-box main-size of our flex container based on the
> 
> Perhaps s/Adjusts/Return/ ? (since Adjusts might suggest that the flex
> container frame is mutated in some way)

Good call. (I initially had "Adjusts" because I was thinking of adjusting an in/out-param.)

> I think I'd prefer to remove the ForContent suffix, since that's the
> name we use in other parts of Layout.  I don't think we have other
> kinds of available height (or width)
[...]
> Same comment as above on the ForContent suffix.

The "for content" label here means "for the content box of the flex container" -- i.e. we've already subtracted out space for the container's top border/padding, if appropriate. (This distinguishes it from aReflowState.availableHeight, which the available height that the flex container's parent is giving it, from the top of the flex container's border-box.)

Bug 811024 added a named variable in several places to nsFlexContainer::Reflow, one of which is passed in as this parameter, so for consistency with those variables' names, I'll keep these ones labeled "ForContent" for now. If you feel strongly that we should drop/improve this naming, we can rename all of the copies of this variable in a followup.
Comment 18 User image Mats Palmgren (:mats) 2013-12-04 08:12:02 PST
I see, so ForChildren is perhaps a better suffix in that case, though I'm not sure
it's needed for disambiguation.  But leave it for now, I'll take another look.

BTW, I think it would be good to add some flexbox tests that use -moz-box-sizing:border-box
(here or in a separate bug) to sanity check we're doing the calculations correctly.
Comment 19 User image Daniel Holbert [:dholbert] 2013-12-04 08:23:25 PST
(In reply to Mats Palmgren (:mats) from comment #18)
> BTW, I think it would be good to add some flexbox tests that use
> -moz-box-sizing:border-box
> (here or in a separate bug) to sanity check we're doing the calculations
> correctly.

Filed bug 946273.

(Also: I filed bug 946269 on the ComputeCrossSize renaming that you suggested in comment 16, since that's a preexisting method and I don't want to mix in a simple rename with the rest of what this bug's patches are doing. I did rename GetFlexContainerMainSizeFromReflowState (which is added in this bug) in my local copy of the patch, though.)
Comment 20 User image Daniel Holbert [:dholbert] 2013-12-04 10:14:07 PST
Created attachment 8342500 [details] [diff] [review]
part 3.5: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after"

This patch gives us one more trigger to wrap to a new Flex Line -- the "page-break-before" / "page-break-after" properties (which end up influencing nsStyleDisplay::mBreakBefore/mBreakAfter).

This is as directed by the spec. Quote:
  # 5. Collect flex items into flex lines:
  # [...] . A break is forced wherever the CSS2.1
  # page-break-before/page-break-after [CSS21] or
  # the CSS3 break-before/break-after [CSS3-BREAK]
  # properties specify a fragmentation break.
http://dev.w3.org/csswg/css-flexbox/#algo-line-break

(Note that we don't yet support the 'break-before' / 'break-after' properties - we only have the 'page' versions.)
Comment 21 User image Daniel Holbert [:dholbert] 2013-12-04 10:25:23 PST
Created attachment 8342505 [details] [diff] [review]
reftests patch (not quite done)

Here's the patch of reftests that I've got so far, FWIW.

Per the XXXdholbert comment that this adds in layout/reftests/w3c-css/submitted/flexbox/reftest.list, I'd still like to add some baseline-alignment reftests for multi-line flex containers. But other than that, I think this is reasonably complete.

Feel free to preemptively review the current contents, if you get through the other patches here and want to get a head-start on this one. :)
Comment 22 User image Mats Palmgren (:mats) 2013-12-04 16:12:41 PST
Comment on attachment 8342011 [details] [diff] [review]
part 3: When generating FlexLines, wrap at page-breaks and at max-main-size

I agree the suffix adds clarity, so please disregard my comment on that.
Comment 23 User image Mats Palmgren (:mats) 2013-12-04 16:22:25 PST
Comment on attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)

>+nscoord
>+SumLineCrossSizes(const nsTArray<FlexLine>& aLines)

static?
Comment 24 User image Mats Palmgren (:mats) 2013-12-04 18:11:07 PST
Comment on attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)

>   // Calculate the cross size and (if necessary) baseline-alignment position
>   // for our (single) flex line:
>-  line.ComputeCrossSizeAndBaseline(axisTracker);
>+  for (uint32_t lineIdx = 0; lineIdx < lines.Length(); ++lineIdx) {
>+    lines[lineIdx].ComputeCrossSizeAndBaseline(axisTracker);
>+  }

Please update the comment also.
Comment 25 User image Mats Palmgren (:mats) 2013-12-04 18:40:32 PST
Comment on attachment 8342060 [details] [diff] [review]
part 5: honor "align-content", using CrossAxisPositionTracker

>layout/generic/nsFlexContainerFrame.cpp
>+CrossAxisPositionTracker::
>+  CrossAxisPositionTracker(nsTArray<FlexLine>& aLines,
...
>+    mPackingSpaceRemaining(aContentBoxCrossSize), // we chip away at this below

It might be clearer if you initialize it to zero here,
remove the zero-assignment in the first if-statement, and
add "mPackingSpaceRemaining = aContentBoxCrossSize;" just
before the loop.  Then you can remove that part in the
comment before the loop.

>+      case NS_STYLE_ALIGN_CONTENT_SPACE_AROUND: {
...
>+        MOZ_ASSERT(!aLines.IsEmpty(), "We should have at least 1 line");

This assertion seems like it would be better to have as the
first line in this method.  It's a general invariant, right?
(I'm assuming you use infallible allocations in this code)


>+  if (mPackingSpaceRemaining != 0) {
...
>+      case NS_STYLE_ALIGN_CONTENT_STRETCH:
>+        if (mPackingSpaceRemaining == 0) {
>+          break; // bail early; no space to distribute
>+        }

Looks like that can't happen.


>+  CrossAxisPositionTracker
>+    crossAxisPosnTracker(lines,
>+                         aReflowState.mStylePosition->mAlignContent,
>+                         contentBoxCrossSize, isCrossSizeDefinite, axisTracker);

Put the mAlignContent arg next to 'lines'?
Comment 26 User image Mats Palmgren (:mats) 2013-12-04 20:09:02 PST
Comment on attachment 8342500 [details] [diff] [review]
part 3.5: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after"

It looks like 9.3.5 says we shouldn't honor them in the single-line case:
http://dev.w3.org/csswg/css-flexbox/#main-sizing

It might be easier to see what GenerateFlexLines is doing (in part 2)
if it started with:
const bool isSingleLine =
  NS_STYLE_FLEX_WRAP_NOWRAP == aReflowState.mStylePosition->mFlexWrap;

and then:
  if (isSingleLine) {
    // don't assign wrapThreshold here

and later in the loop:
-if (wrapThreshold != NS_UNCONSTRAINEDSIZE &&
+if (!isSingleLine &&

and add "!isSingleLine" to the conditions for the forced breaks.

AFAICT from the spec, it's only 'flex-wrap:nowrap' that are single-line,
a (wrap*) flexbox with unconstrained main-axis dimension still counts
as multi-line:  http://dev.w3.org/csswg/css-flexbox/#propdef-flex-wrap
(in case GET_MAIN_COMPONENT returns NS_UNCONSTRAINEDSIZE)

Does that make sense?
Comment 27 User image Daniel Holbert [:dholbert] 2013-12-04 21:19:11 PST
(In reply to Mats Palmgren (:mats) from comment #23)
> Comment on attachment 8342030 [details] [diff] [review]
> part 4 ("diff -w" version)
> 
> >+nscoord
> >+SumLineCrossSizes(const nsTArray<FlexLine>& aLines)
> 
> static?

Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/583c18eb7da6

(In reply to Mats Palmgren (:mats) from comment #24)
> Please update the comment also.

Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/b0d5b61ccf8d

(In reply to Mats Palmgren (:mats) from comment #25)
> Comment on attachment 8342060 [details] [diff] [review]
> part 5: honor "align-content", using CrossAxisPositionTracker

Addressed review comments in https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/2f5af7c82177

(In reply to Mats Palmgren (:mats) from comment #26)
> Comment on attachment 8342500 [details] [diff] [review]
> part 3.5: When generating FlexLines, wrap lines as directed by
> "page-break-before"/"page-break-after"
> 
> It looks like 9.3.5 says we shouldn't honor them in the single-line case:
> http://dev.w3.org/csswg/css-flexbox/#main-sizing

Yikes, good point! I'll add a test for that, and I'll post a fixed version of this patch shortly, with the suggested isSingleLine bool.
Comment 28 User image Daniel Holbert [:dholbert] 2013-12-04 21:19:58 PST
Created attachment 8342863 [details] [diff] [review]
part 3.5 v1 again, oops [disregard]

Updated per comment 26.
Comment 29 User image Daniel Holbert [:dholbert] 2013-12-04 21:22:18 PST
Created attachment 8342864 [details] [diff] [review]
part 3.5 v2: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after"

Updated per comment 26 (for real now; forgot to qref before)
Comment 30 User image Daniel Holbert [:dholbert] 2013-12-04 21:25:11 PST
Created attachment 8342866 [details] [diff] [review]
part 3.5 v3: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after"

(made a minor tweak to condense a boolean expression to 2 lines instead of 3)
Comment 31 User image Daniel Holbert [:dholbert] 2013-12-04 23:52:56 PST
Created attachment 8342915 [details] [diff] [review]
part 7: Improve flex container baseline calculation from first FlexLine

This patch (final code patch here, I think) fixes a bug with baseline calculation in wrap-reverse flex containers, and wrapped flex containers whose first line isn't all the way at the top (e.g. due to align-content: center)

Specifically, this patch...
 a) Refactors out some (simple) existing code into a helper-method "PhysicalPosFromLogicalPos"

 b) For clarity/consistency, renames PhysicalPositionFromLogicalPosition (which the above method was refactored out of) with s/Position/Point/, both for brevity and for naming-consistency. (We frequently talk about the "main-axis position" vs "cross-axis position", which are one-dimensional values, so it's nicer to rename this two-dimensional method to use the more-clearly two-dimensional term "point", particularly given that it returns a nsPoint.)

 c) (the interesting part) Uses the new method from (a) to assist in determining the flex container's ascent from its first flex line. (In particular: we need to add the line's position within the container to its baseline-offset, and then if it's bottom-to-top, we need to flip the polarity (which that helper-function manages). Previously we weren't doing either of those things.)
Comment 32 User image Daniel Holbert [:dholbert] 2013-12-04 23:58:52 PST
Created attachment 8342919 [details] [diff] [review]
part 8: reftests

Here are the reftests I've got right now. I think they're pretty comprehensive. The things I'd still like to write tests are:
 - Baseline alignment of items on multiple lines, simultaneously, in a multi-line flex container (each line calculating its own baseline, independently)

 - Computing the baseline of a vertical flex container (single and multi-line) (We already have tests for computing the baseline of a horizontal single-line flex container, and this patch adds a test for computing the baseline of a horizontal multi-line flex container).

I'm pretty sure both of those situations work; I just haven't gotten to writing tests for them yet because baseline-related tests can be annoying to write references for. I intend to either add those tests before landing, or file a followup bug for them, though.
Comment 33 User image Mats Palmgren (:mats) 2013-12-05 07:57:01 PST
Comment on attachment 8342919 [details] [diff] [review]
part 8: reftests

r=mats
Comment 34 User image Daniel Holbert [:dholbert] 2013-12-05 08:01:32 PST
Thanks for the reviews!

Try run with the current reftests patch, FWIW: https://tbpl.mozilla.org/?tree=Try&rev=b4f56ef17bef

The reftest orange there is from just one part of one (new) test -- flexbox-baseline-multi-line-horiz-2.html -- which has an off-by-1px baseline-alignment difference on some platforms. (not due to a bug; just due to my reference case being too simple) The only mismatch is in the final flex item in that testcase (the one where the flex container derives its own baseline from its first flex line's negotiated baseline-alignment position.)

I'm going to split that tricky part of that test into its own test, to so I can keep the rest of that test relatively simple, and make a more targeted slightly-more-complex reference for the tricky bit.
Comment 35 User image Daniel Holbert [:dholbert] 2013-12-05 08:37:42 PST
Try run with comment 34's tricky part removed from flexbox-baseline-multi-line-horiz-* (so that those tests just focus on testing the piece of the spec that they quote -- getting a baseline when there are no baseline-aligned items in the first flex line):  https://tbpl.mozilla.org/?tree=Try&rev=8582649e5d4e
Comment 36 User image Daniel Holbert [:dholbert] 2013-12-05 11:10:28 PST
[I filed bug 946835 on writing a few additional tests that I'd like for this, per comment 32 through comment 34.]

Comment 35's Try Run is looking good -- it has a green reftest run on Mac (which was one of the orange platforms from the previous try run). That (combined with the fact that I took out the problematic bit causing orange, and am folding that into bug 946835) makes me confident that this will be green on TBPL. (But I'll keep watching as more jobs on the Try run complete, in case of weirdness on any other platforms.)

Hence, I landed the patches here and the other helper-bugs for multi-line flexbox, on inbound:
part 1:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80fc9290af34
part 2:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e4bebee995b4
part 3:   https://hg.mozilla.org/integration/mozilla-inbound/rev/54a3dd704f9a
part 3.5: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ffb0aa5121f
part 4:   https://hg.mozilla.org/integration/mozilla-inbound/rev/79a76a1dce4e
part 5:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47901ad6d20b
part 6:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9afb8ec2269
part 7:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e621918f8bf
part 8:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a963c41576

(It should be merged from there to mozilla-central in the next day or so, and after that, it'll appear in the subsequent Nightly build.)

Looks like this is making it for Firefox 28, barring unforseen issues.
Comment 37 User image Daniel Holbert [:dholbert] 2013-12-06 01:44:03 PST
Just landed two small followups to fix some typos in metadata within the reftests (no effect on their rendering):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/64240496195e
 https://hg.mozilla.org/integration/mozilla-inbound/rev/c491f36ee82c
Comment 40 User image Florian Bender 2013-12-06 12:46:53 PST
Great stuff! Fx 28 is going to be a huge release! Thanks a lot, guys! :)

(Docs need to be updated that we finally support all of flexbox -> flag.)
Comment 41 User image Daniel Holbert [:dholbert] 2013-12-06 13:02:23 PST
Thanks!

[Dropping dev-doc-needed, since bugs 702508 and bug 939905 cover all of the dev-doccable stuff here, and they're already dev-doc-needed.]
Comment 42 User image Daniel Holbert [:dholbert] 2013-12-06 13:06:01 PST
[sorry, "bug 702508" (not 'bugs')]
(This bug here was just a gecko-internals helper-bug for bug 702508, to limit the patches/comments/work required for that other bug.  Feel free to add back dev-doc-needed if you really think something here needs doccing, though (in which case, apologies for stomping on it))
Comment 43 User image Florian Bender 2013-12-06 14:42:55 PST
Ah, that's alright. Wasn't sure if this bug had additional (externally exposed) stuff – seems like I added the wrong bug to the dependency list of Bug 666041. Anyway, that's all fixed now!
Comment 44 User image Florian Bender 2013-12-06 14:53:37 PST
(Meant Bug 913153, of course. No more bugspam now, sorry!)
Comment 45 User image thinsoldier 2013-12-09 07:34:27 PST
I'm probably using flexbox horribly incorrectly, but it seems either Firefox or Chrome has a bug related to flex-wrap:

http://imgur.com/NsywXJK
Comment 46 User image Daniel Holbert [:dholbert] 2013-12-09 10:01:15 PST
@thinsoldier, could you file a new bug with an attachment that demonstrates the difference? (and either CC me on the bug or mention the bug number here)

Regardless of which browser is correct / incorrect, I'd like to dig in a bit and see what's going on there.
Comment 47 User image Daniel Holbert [:dholbert] 2013-12-09 10:02:07 PST
(sorry -- and by "an attachment that demonstrates the difference", I mean an HTML file instead of a screenshot of renderings + CSS snippets :))
Comment 48 User image thinsoldier 2013-12-10 13:51:58 PST
Created attachment 8345511 [details]
overflow:hidden interfering with flex-wrap:wrap

My apologies. I did not realize my page was using html4 loose doctype. I probably should not have expected it to work.

But since it still does not work with an html5 doctype in Fx but does work in chrome I put together a simplified page so you can see.

It seems overflow needs to be set to visible or omitted completely to have it work in Fx.
Comment 49 User image Daniel Holbert [:dholbert] 2013-12-10 14:06:08 PST
As noted in Comment 46, it's best to file new bugs for issues with a feature, rather than posting attachments on the bugpage for the feature. Otherwise, bug pages for new features get unmanageable very quickly.

Anyway, I filed bug 948654 for comment 48.
Comment 50 User image Daniel Holbert [:dholbert] 2013-12-10 14:07:05 PST
(and thanks for the report! I hope to dig into it soon & see what's going on.)
Comment 51 User image diegobfernandez 2013-12-17 15:37:23 PST
I think I found a bug regarding using box-sizing and flexbox.
But since I don't know the expected results I'll ask here before filling a bug.

So, it seems that setting box-sizing: border: box in a display: flex has no effect?
Does anyone know anything about it?
Comment 52 User image Daniel Holbert [:dholbert] 2013-12-17 15:45:05 PST
(In reply to diegobfernandez from comment #51)
> So, it seems that setting box-sizing: border: box in a display: flex has no
> effect?
> Does anyone know anything about it?

I would believe that. I've been meaning to add some tests and see how it behaves in bug 946273, so that bug sort of covers this issue.
Comment 53 User image Daniel Holbert [:dholbert] 2014-02-11 21:12:27 PST
*** Bug 971145 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.