Support multi-line flexbox in layout

RESOLVED FIXED in mozilla28

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 28+)

Details

(URL)

Attachments

(12 attachments, 6 obsolete attachments)

29.28 KB, patch
Details | Diff | Splinter Review
3.88 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
7.61 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.77 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
26.39 KB, patch
Details | Diff | Splinter Review
15.76 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
11.26 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
1.38 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
3.62 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
7.69 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
123.89 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
5.55 KB, text/html
Details
(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Comment 1

4 years ago
[oops, forgot to update summary when cloning bug; fixed now]
Summary: Support parsing/computing multi-line flexbox properties "flex-wrap", "align-content" → Support multi-line flexbox in layout
(Assignee)

Updated

4 years ago
No longer depends on: 696253
(Assignee)

Updated

4 years ago
No longer blocks: 836881
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Component: CSS Parsing and Computation → Layout
Keywords: dev-doc-needed

Updated

4 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 2

4 years ago
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.)
Sweet! :)
(Assignee)

Comment 4

4 years ago
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.
Attachment #8338895 - Flags: review?(matspal)
(Assignee)

Comment 5

4 years ago
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.
Attachment #8338898 - Flags: review?(matspal)
Attachment #8338895 - Flags: review?(matspal) → review+

Comment 6

4 years ago
Well this is very exciting! Great to see movement on it; thanks for your hard work
Comment on attachment 8338898 [details] [diff] [review]
part 2: Convert GenerateFlexItems into GenerateFlexLines

r=mats

>+  nsTArray<FlexLine> lines;

Why not nsAutoTArray?
Attachment #8338898 - Flags: review?(matspal) → review+
(Assignee)

Comment 8

4 years ago
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).
Attachment #8338898 - Attachment is obsolete: true
Attachment #8338954 - Flags: review+
(Assignee)

Comment 9

4 years ago
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>.)
Attachment #8338954 - Attachment is obsolete: true
Attachment #8338958 - Flags: review+
(Assignee)

Comment 10

4 years ago
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.)
Attachment #8342011 - Flags: review?(matspal)
(Assignee)

Comment 11

4 years ago
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.
(Assignee)

Comment 12

4 years ago
Created attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)
Attachment #8342030 - Flags: review?(matspal)
(Assignee)

Comment 13

4 years ago
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.
Attachment #8342060 - Flags: review?(matspal)
(Assignee)

Comment 14

4 years ago
(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'.))
(Assignee)

Comment 15

4 years ago
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.)
Attachment #8342076 - Flags: review?(matspal)
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.
Attachment #8342011 - Flags: review?(matspal) → review+
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 946269
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.
(Assignee)

Comment 19

4 years ago
(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.)
(Assignee)

Comment 20

4 years ago
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.)
Attachment #8342500 - Flags: review?(matspal)
(Assignee)

Comment 21

4 years ago
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 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 on attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)

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

static?
Attachment #8342030 - Flags: review?(matspal) → review+
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 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'?
Attachment #8342060 - Flags: review?(matspal) → review+
Attachment #8342076 - Flags: review?(matspal) → review+
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?
Attachment #8342500 - Flags: review?(matspal) → review-
(Assignee)

Comment 27

4 years ago
(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.
(Assignee)

Comment 28

4 years ago
Created attachment 8342863 [details] [diff] [review]
part 3.5 v1 again, oops [disregard]

Updated per comment 26.
Attachment #8342500 - Attachment is obsolete: true
Attachment #8342863 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #8342863 - Attachment description: part 3.5 v2: When generating FlexLines, wrap lines as directed by "page-break-before"/"page-break-after" → part 3.5 v1 again, oops
Attachment #8342863 - Attachment is obsolete: true
Attachment #8342863 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #8342863 - Attachment description: part 3.5 v1 again, oops → part 3.5 v1 again, oops [disregard]
(Assignee)

Comment 29

4 years ago
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)
Attachment #8342864 - Flags: review?(matspal)
(Assignee)

Comment 30

4 years ago
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)
Attachment #8342864 - Attachment is obsolete: true
Attachment #8342864 - Flags: review?(matspal)
Attachment #8342866 - Flags: review?(matspal)
(Assignee)

Comment 31

4 years ago
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.)
Attachment #8342915 - Flags: review?(matspal)
(Assignee)

Comment 32

4 years ago
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.
Attachment #8342505 - Attachment is obsolete: true
Attachment #8342919 - Flags: review?(matspal)
Attachment #8342866 - Flags: review?(matspal) → review+
Attachment #8342915 - Flags: review?(matspal) → review+
Comment on attachment 8342919 [details] [diff] [review]
part 8: reftests

r=mats
Attachment #8342919 - Flags: review?(matspal) → review+
(Assignee)

Comment 34

4 years ago
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.
(Assignee)

Comment 35

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 946835
(Assignee)

Comment 36

4 years ago
[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.
Flags: in-testsuite+
(Assignee)

Comment 37

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/80fc9290af34
https://hg.mozilla.org/mozilla-central/rev/e4bebee995b4
https://hg.mozilla.org/mozilla-central/rev/54a3dd704f9a
https://hg.mozilla.org/mozilla-central/rev/7ffb0aa5121f
https://hg.mozilla.org/mozilla-central/rev/79a76a1dce4e
https://hg.mozilla.org/mozilla-central/rev/47901ad6d20b
https://hg.mozilla.org/mozilla-central/rev/c9afb8ec2269
https://hg.mozilla.org/mozilla-central/rev/1e621918f8bf
https://hg.mozilla.org/mozilla-central/rev/c4a963c41576
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
relnote-firefox: --- → ?
https://hg.mozilla.org/mozilla-central/rev/64240496195e
https://hg.mozilla.org/mozilla-central/rev/c491f36ee82c

Comment 40

4 years ago
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.)
Keywords: dev-doc-needed
(Assignee)

Comment 41

4 years ago
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.]
Keywords: dev-doc-needed
(Assignee)

Comment 42

4 years ago
[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

4 years ago
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

4 years ago
(Meant Bug 913153, of course. No more bugspam now, sorry!)

Comment 45

4 years ago
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
(Assignee)

Comment 46

4 years ago
@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.
(Assignee)

Comment 47

4 years ago
(sorry -- and by "an attachment that demonstrates the difference", I mean an HTML file instead of a screenshot of renderings + CSS snippets :))

Comment 48

4 years ago
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.

Updated

4 years ago
Attachment #8345511 - Attachment description: overflow:hidden interfereing with flex-wrap:wrap → overflow:hidden interfering with flex-wrap:wrap
(Assignee)

Updated

4 years ago
Blocks: 948654
(Assignee)

Comment 49

4 years ago
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.
(Assignee)

Comment 50

4 years ago
(and thanks for the report! I hope to dig into it soon & see what's going on.)
(Assignee)

Updated

4 years ago
No longer blocks: 948654

Comment 51

4 years ago
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?
(Assignee)

Comment 52

4 years ago
(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.
relnote-firefox: ? → 28+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 971145
(Assignee)

Updated

3 years ago
Blocks: 995020
You need to log in before you can comment on or make changes to this bug.