Daniel Holbert [:dholbert] Assignee
	Description • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 11 years ago

[oops, forgot to update summary when cloning bug; fixed now]

	Daniel Holbert [:dholbert] Assignee
	Comment 2 • 11 years ago

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

	Jonathan Watt [:jwatt]
	Comment 3 • 11 years ago

Sweet! :)

	Daniel Holbert [:dholbert] Assignee
	Comment 4 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 11 years ago

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.

	David
	Comment 6 • 11 years ago

Well this is very exciting! Great to see movement on it; thanks for your hard work

	Mats Palmgren (inactive)
	Comment 7 • 11 years ago

Comment on attachment 8338898 [details] [diff] [review]
part 2: Convert GenerateFlexItems into GenerateFlexLines

r=mats

>+  nsTArray<FlexLine> lines;

Why not nsAutoTArray?

	Daniel Holbert [:dholbert] Assignee
	Comment 8 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 13 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 11 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'.))

	Daniel Holbert [:dholbert] Assignee
	Comment 15 • 11 years ago

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

	Mats Palmgren (inactive)
	Comment 16 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 17 • 11 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.

	Mats Palmgren (inactive)
	Comment 18 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 19 • 11 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.)

	Daniel Holbert [:dholbert] Assignee
	Comment 20 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 21 • 11 years ago

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. :)

	Mats Palmgren (inactive)
	Comment 22 • 11 years ago

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.

	Mats Palmgren (inactive)
	Comment 23 • 11 years ago

Comment on attachment 8342030 [details] [diff] [review]
part 4 ("diff -w" version)

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

static?

	Mats Palmgren (inactive)
	Comment 24 • 11 years ago

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.

	Mats Palmgren (inactive)
	Comment 25 • 11 years ago

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

	Mats Palmgren (inactive)
	Comment 26 • 11 years ago

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?

	Daniel Holbert [:dholbert] Assignee
	Comment 27 • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 28 • 11 years ago

Updated per comment 26.

	Daniel Holbert [:dholbert] Assignee
	Comment 29 • 11 years ago

Updated per comment 26 (for real now; forgot to qref before)

	Daniel Holbert [:dholbert] Assignee
	Comment 30 • 11 years ago

(made a minor tweak to condense a boolean expression to 2 lines instead of 3)

	Daniel Holbert [:dholbert] Assignee
	Comment 31 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 32 • 11 years ago

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.

	Mats Palmgren (inactive)
	Comment 33 • 11 years ago

Comment on attachment 8342919 [details] [diff] [review]
part 8: reftests

r=mats

	Daniel Holbert [:dholbert] Assignee
	Comment 34 • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 35 • 11 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

	Daniel Holbert [:dholbert] Assignee
	Comment 36 • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 37 • 11 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

	Carsten Book [:Tomcat]
	Comment 38 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/64240496195e
https://hg.mozilla.org/mozilla-central/rev/c491f36ee82c

	Florian Bender
	Comment 40 • 11 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.)

	Daniel Holbert [:dholbert] Assignee
	Comment 41 • 11 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.]

	Daniel Holbert [:dholbert] Assignee
	Comment 42 • 11 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))

	Florian Bender
	Comment 43 • 11 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!

	Florian Bender
	Comment 44 • 11 years ago

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

	thinsoldier
	Comment 45 • 11 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

	Daniel Holbert [:dholbert] Assignee
	Comment 46 • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 47 • 11 years ago

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

	thinsoldier
	Comment 48 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 49 • 11 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 50 • 11 years ago

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

	diegobfernandez
	Comment 51 • 11 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?

	Daniel Holbert [:dholbert] Assignee
	Comment 52 • 11 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.