Closed Bug 666041 (css3-flexbox) Opened 13 years ago Closed 12 years ago

CSS Flexbox Layout Level 3 (disabled at build-time, enabled in a subsequent bug)

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: dholbert)

References

(Depends on 1 open bug, Blocks 11 open bugs, )

Details

(Keywords: css3, dev-doc-complete, Whiteboard: [enabled in builds in bug 797022, preffed on everywhere in bug 841876])

Attachments

(12 files, 34 obsolete files)

2.27 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
29.17 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
7.91 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
6.45 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
2.54 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
286.23 KB, patch
Details | Diff | Splinter Review
21.71 KB, patch
dbaron
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
7.42 KB, patch
dbaron
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
5.36 KB, patch
dbaron
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
7.75 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.96 KB, patch
dbaron
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
98.66 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
We should implement the CSS3 Flexbox spec. The spec is still under development, and the latest editor's draft is available here (currently dated 16 June 2011): http://dev.w3.org/csswg/css3-flexbox/
Keywords: css3
Note that we have a bunch of bug reports about differences between that spec and what the XUL flexbox model does.
(In reply to comment #1) > differences between that spec and what the XUL flexbox model does. Note: conveniently, recent versions of the flexbox spec ED use "display: flexbox", whereas the XUL flexbox model (& older versions of the spec[1]) use "display: box". So, we can have different code under the hood for these different models, conditioned off of that. [1] e.g. http://www.w3.org/TR/2009/WD-css3-flexbox-20090723/
Alias: css3-flexbox
Ah, perfect. We want that different code. ;)
At some point after we've shipped display:flexbox we can think about mapping display:-moz-box onto it. Until then we shouldn't worry about it.
This patch just adds a MOZ_FLEXBOX flag to configure.in (similar to the patch in bug 614732). This patch uses AC_SUBST to define MOZ_FLEXBOX in Makefiles, too, because future patches here will add new .cpp files that can be ignored in flexbox-disabled builds.
Attachment #544629 - Flags: review?(khuey)
(sorry, just noticed that previous version wasn't consistent in where it inserted MOZ_FLEXBOX in the various lists of flags. This version tidies that up.)
Attachment #544629 - Attachment is obsolete: true
Attachment #544631 - Flags: review?(khuey)
Attachment #544629 - Flags: review?(khuey)
Attachment #544631 - Attachment description: fix 1 v2: add MOZ_FLEXBOX build flag (disabled for now) → patch 1 v2: add MOZ_FLEXBOX build flag (disabled for now)
Comment on attachment 544631 [details] [diff] [review] patch 1 v2: add MOZ_FLEXBOX build flag (disabled for now) Review of attachment 544631 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4805,5 @@ > MOZ_HELP_VIEWER= > MOZ_SPELLCHECK=1 > MOZ_SPLASHSCREEN= > MOZ_STORAGE=1 > +MOZ_FLEXBOX= As we discussed in person, putting this in alphabetical order wouldn't hurt.
Attachment #544631 - Flags: review?(khuey) → review+
This version fixes the ordering note in prev comment.
Attachment #560241 - Flags: review+
Attachment #544631 - Attachment is obsolete: true
This patch adds support for parsing the flex() function (a new value for the width & height properties, which is used to specify flexible lengths inside of a flexbox -- replacing the "flex" property.) http://dev.w3.org/csswg/css3-flexbox/#flex-function Mostly ready for review, aside from one XXXdholbert in the new parsing code.
This is a tiny patch that makes us recognize the values > display: -moz-flexbox > display: -moz-inline-flexbox and translate them to internal NS_STYLE_DISPLAY[_INLINE]_FLEXBOX values (which are used in future patches). Reference: http://dev.w3.org/csswg/css3-flexbox/#display-flexbox
This patch adds the computed-style code for the "flex()" function.
As noted in the flexbox spec: * all of the ‘column-*’ properties in the Multicol module compute to their initial values on a flexbox (‘break-before’, ‘break-inside’, and ‘break-after’ are still valid on a flexbox). * ‘float’ and ‘clear’ compute to their initial values on a flexbox item This patch makes those changes. Source: http://dev.w3.org/csswg/css3-flexbox/#display-flexbox
Depends on: 692465
Depends on: 696253
Blocks: 702508
You are probably all aware that the spec has changed as of Nov 14th, 2011, what with dbaron being an editor and so on. I was just surprised it wasn't mentioned in this bug. So I'm adding this comment. The changes will break previous usage of the Mozilla flexbox implementation, I suppose. This should be important info for MDN... Between which releases will this happen?
> The changes will break previous usage of the Mozilla flexbox implementation There is no Mozilla flexbox implementation. There is the XUL box implementation, but that uses different syntax and different code, and is not affected by this bug in any way.
(In reply to Marc Diethelm from comment #13) > You are probably all aware that the spec has changed as of Nov 14th, 2011, I appreciate the heads-up! In fact, the editor's draft has changed much more frequently / recently than that. :) (last change 6 days ago -- see http://dev.w3.org/csswg/css3-flexbox/ ) > The changes will break previous usage of the Mozilla flexbox implementation, > I suppose. As noted in comment 2, there's a completely different "display" value used to pick old spec vs new spec (-moz-box vs -moz-flexbox), so it won't break anything. > Between which releases > will this happen? Unsure, but hoping to have patches landing on trunk around February, which would put it around Firefox 13.
Depends on: 718877
Depends on: 725723
Depends on: 732610
marking bug sec-review-needed so we can triage and decide on appropriate action. This likely only needs fuzzing and not a full team meeting type review. Adding dveditz and jesse to this bug to see if they agree.
Sounds good to me. Does the mini-parser need fuzzing? How is this supposed to interact with XUL/box flex?
(In reply to Jesse Ruderman from comment #17) > Sounds good to me. Does the mini-parser need fuzzing? Fuzzing the new properties' parsing would be good, yeah. (there's no parsing aside from the new CSS properties) See bug 696253 -- I should be posting new patches over there today. > How is this supposed > to interact with XUL/box flex? It doesn't. They're completely distinct, per comment 2. (new-flexbox ignores the XUL box properties, and vice versa)
Landed patch 1 (adding build flag, disabled for now), so that the ifdef-guarded patches from bug 696253 can start landing once they've got r+: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b97c946e4eb
Whiteboard: [leave open after merging]
hi. oblivious to the fact that there is a new implementation in the works, i submitted bug 734525 i’m commenting here to ask if the mentioned synergy (column-with specified on a flexbox’ child) works in the new implementation? more specifically, if the following code produces three columns in the header: <!doctype html> <div style="display:-moz-flexbox; -moz-flex-flow: column"> <header>one<br>two<br>three</header> <article style="-moz-flex: 1">Content</div> </div> (bonus points if it still does so when making the body and the div cover the whole viewport ;))
sorry: s/<header>/<header style="column-width:100px">/
(In reply to flying sheep from comment #21) > i’m commenting here to ask if the mentioned synergy (column-with specified > on a flexbox’ child) works in the new implementation? As noted over on bug 734525, my local patches don't yet support vertical flexboxes, so I can't really test that situation at this point. I'll be sure to give that a try once I've got vertical flexbox working, though.
Comment on attachment 560275 [details] [diff] [review] patch 2 v1: parsing code for "flex()" function Obsoleting stale patches here. Note that the "flex()" function for width/height has now been replaced with a "flex:" property[1], which I've posted a patch to parse/compute over on bug 696253. Also, flexboxes are no longer supposed to have special effects on other properties' computed style.[2] (so attachment 560569 [details] [diff] [review] is no longer applicable) [1] http://dev.w3.org/csswg/css3-flexbox/#flexibility [2] http://lists.w3.org/Archives/Public/www-style/2012Jan/1130.html
Attachment #560275 - Attachment is obsolete: true
Attachment #560288 - Attachment is obsolete: true
Attachment #560566 - Attachment is obsolete: true
Attachment #560569 - Attachment is obsolete: true
This patch support for parsing "-moz-flexbox" / "-moz-inline-flexbox" as values for the 'display' property, and wires that up to a no-op nsCSSFrameConstructor method (to be populated by a subsequent patch here, coming in the next day or so once I've finalized it). Most of this patch is just display-keyword-hookup internals. There are two bonus chunks at the end of the patch (which I bundled here since they're basically just special uses of this patch's NS_DISPLAY_ keywords): * The "EnsureBlockDisplay" chunk prevents "display:-moz-flexbox" from computing to "display:block" when it's floated / absolutely-positioned (as it otherwise would). * The nsStyleStruct.h chunk (IsBlockOutside / IsDisplayTypeInlineOutside) is where we behaviorally differentiate -moz-flexbox from -moz-inline-flexbox. SIDE NOTE: If I enable the MOZ_FLEXBOX build flag & uncomment the new chunk in property_database.js, this nearly makes it through layout/style mochitests, but the no-op frame constructor ends up inadvertantly triggering an abort right now -- I've filed bug 737313 on that. (Once the rest of my patches are applied (notably, once the frame constructor method succeeds), we don't hit that abort & we pass style mochitests.)
Attachment #607458 - Flags: review?(dbaron)
Attachment #607458 - Attachment description: patch 2: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords → patch 2 v2: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords
This patch adds a nsFlexboxFrame class, and populates the previous patch's nsCSSFrameConstructor stub-method to return an instance of this new class. (With bug 696253's patches & this bug's patches up to this point applied, mochitests pass.) As of this patch, nsFlexboxFrame is a very simple class, with just boilerplate basically. It inherits the dummy Reflow() & intrinsic-sizing methods from nsFrame, so it won't render anything as of this patch. The interesting non-boilerplate parts will be added in subsequent patches.
Comment on attachment 607458 [details] [diff] [review] patch 2 v2: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords Switching r? to bz on the patches here so far, to lighten dbaron's review-load a bit for this.
Attachment #607458 - Flags: review?(dbaron) → review?(bzbarsky)
Comment on attachment 607458 [details] [diff] [review] patch 2 v2: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords canceling r?, to switch over to a simpler frame-constructor, per IRC discussion w/ bz
Attachment #607458 - Flags: review?(bzbarsky)
Depends on: 738007
assigning to jesse for fuzzing
Whiteboard: [secr:jesse]
Blocks: 734525
OK, this patch uses a simpler FC_DECL rather than a full frame-constructor. (thanks for the tip on that, bz!) This stacks on top of a cleanup patch that I posted over on bug 738007. This patch uses FCDATA_WRAP_KIDS_IN_BLOCKS / FlushAccumulatedBlock to wrap inline children of a flexbox in anonymous blocks. See the XXXdholbert note there about floats _within_ those inline-level children -- with this patch, we won't honor 'float' in that circumstance (which is incorrect). I'm not sure how best to address that right now.
Attachment #607458 - Attachment is obsolete: true
Attachment #608166 - Flags: review?(bzbarsky)
...and here's an updated version of the nsFlexboxFrame boilerplate to stack on top of that. This is basically the same as this patch's previous version ("patch 3 v2"), except: - I've shifted the nsGkAtoms tweak out of this & into patch 2, so that I can check whether frame->GetType() == nsGkAtoms::flexboxFrame there & still compile. - There's no longer a frame-constructor method for this patch to populate. (instead, there's a dummy NS_NewFlexboxFrame() impl that this patch moves from nsCSSFrameConstructor over to the actual nsFlexboxFrame class)
Attachment #607777 - Attachment is obsolete: true
Attachment #608167 - Flags: review?(bzbarsky)
Sorry, forgot to fold in a helper-patch (with 2 chunks to nerf floats on direct children of a flexbox, during frame construction). Folded in now.
Attachment #608166 - Attachment is obsolete: true
Attachment #608171 - Flags: review?(bzbarsky)
Attachment #608166 - Flags: review?(bzbarsky)
Comment on attachment 608171 [details] [diff] [review] patch 2 v4: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords While you're here, want to nix the "To keep the hashtable small" comment in the context there, as well as the XXXbz comment that follows it? In FlushAccumulatedBlock, do you want to use NS_NewBlockFormattingContext to create the block? It ends up doing the same thing as what you wrote, but if this is conceptually supposed to be a BFC, better to make that clear. >+ // We also don't have to worry about our children themselves being floated, >+ // because flexboxes nerf 'float' on their direct children, via a call to >+ // PushFloatContainingBlock(nsnull, ...) inside ProcessChildren. Probably better to push the flexbox itself and then fix up the parentage later or something. If you push null, then the floats won't be out-of-flow at all, and then fixing things up is _really_ hard. Is there a good reason to condition the atom on MOZ_FLEXBOX? Seems like the code would be a lot more readable if that and its consumers were unconditional; there would just be checks that always test false in builds without MOZ_FLEXBOX. Oh, and what I said about this being like what blocks of blocks want? It's not, because you only want to wrap non-replaced inlines. Which, by the way, means that something like <div style="display: flexbox">Some <img> text</div> would end up wrapping the two pieces of text in separate blocks. Is that really the desired behavior? r=me, with the above caveats.
Attachment #608171 - Flags: review?(bzbarsky) → review+
Comment on attachment 608167 [details] [diff] [review] patch 3 v3: nsFlexboxFrame boilerplate r=me
Attachment #608167 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #33) > While you're here, want to nix the "To keep the hashtable small" comment in > the context there, as well as the XXXbz comment that follows it? Done. > In FlushAccumulatedBlock, do you want to use NS_NewBlockFormattingContext to > create the block? It ends up doing the same thing as what you wrote, but if > this is conceptually supposed to be a BFC, better to make that clear. Sure, that sounds good -- done. BTW, I'm actually thinking of using a specialized subclass of nsBlockFrame, much like we do for MathML (with the NS_NewMathMLmathBlockFrame), which would have custom behavior when frames are appended/removed (e.g. to split the anonymous block immediately, when a block-level child is inserted, or to have it destroy itself when its last child is removed). That can probably happen in a separate patch, though -- for the purposes of this base patch, NS_NewBlockFormattingContext is nice & simple. > Probably better to push the flexbox itself and then fix up the parentage > later or something. If you push null, then the floats won't be out-of-flow > at all, and then fixing things up is _really_ hard. OK -- so then, for frames where we _want_ to nerf the 'float' property (direct children of the flexbox), we'll end up needing to forcibly un-float them... but yeah, that's probably easier than forcibly floating the other frames. > Is there a good reason to condition the atom on MOZ_FLEXBOX? Seems like the > code would be a lot more readable if that and its consumers were > unconditional; there would just be checks that always test false in builds > without MOZ_FLEXBOX. Good call -- I'll uncondition nsCSSAnonBoxes::anonymousFlexboxItem as well, and then nsCSSFrameConstructor::FlushAccumulatedBlock won't need any #ifdefs - nice. > Oh, and what I said about this being like what blocks of blocks want? It's > not, because you only want to wrap non-replaced inlines. OK. > Which, by the way, > means that something like <div style="display: flexbox">Some <img> > text</div> would end up wrapping the two pieces of text in separate blocks. > Is that really the desired behavior? Yup, that's the desired behavior -- the correct output would be 3 flexbox items: an anonymous block around "Some", then the image frame, and then the an anonymous block around "text" > r=me, with the above caveats. Thanks!
> OK -- so then, for frames where we _want_ to nerf the 'float' property (direct children > of the flexbox), we'll end up needing to forcibly un-float them... So... I think this is really bizarre behavior the spec calls for. The float should not float unless it happens to be inside a span and then it should? That doesn't make very much sense. Imho. In particular, the fact that this markup: <span style="float: left"></span><span>Text</span> and this markup: <span><span style="float: left"></span>Text</span> would render totally differently is really odd. Why is that desirable? > Yup, that's the desired behavior Seems pretty odd for a vertical flexbox. Esp. if the image fails to load, falls back, and suddenly all you have is non-replaced inlines and only one flexbox item....
(In reply to Boris Zbarsky (:bz) from comment #36) > > OK -- so then, for frames where we _want_ to nerf the 'float' property (direct children > > of the flexbox), we'll end up needing to forcibly un-float them... > > So... I think this is really bizarre behavior the spec calls for. I kind of agree. So, I think I'm just not going to worry this behavior in the initial implementation. Rather than pushing the flexbox as a float container like you suggested (and then manually fixing up the parentage), I'd like to just leave the two float-nerfing chunks in. (in GetFloatContainingBlock() and ProcessChildren()) The upshot of this is that "def" won't float to the left of its anonymous block, in e.g. <div style="display:flexbox"><span>abc<span style="float:left">def</span></span></div> ...but it would of course work if we changed the first <span> to a <div>. I'm not sure whether I actually think the spec should change on this point, but I don't think it's a big enough deal to fuss over in the initial implementation. Does that sound OK to you, bz?
Yes. Furthermore, if we wrapped things in blocks on the frame construction item list level, the float thing would Just Work as the spec says now.... fwiw.
Cool, thanks! [Per IRC discussion, we'll figure that out on Monday when bz is in town.]
...and here's an updated version of patch 2 that addresses the rest of comment 33. (carrying forward r+) (I confirmed that this still builds successfully regardless of whether MOZ_FLEXBOX is defined)
Attachment #608171 - Attachment is obsolete: true
Attachment #608902 - Flags: review+
Updated version of patch 2, now doing the wrapping at the FCItem list level, as bz suggested in comment 38. (Nothing outside of nsCSSFrameConstructor.* has changed) Caveats: - This doesn't have the WipeContainingBlock logic yet. (have to sort out a few things there) As flagged with an XXXdholbert comment, I'm pretty sure I'm doing the wrong thing at the end of nsCSSFrameConstructor::CreateNeededAnonFlexboxItems: > static const FrameConstructionData sBlockFormattingContextFCData = > FCDATA_DECL(0, NS_NewBlockFormattingContext); > > FrameConstructionItem* newItem = > new FrameConstructionItem(&sBlockFormattingContextFCData, That ^ ends up producing a block frame that sets itself as our flexbox element's primary frame (fighting with the actual flexbox frame & triggering an assertion failure). How should I be generating that frame construction item? Also: I also don't entirely grok what "mHasInlineEnds" is supposed to be set to -- maybe it should unconditionally true, since we know we're populated with inline content? I'm juts mimicking CreateNeededTablePseudos()'s chunk about that member var for now.
Attachment #608902 - Attachment is obsolete: true
Attachment #609908 - Flags: feedback?(bzbarsky)
Comment on attachment 609908 [details] [diff] [review] patch 2 v6: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and cssfc code Hmm. Why FCDATA_FORCE_NULL_FLOAT_CONTAINER instead of the frame-type check in ProcessChildren where we handle other things that should push a null float containing block? And you still need the check for flexbox in GetFloatContainingBlock, no? Otherwise dynamic insertion of a float as a direct kid of a flexbox won't work right (add a testcase!). Please document the return value of SkipItemsThatDontNeedAnonFlexboxItem. > ends up producing a block frame that sets itself as our flexbox element's primary frame You want FCDATA_SKIP_FRAMESET as the first argument to FCDATA_DECL, instead of 0. The comment about the namespace mattering was true for tables only because of the mathml weirdness. I believe in your case the namespace in fact does not matter. The code to set mIsAllInline and friends looks correct (though chances are nothing reads them after this point), but in practice shouldn't there be a ua.css rule setting display:block on :-moz-anonymous-flexbox-item? Seems like that's what the spec calls for. And then these booleans will all end up set to false, of course. I would somewhat prefer to hoist the check for a flexbox parent out of CreateNeededAnonFlexboxItems because then in the common case we won't have to pay the extra function call cost. Looks pretty good in general!
Attachment #609908 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #42) > Comment on attachment 609908 [details] [diff] [review] > patch 2 v6: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and > cssfc code > > Hmm. Why FCDATA_FORCE_NULL_FLOAT_CONTAINER instead of the frame-type check > in ProcessChildren where we handle other things that should push a null > float containing block? You're right, that change made no sense. (I hadn't noticed that ProcessChildren()'s null float-containing-block push happened _right_ after that.) I've reverted that & restored the previous patch's frame-type check in ProcessChildren. > And you still need the check for flexbox in GetFloatContainingBlock, no? > Otherwise dynamic insertion of a float as a direct kid of a flexbox won't > work right (add a testcase!). Yup, you're right. I've added that back. I'll add a testcase for that, too. (Tests are coming in their own separate patch, once I've posted the main patch here to actually render stuff. :)) > Please document the return value of SkipItemsThatDontNeedAnonFlexboxItem. Done. (I copypasted the documentation for the analogous SkipItemsWantingParentType()'s return value.) > You want FCDATA_SKIP_FRAMESET as the first argument to FCDATA_DECL, instead > of 0. Aha! Thanks -- I saw that flag elsewhere in the file, but I assumed it had something to do with the <frameset> element. :) Makes sense now though. I added USE_CHILD_ITEMS, too, per IRC conversation. > The comment about the namespace mattering was true for tables only because > of the mathml weirdness. I believe in your case the namespace in fact does > not matter. OK, removed that comment. > in practice shouldn't there be a > ua.css rule setting display:block on :-moz-anonymous-flexbox-item? Ah, yes -- thanks! (context / reasoning from IRC discussion: this is needed to make these anonymous items blocks instead of inline-blocks, which is needed for their baselines to be correct, for "flex-align" to work.) (I'll add a test for this, too, to be sure the anonymous flexbox items' baselines align correctly for blocks (rather than inline-blocks), for when we've got 'flex-align:baseline' support.) > I would somewhat prefer to hoist the check for a flexbox parent out of > CreateNeededAnonFlexboxItems Good call -- fixed. (I was cribbing too closely from CreateNeededTablePseudos, which has a slightly more complex initial check.) FWIW, I also tweaked CreateNeededAnonFlexboxItems() as follows: - made it return void, since its only return value was NS_OK - added documentation in .h file for this method - renamed local var "isDroppingWhitespace" to "shouldDropWhitespace" for clarity (Those are the only changes other than the things I mentioned for review feedback above.) The checks for when we need frame-reconstruction are coming in a separate patch -- I'm writing tests for that patch now, to be sure it's not missing some edge case.
Attachment #609908 - Attachment is obsolete: true
Attachment #610665 - Flags: review?(bzbarsky)
One more tweak: so the FCDATA_USE_CHILD_ITEMS flag (new in the last patch revision) makes us use ConstructFramesFromItemList instead of ProcessChildren to handle the anonymous flexbox item's children, which means we don't end up pushing it as a float containing block. (but we need to) So -- this updated patch adds a special PushFloatContainingBlock call for that case (with a comment explaining why)
Attachment #610665 - Attachment is obsolete: true
Attachment #611082 - Flags: review?(bzbarsky)
Attachment #610665 - Flags: review?(bzbarsky)
Good catch on the PushFloatContainingBlock bit. I think the right way to do that is to, in the FCDATA_USE_CHILD_ITEMS case in nsCSSFrameConstructor::ConstructFrameFromItemInternal, before calling ConstructFramesFromItemList, assert that the frame is not mathml or a boxframe and then push a float containing block if IsFloatContainingBlock. Basically, similar code to what we have in ProcessChildren, except assert that the first condition doesn't hold. I'll review the rest on Monday when I can be more sure to not miss something in the iterator code...
(In reply to Boris Zbarsky (:bz) from comment #45) > I think the right way to do > that is to, [...] > Basically, similar code to what we have in ProcessChildren, except assert > that the first condition doesn't hold. Sounds good -- done. To avoid code-duplication, I split out the "not mathml nor box frame nor flexbox frame" check into a helper method "IsFrameAllowedToBeFloatContainingBlock()", since this will be the third place where we need that logic. I also added an early-return in CreateNeededAnonFlexboxItems for when the items list is empty, since apparently we can receive empty lists there. (That triggered a crash when I ran through layout/style mochitests) > I'll review the rest on Monday when I can be more sure to not miss something > in the iterator code... Thanks!
Attachment #611082 - Attachment is obsolete: true
Attachment #611236 - Flags: review?(bzbarsky)
Attachment #611082 - Flags: review?(bzbarsky)
(removed accidental double-blank-line between methods)
Attachment #611236 - Attachment is obsolete: true
Attachment #611237 - Flags: review?(bzbarsky)
Attachment #611236 - Flags: review?(bzbarsky)
Here's the patch to detect when we need to recreate flexbox frames. The basic idea is as follows -- when removing a frame, we need to check for these special cases: (a) ...if it might've been the only thing separating two anonymous flexbox items (which then need to be merged) (b) ...if it might've been the last remaining child of an anonymous flexbox item (which can then go away) When inserting a frame, we need to check... (c) ...if it's an inline non-replaced child inserted into a flexbox (and needs wrapping) (d) ...if it's _not_ an inline-non-replaced-child (i.e. it doesn't want to be wrapped), but we've inserted it inside of an anonymous flexbox item (which we now need to split) If we detect any of the above cases, we recreate frames for the flexbox, to benefit from all the child-wrapping logic in the previous patch. FWIW, I've got reftests for these situations, which live in https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/tip/flexbox-reftests.patch (flexbox-horiz-inlineContent-1*, flexbox-horiz-inlineContent-2*, and flexbox-horiz-inlineContent-5* are the reftests that are most relevant to this patch.)
Attachment #611598 - Flags: review?(bzbarsky)
No longer depends on: 692465
(Removed a useless null-check on |pseudoType| that I just noticed in IsAnonymousFlexboxItem)
Attachment #611598 - Attachment is obsolete: true
Attachment #612315 - Flags: review?(bzbarsky)
Attachment #611598 - Flags: review?(bzbarsky)
Comment on attachment 611237 [details] [diff] [review] patch 2 v9a: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and cssfc code >+IsFrameAllowedToBeFloatContainingBlock(nsIFrame* aFrame) That's not quite what this test is used for in GetFloatContainingBlock. How about naming the function ShouldSuppressFloatingOfDescendants() and reversing the sense of the boolean accordingly? The assert text in the USE_CHILD_ITEMS code you're adding might need changing accordingly too. >+nsCSSFrameConstructor::CreateNeededAnonFlexboxItems( >+ // If we should drop whitespace and we're currently pointing to >+ // whitespace, then drop everything from here to the next >+ // non-whitespace thing. I'm not sure the logic here is right. Say you have this markup: <div style="display: flexbox"><span>x</span> <span>y</span></div> This generates 3 child items; call them A, B, C. Now we enter the outermost loop. iter == A. We skip nothing, since A is an item for a <span>. We set endIter == A and enter the inner loop. A is not whitespace, so we skip that whole block. A needs an anon flexbox item, so we increment endIter and return to the top of the inner loop. Now endIter == B. This is a whitespace item, so we enter the block inside the inner loop, set spaceEndIter to B, skip it forward to C, then delete the whitespace item. Now endIter == C. This still needs an anon flebox item, so we advance endIter to end of list and finish the inner loop because the while condition tests false. We then wrap the remaining items (A and C) in an anonymous block. But the rendering will come out "xy" instead of the "x y" it should be. Should be testable by comparison to <<div style="display: flexbox">x y</div>. >+ newItem->mIsAllInline = newItem->mHasInlineEnds = >+ newItem->mStyleContext->GetStyleDisplay()->IsInlineOutside(); You should probably also set mIsBlock to be !mIsAllInline. >@@ -9441,16 +9590,23 @@ nsCSSFrameConstructor::ConstructFramesFr >+#ifdef MOZ_FLEXBOX >+ nsFrameConstructorSaveState floatSaveState; >+ if (aParentFrame->GetType() == nsGkAtoms::flexboxFrame) { >+ CreateNeededAnonFlexboxItems(aState, aItems, aParentFrame); >+ } >+#endif // MOZ_FLEXBOX You don't need the floatSaveState there. >@@ -9488,20 +9644,18 @@ nsCSSFrameConstructor::ProcessChildren(n >- // The logic here needs to match the logic in GetFloatContainingBlock() Please keep that comment; it's still relevant. The rest looks fine, but I'd like to review a new patch with the whitespace issue fixed.
Attachment #611237 - Flags: review?(bzbarsky) → review-
Comment on attachment 612315 [details] [diff] [review] patch 2.5 v2: Recreate flexbox frame on dynamic modifications >+++ b/layout/base/nsCSSFrameConstructor.cpp >+static bool >+IsAnonymousFlexboxItem(const nsIFrame* aFrame) Worth declaring this inline? Or just leave it up to the compiler? >+ // Might need to reconstruct things if the removed frame's nextSibling is an >+ // anonymous flexbox item. The removed frame might've been what divided two >+ // runs of inline content into two anonymous flexbox items, which would now >+ // need to be merged. >+ if (nextSibling && IsAnonymousFlexboxItem(nextSibling)) { It's worth adding a comment that the fact that nextSibling skipped over whitespace is ok, since there shouldn't be cases when whitespace has an anonymous flexbox item nextsibling anyway. >@@ -11128,19 +11183,68 @@ nsCSSFrameConstructor::WipeContainingBlo >+ // Situation #2 is a Flexbox frame that has inline non-replaced children >+ // added (which need to be wrapped in an anonymous flexbox item) This is a bit dangerous when dynamic appends during parsing are involved, since whitespace in aItems will trigger reconstructs... Is it worth beefing up the check here to skip whitespace as well? r=me with those nits (of which the last one is the most important, imo).
Attachment #612315 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #50) > Comment on attachment 611237 [details] [diff] [review] > patch 2 v9a: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords > and cssfc code > > >+IsFrameAllowedToBeFloatContainingBlock(nsIFrame* aFrame) > How about naming the function ShouldSuppressFloatingOfDescendants() and > reversing the sense of the boolean accordingly? The assert text in the > USE_CHILD_ITEMS code you're adding might need changing accordingly too. Good call. Did all three of those things. > >+nsCSSFrameConstructor::CreateNeededAnonFlexboxItems( > > I'm not sure the logic here is right. Say you have this markup: > <div style="display: flexbox"><span>x</span> <span>y</span></div> Yeah, that logic was a bit horked -- sorry about that. I've now simplified (and improved commenting on) this chunk significantly. I removed most of the whitespace-checking, now that I understand this better & that it's been clarified a bit on www-style. (in particular: leading/trailing whitespace within an anonymous flexbox item doesn't need any special treatment, and I only need to screen for & drop whitespace that would end up creating a whitespace-only anonymous flexbox item.) (BTW, as part of this logic-simplification, I ended up refactoring out some code into a new "SkipItemsThatNeedAnonFlexboxItem" function, since this was the second place I needed that logic. It's identical to "SkipItemsThatDontNeedAnonFlexboxItem", but with its 'while' conditional reversed. (note the "Need" vs "DontNeed" distinction in the function names) > Should be testable by comparison to <div style="display: > flexbox">x y</div>. Righto -- I've now got a reftest that does just that -- "flexbox-whitespace-handling-3.xhtml" in https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/f7dcdd259350/flexbox-reftests.patch#l4984 (reference case directly above it) > >+ newItem->mIsAllInline = newItem->mHasInlineEnds = > >+ newItem->mStyleContext->GetStyleDisplay()->IsInlineOutside(); > > You should probably also set mIsBlock to be !mIsAllInline. Done. We expect our anonymous flexbox items to always be block-level, so I added an assertion to that effect. (This assertion also turns out to be a handy way to test that we've got ua.css set up correctly -- i.e. that-moz-anonymous-flexbox-items are "display:block" instead of "display:inline-block".) > You don't need the floatSaveState there. Good catch -- removed. > >@@ -9488,20 +9644,18 @@ nsCSSFrameConstructor::ProcessChildren(n > >- // The logic here needs to match the logic in GetFloatContainingBlock() > > Please keep that comment; it's still relevant. OK, cool -- I've restored it & its partner-comment in GetFloatContainingBlock.
Attachment #611237 - Attachment is obsolete: true
Attachment #613079 - Flags: review?(bzbarsky)
Attachment #613079 - Attachment description: atch 2 v10: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and cssfc code → patch 2 v10: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and cssfc code
(In reply to Boris Zbarsky (:bz) from comment #51) > >+static bool > >+IsAnonymousFlexboxItem(const nsIFrame* aFrame) > > Worth declaring this inline? Or just leave it up to the compiler? Sure -- added 'inline' locally. > It's worth adding a comment that the fact that nextSibling skipped over > whitespace is ok, since there shouldn't be cases when whitespace has an > anonymous flexbox item nextsibling anyway. Fixed locally. > >@@ -11128,19 +11183,68 @@ nsCSSFrameConstructor::WipeContainingBlo > >+ // Situation #2 is a Flexbox frame that has inline non-replaced children > >+ // added (which need to be wrapped in an anonymous flexbox item) > > This is a bit dangerous when dynamic appends during parsing are involved, > since whitespace in aItems will trigger reconstructs... Is it worth beefing > up the check here to skip whitespace as well? Ah -- so you mean if the inserted/appended content is e.g. "<div>abc</div> <div>def</div>", then we don't want that whitespace in the middle to fool us into thinking we're going to need some new anonymous flexbox items & unnecessarily call RecreateFramesForContent -- correct? That makes sense. I _think_ if |aItems| has whitespace at its beginning or end, though, we'll still need to RecreateFramesForContent, at least if WhitespaceIsSignificant(), because that whitespace could end up being adjacent to an existing anonymous flexbox item, in which case it should get bundled into it & could affect its rendering. Does this sound right?
> correct? Yes. > I _think_ if |aItems| has whitespace at its beginning or end, though, we'll still need to > RecreateFramesForContent Or at least check what's coming before/after it, yes. If we have no good way to do that yet, recreating for now and filing a bug to fix that makes sense to me.
Comment on attachment 613079 [details] [diff] [review] patch 2 v10: add "display: -moz-flexbox" & "-moz-inline-flexbox" keywords and cssfc code >+++ b/layout/base/nsCSSFrameConstructor.cpp >+nsCSSFrameConstructor::CreateNeededAnonFlexboxItems( >+ bool nextChildNeedsAnonFlexItem = hitEnd ? >+ false : afterWhitespaceIter.item().NeedsAnonFlexboxItem(aState); I would personally find this more readable as: bool nextChildNeedsAnonFlexItem = !hitEnd && afterWhitespaceIter.item().NeedsAnonFlexboxItem(aState); but maybe that's just opinion, not fact... ;) >+ if (hitEnd || !nextChildNeedsAnonFlexItem) { In either case, hitEnd being true implies nextChildNeedsAnonFlexItem is false, so there's no need to test for hitEnd here. Just testing !nextChildNeedsAnonFlexItem is enough. >+ iter.DeleteItemsTo(afterWhitespaceIter); >+ iter = afterWhitespaceIter; The DeleteItemsTo call updates iter to point to afterWhitespaceIter, so no need for the second line there, I think. >+ NS_ABORT_IF_FALSE(!nextChildNeedsAnonFlexItem, >+ "how else did we get here?"); If the block entry condition changes, this can go away (though the comment above it should stay). Or you could make this NS_ABOSRT_IF_FALSE(afterWhitespaceIter.item().NeedsAnonFlexboxItem(aState)), I guess. There's only one caller of SkipItemsThatNeedAnonFlexboxItem in here; I assume the other patch will also use it? r=me with the above nits.
Attachment #613079 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #55) > I would personally find this more readable as: > > bool nextChildNeedsAnonFlexItem = > !hitEnd && afterWhitespaceIter.item().NeedsAnonFlexboxItem(aState); Agreed -- fixed. > >+ if (hitEnd || !nextChildNeedsAnonFlexItem) { > > In either case[...] Just testing !nextChildNeedsAnonFlexItem is enough. True -- fixed. > The DeleteItemsTo call updates iter to point to afterWhitespaceIter, so no > need for the second line there, I think. Ok, removed that second line. > >+ NS_ABORT_IF_FALSE(!nextChildNeedsAnonFlexItem, > >+ "how else did we get here?"); > > If the block entry condition changes, this can go away (though the comment > above it should stay). Or you could make this > NS_ABOSRT_IF_FALSE(afterWhitespaceIter.item().NeedsAnonFlexboxItem(aState)), > I guess. Changed this to assert that |iter| is not done & points to something non-wrappable -- I'm asserting that jumping back to the start of the loop is a sensible thing to do, essentially. (updated the comment, too) > There's only one caller of SkipItemsThatNeedAnonFlexboxItem in here; I > assume the other patch will also use it? Yup, the other patch will indeed use it. (new version of that patch coming up next) > r=me with the above nits. Thanks! Carrying that forward.
Attachment #613079 - Attachment is obsolete: true
Attachment #613458 - Flags: review+
Here's the updated version of patch 2.5, for detecting when we need to do frame-reconstruction. This addresses the things bz brought up in Comment 51 (& expanded on in comment 54). It also adds the second call to SkipItemsThatNeedAnonFlexboxItem() (replacing the equivalent logic), now that "patch 2" adds that method. Requesting review again, since the modifications to "Situation #2" in WipeContainingBlock are nontrivial (though pretty straightforward I think).
Attachment #612315 - Attachment is obsolete: true
Attachment #613463 - Flags: review?(bzbarsky)
(To test the new code in patch 2.5, I added a bunch of new tests to my reftests patch in my patch queue, to test dynamic insertions of whitespace together with text/divs/spans. Those tests are called "flexbox-dyn-insertAround*" and are listed here: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/d818235a2083/flexbox-reftests.patch#l58 )
Comment on attachment 613463 [details] [diff] [review] patch 2.5 v3: Recreate flexbox frame on dynamic modifications Hmm. Looking at this code, it seems that it's more complicated than it needs to be. Isn't it enough to check for aPrevSibling being an anon flexbox item and our list starting with something that will want an anon flexbox parent and likewise for the end of the list? We don't actually care if things in the _middle_ want anon flexbox items; if they do, we'll create them as needed when we go to construct frames.
Yup, you're absolutely right. Straightened that out here.
Attachment #613463 - Attachment is obsolete: true
Attachment #613491 - Flags: review?(bzbarsky)
Attachment #613463 - Flags: review?(bzbarsky)
Comment on attachment 613491 [details] [diff] [review] patch 2.5 v4: Recreate flexbox frame on dynamic modifications >+++ b/layout/base/nsCSSFrameConstructor.cpp >@@ -11130,17 +11189,87 @@ nsCSSFrameConstructor::WipeContainingBlo >+ // Seek to the end of aItems and see if its final entry needs an >+ // anonymous flexbox item. The efficient way to do that with existing APIs given a known-nonempty list as in this case is: iter.SetToEnd(); iter.Prev(); though quite honestly, it may make sense to add (in a followup bug) First() and Last() accessors on FrameConstructionItemList so you don't even have to use an iterator here... In either case, walking the whole list linearly is totally unnecessary. r=me with that fixed.
Attachment #613491 - Flags: review?(bzbarsky) → review+
Nice! I didn't realize the iterator API was that friendly. :) Fixed that here. > it may make sense to add (in a followup bug) First() and Last() accessors > on FrameConstructionItemList so you don't even have to use an iterator here.. (I'll hold off on that for now, since it looks like those methods aren't particularly needed yet -- at least, existing code only calls "SetToEnd" in one spot, that spot wouldn't benefit from using Last().) (Also, AFAICT, First()/Last() would be the first methods to directly expose items from the FrameConstructionItemList API.)
Attachment #613491 - Attachment is obsolete: true
Attachment #613689 - Flags: review+
Blocks: 744642
No longer blocks: 734525
I fuzzed a version of dholbert's flexbox patch queue and didn't hit any problems :)
:jesse if you think this is good to go then please mark sec-review-complete
Whiteboard: [secr:jesse] → [sec-assigned:jesse]
Whiteboard: [sec-assigned:jesse]
Blocks: 753559
Depends on: 756671
Depends on: 763689
I realized that the patches here (up to the frame-class boilerplate patch) don't actually depend on bug 696253's patches, so I'm going to go ahead and land them (or their up-to-date versions, rather) before landing the patches from bug 696253, to keep them from bitrotting further. (Note that this code is basically all disabled by the MOZ_FLEXBOX build flag for now, too.) Before landing, I'm posting the updated versions of the patches #2, #2.5, & #3 from my patch queue. They're functionally the same as the already-r+'d patches, but with some global renamings (in code, comments, and assertion messages), to keep up with spec changes: -moz-flexbox --> -moz-flex (& underscore variants) -moz-inline-flexbox --> -moz-inline-flex (& underscore variants) nsFlexboxFrame --> nsFlexContainerFrame flexbox frame --> flex container frame nsGkAtoms::flexboxFrame --> nsGkAtoms::flexContainerFrame flexbox item --> flex item These changes were simple search & replace, followed by manual sanity-checking / reindentation / re-wrapping comments that became too long, etc. So -- no functional changes. Hence, carrying forward r+. (The handling of replaced children has changed / is changing in the spec, too[1] but I haven't implemented that change yet. I'm planning on landing these patches as-they-are and then sort out the replaced-element-handling updates in bug 756671.) [1] http://wiki.csswg.org/topics/css3-flexbox-flexbox-replaced-children
Attachment #613458 - Attachment is obsolete: true
Attachment #636871 - Flags: review+
Attachment #636872 - Attachment description: patch 2.5 v5: Recreate flexbox frame on dynamic modifications [r=bz] → patch 2.5 v5: Recreate flex container frame on dynamic modifications [r=bz]
Attachment #560241 - Flags: checkin+
Attachment #636871 - Flags: checkin+
Attachment #636872 - Flags: checkin+
Attachment #636873 - Flags: checkin+
Attachment #636873 - Attachment description: patch 3 v4: nsFlexContainerFrame boilerplate → patch 3 v4: nsFlexContainerFrame boilerplate [r=bz]
Attachment #636873 - Flags: review+
This patch adds a mochitest for the flexbox layout algorithm. It includes an html file "test_flexbox_ as a "harness", and a JS file with a list of testcases. Each testcase in the JS file specifies a list of flex items, each with its own set of CSS properties, to stick in a 200px-wide flex container. Then, we check the resulting computed style of the items against what we'd expect the flexbox layout algorithm to give it. Side note: the goal of the testcase structure is to be extensible, so that at some point we could add additional JS properties in new entries within flexbox_layout_testcases.js (alongside "items", the only JS property in each testcase right now), to e.g. customize the orientation of the flexbox, override the default container-size, etc.
Attachment #638899 - Flags: review?(dbaron)
(updated mochitest to include some cases with gigantic (== float infinity) flex-grow values.)
Attachment #638899 - Attachment is obsolete: true
Attachment #638899 - Flags: review?(dbaron)
Attachment #641777 - Flags: review?(dbaron)
This patch makes us use the flex-basis in place of 'width' when we're in nsFrame::ComputeSize and nsLayoutUtils::ComputeSizeWithIntrinsicDimensions (te latter of which is used by several nsIFrame::ComputeSize implementations). This is for the purpose of computing the "flex base size", which is described in the spec here: http://dev.w3.org/csswg/css3-flexbox/#line-sizing
Attachment #641813 - Flags: review?(dbaron)
Similarly, this patch makes us ignore min & max-width properties for flex items when we're evaluating ComputeSize() for them. (since their min & max properties are taken into account at a later stage in the flexbox algorithm)
Attachment #641814 - Flags: review?(dbaron)
Attached patch patch 7: main flexbox class impl (obsolete) — Splinter Review
Here's the patch that fleshes out nsFlexContainerFrame -- the bulk of the implementation. (Still making incremental improvements to this, so requesting feedback? instead of review? for now, but feel free to start reviewing if you have the cycles for it)
Attachment #641815 - Flags: feedback?(dbaron)
And here's a patch to make "min-width:auto" evaulate to min-content in the places that we care about it for a flex item, as described in http://dev.w3.org/csswg/css3-flexbox/#min-size-auto (those places being: getComputedStyle, and nsHTMLReflowState.mComputedMinWidth) (NOTE: These patches (#5-8) all stack on top of bug 763689 patch 2, which makes us convert "min-width:auto" to 0 globally.)
Attachment #641818 - Flags: review?(dbaron)
Attached patch reftests patchSplinter Review
...and for the record, here are the reftests I'm using. (though I want to hack on these a bit more before requesting review on them.)
...and here's the patch to enable flexbox in builds. (numbering as #99 since it comes after all the other patches) Without this patch, the other patches here (and those that landed in bug 696253) won't have any functional effect.
Attachment #641820 - Attachment description: patch 99: enable → patch 99: enable flexbox in builds
I've now added support for vertical flexbox to my patches for this bug -- here's the mochitest for the flexbox layout algorithm again, converted such that its testcases are axis-independent.
Attachment #641777 - Attachment is obsolete: true
Attachment #641777 - Flags: review?(dbaron)
Attachment #646884 - Flags: review?(dbaron)
Attachment #641813 - Attachment is obsolete: true
Attachment #641813 - Flags: review?(dbaron)
Attachment #646891 - Attachment is obsolete: true
Attachment #646891 - Flags: review?(dbaron)
Attachment #646893 - Flags: review?(dbaron)
Attachment #646891 - Attachment description: patch 5: use 'flex-basis' in place of 'width' or 'height' in ComputeSize → patch 5 v2: use 'flex-basis' in place of 'width' or 'height' in ComputeSize
Attachment #641814 - Attachment is obsolete: true
Attachment #641814 - Flags: review?(dbaron)
This patch adds a flag to nsHTMLReflowState and nsIFrame::ComputeSize to let us measure the auto-height of a child-frame. (We need to be able to do that, regardless of whether 'height' and 'flex-basis' are actually set to 'auto', to get an intrinsic height for us to use in flexbox layout when we've got "min-height:auto".)
Attachment #646903 - Flags: review?(dbaron)
Attachment #641815 - Attachment is obsolete: true
Attachment #641815 - Flags: feedback?(dbaron)
Attachment #646904 - Flags: review?(dbaron)
Comment on attachment 646884 [details] [diff] [review] patch 4 v3: mochitest for flexbox layout I wonder why the property mapping for padding/border/margin doesn't change when you use row-reverse or column-reverse (relative to row or column). Should it? r=dbaron
Attachment #646884 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #84) > Comment on attachment 646884 [details] [diff] [review] > patch 4 v3: mochitest for flexbox layout > > I wonder why the property mapping for padding/border/margin doesn't change > when you use row-reverse or column-reverse (relative to row or column). > Should it? Ah, good point -- a "direction: row-reverse" flex container has its main-start edge at the right side, so "_margin-main-start" should technically map to margin-right instead of margin-left. I'll add "reverse" versions of the property maps, with that fixed. > r=dbaron Thanks!
Comment on attachment 646893 [details] [diff] [review] patch 5 v2a: use 'flex-basis' in place of 'width' or 'height' in ComputeSize I think you should make a method on nsIFrame called IsFlexItem() rather than writing it out in two places here (and I expect more to come, perhaps). r=dbaron with that
Attachment #646893 - Flags: review?(dbaron) → review+
Comment on attachment 646896 [details] [diff] [review] patch 6 v2: ignore min/max-main-size property in ComputeSize I think you should have the comment in nsLayoutUtils.cpp in addition to in nsFrame.cpp. r=dbaron with that
Attachment #646896 - Flags: review?(dbaron) → review+
Comment on attachment 646903 [details] [diff] [review] patch 6.5 v1: Add flag to nsHTMLReflowState & ComputeSize for measuring auto height There's a missing ) in the comment in nsIFrame.h. I'm wondering whether this patch is really sufficient -- do you need to handle more than blocks? (And if you're dealing with anything that uses nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, should anything else be ignored?) I'm also not sure what part of the spec explains what this patch is supposed to do.
Landed patch 4 (mochitest for flexbox layout), with review fixes per comment 84 / comment 85: https://hg.mozilla.org/integration/mozilla-inbound/rev/403c8e569571
Attachment #646884 - Flags: checkin+
Attachment #646903 - Attachment description: patch 6.5 v1: Add flag to nsHTMLReflowState & ComputeSize for measuiring auto height → patch 6.5 v1: Add flag to nsHTMLReflowState & ComputeSize for measuring auto height
Comment on attachment 646904 [details] [diff] [review] patch 7 v2: main flexbox class impl >+ eAxis_LR = 0, // Start at 0 so we can use these values as array indices. I think you're ok leaving out both the =0 and the comment, though I suppose it's ok. C and C++ guarantee that enums start at 0 unless specified otherwise, and IMO it's pretty normal to use enums as array indices. But it's ok either way. (The "For sizing arrays..." comment at the end seems more useful, and should perhaps be duplicated to the next enum.) >+static const nscoord& >+GetMarginComponentForSideInternal(const nsMargin& aMargin, Side aSide) Probably better to return nscoord than const nscoord&. Then again, you could alternatively use the pattern: nscoord MarginComponentForSide(const nsMargin& aMargin, Side aSide); nscoord& MarginComponentForSide(nsMargin& aMargin, Side aSide); instead of the getter/setter pattern. But the way you have it is fine too. FlexboxAxisTracker: Regarding the PhysicalPositionFromLogicalPosition and PhysicalSizeFromLogicalSizes methods: I wonder if we should have types for logical points and logical sizes. SortableFrame: The GetFrame() method should probably be called Frame() instead since it should be assumed to be non-null. (And maybe the constructor should assert that the frame is non-null.) FlexItem: As before, silly to return |const nscoord &|. Things should either return |nscoord| or |nscoord&|. (In general, no point using const references for simple types.) This happens a few times in this class. Would it make sense for any more of the setters to assert about frozenness? >+ // XXXdholbert Is it kosher to say we have unconstrained available width? No. What is it you're trying to do? Also, if you're trying to set the available space for a reflow state, you probably don't want to set a height other than NS_UNCONSTRAINEDSIZE -- that means you're paginating. (Yes, the distinction in the reflow state is really broken right now -- those belong as separate concepts, and flexbox may actually require us to fix it by renaming nsHTMLReflowState::availableHeight to something related to breaking and then introducing a new availableHeight concept.) Then again, this method looks unused. Can you remove it? You should have a blank line after the end of nsFlexContainerFrame::IsHorizontal(). >+ // Do an unconstrained-height reflow into our container's available space >+ // to get an intrinsic height for this child. It's not really unconstrained height that's the key factor here, since unconstrained height currently means "not paginating". The interesting stuff is done by mIsFlexContainerMeasuringHeight combined with patch 6.5. If you're passing NS_FRAME_NO_MOVE_FRAME, you should also pass NS_FRAME_NO_MOVE_VIEW. Note that it doesn't affect descendants from being resized, though, if you're reflowing at a different size. What behavior do we expect for percentage heights on children in this auto-height computation pass -- particularly if we're computing an auto min-height and there's a specified height? And what does this code do? >+ // CROSS MIN/MAX SIZE For crossMinSize -- do you need to support min-height:auto for the cross size as well? FlexItem::FlexItem: - initializing to nscoord_MIN feels odd -- is this meaningful or can you use 0 instead? - in the #ifdef DEBUG block, you can pull styleMargin out of the loop, but put an additional set of braces around it for scoping, i.e. #ifdef DEBUG { ... } #endif >+ // Resolve "align-self: auto" to parent's "align-items" value. >+ if (mAlignSelf == NS_STYLE_ALIGN_SELF_AUTO) { >+ mAlignSelf = mFrame->GetParent()->GetStylePosition()->mAlignItems; >+ } This isn't quite right -- you want the element parent, so you want data from what you'd inherit style from. So instead, you want the assignment to assign mFrame->GetStyleContext()->GetParent()->GetStylePosition()->mAlignItems; mFrame->GetParent()->GetStyleContext() is not necessarily the same as mFrame->GetStyleContext()->GetParent(), and in this case you want the latter. (This means avoiding the helper function that shortens mFrame->GetStyleContext()->GetStylePosition() to mFrame->GetStylePosition(), which makes it look longer, but it isn't conceptually any more complicated.) FlexItem::GetNumAutoMarginsInAxis - pull |styleMargin| out of the loop ... to be continued ...
[responding to comments on patch 5, 6, 6.5, and 7] (In reply to David Baron [:dbaron] from comment #86) > Comment on attachment 646893 [details] [diff] [review] > patch 5 v2a: use 'flex-basis' in place of 'width' or 'height' in ComputeSize > > I think you should make a method on nsIFrame called IsFlexItem() rather than > writing it out in two places here (and I expect more to come, perhaps). Fixed in patch queue: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/fb166e1daf7b (In reply to David Baron [:dbaron] from comment #87) > Comment on attachment 646896 [details] [diff] [review] > patch 6 v2: ignore min/max-main-size property in ComputeSize > > I think you should have the comment in nsLayoutUtils.cpp in addition to in > nsFrame.cpp. Fixed in patch queue: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/201ef375dcd6 (Can't land those patches yet, FWIW, because they layer on top of some not-yet-landed-code from bug 763689.) (In reply to David Baron [:dbaron] from comment #88) > Comment on attachment 646903 [details] [diff] [review] > patch 6.5 v1: Add flag to nsHTMLReflowState & ComputeSize for measuiring > auto height > > There's a missing ) in the comment in nsIFrame.h. Fixed in patch queue: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/3819539f2639 > I'm wondering whether this patch is really sufficient -- do you need to > handle more than blocks? So, the nsBlockFrame tweak is just a special-case, to disable some code that would otherwise clamp the block to its min/max range, after the block has computed its auto-height. (This is important because we don't want the min/max-height properties to affect our intrinsic height calculation.) I think most other frame classes figure out their auto-height in their ComputeSize method, and Patch 6 already makes sure that min/max won't have an effect in that method, at least. But yeah, there are probably some other classes that do min/max checks in reflow that I missed. (e.g. I ran across a NS_CSS_MINMAX() call in nsHTMLButtonControlFrame, and that'd definitely need the same sort of special-case...) It's probably better if I just make mIsFlexContainerMeasuringHeight forcibly set mComputed[Min|Max]Height to their extremes, so that they won't have any effect. Then I won't need this special-case code for blocks, buttons, or anything else. I'll do that and drop the nsBlockFrame special-case. > (And if you're dealing with anything that uses > nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, should anything else be > ignored?) I don't think so (other than min-height / max-height, which patch 6 already ensures we'll ignore for vertical flex items). We're trying to compute the intrinsic height to get our flex basis, and our normal height-computing code should all be fine for that, as long as we skip the min/max clamping (since flexbox applies those constraints later on). > I'm also not sure what part of the spec explains what this patch is supposed > to do. The main reason for this patch is to give us a way of resolving "min-height: auto" into a value that we can use for section 4.5 "Implied Minimum Size of Flex Items": # auto # When used as the value of a flex item's min main # size property, this keyword indicates a minimum # of the min-content size, to help ensure that the # item is large enough to fit its contents. Bug 763689 is on supporting "min-height:auto" in general, but this patch lets us react to it appropriately *inside of a vertical flexbox* and resolve it to a reasonable value for the min-content height, for use in clamping when resolving flexible lengths. (For reference, see the clause where we set "mIsFlexContainerMeasuringHeight = true" in patch 7, which is inside a "isMainMinSizeAuto" check.) I also use this patch to compute the flex-basis when we've got "height:auto" in a vertical flexbox -- this corresponds to Section 9.2, step 3, final bullet-point. ("Otherwise, ...") http://dev.w3.org/csswg/css3-flexbox/#line-sizing (This is controlled by the "isMainSizeAuto" bool, in patch 7.) NOTE: fantasai tells me that the min-content and max-content height of an element (in the default writing-mode, the only one we support at the moment) are both simply the height of that element when it's reflowed into its available width, with unconstrained available height. (This value is _also_ the element's max-content height, incidentally.) I'm not sure offhand what spec text defines that, but I think it's in the writing-modes spec... In any case, that's the height-value that this patch tries to let us measure, regardless of what the "height" property is actually set to.
Comment on attachment 646904 [details] [diff] [review] patch 7 v2: main flexbox class impl ...continued from comment 91. DoesAxisGrowInPositiveDirection could probably be shortened to AxisGrowsInPositiveDirection PositionTracker should probably also declare a protected copy constructor. >+ // Largest offset from an item's cross-start margin-box edge to its >+ // baseline -- computed in ComputeLineCrossSize: >+ nscoord mCrossStartToFurthestBaseline; This (and the other baseline stuff) could use better comments about how it works when the cross-axis is vertical vs. horizontal. >+NS_IMETHODIMP >+nsFlexContainerFrame::Init(nsIContent* aContent, >+ nsIFrame* aParent, >+ nsIFrame* aPrevInFlow) >+{ >+ return nsFlexContainerFrameSuper::Init(aContent, aParent, aPrevInFlow); >+} This seems unnecessary. So I think the BuildDisplayList method of nsFlexContainerFrame is correct, but I think that *other* BuildDisplayList methods are going to need adjustment in two ways: (1) to act as block-level when they're flex items (2) to support z-index when they're flex items That said, BuildDisplayList should use nsFrameList::Enumerator rather than walking the frames directly. I think that nsFlexContainerFrame::SanityCheckAnonymousFlexItems is substantially incorrect given recent spec changes, although maybe I'm misremembering the spec changes. I believe, however, that the spec changed so that there are no longer any rules for coalescing things into a single anonymous flex item; you just get one for each element. (Or piece of text???) FreezeOrRestoreEachFlexibleSize should be static. >+ for (PRUint32 i = 0; i < aItems.Length(); i++) { maybe avoid calling aItems.Length() each time through the loop? Is it possible for ShouldUseFlexGrow to be called with aTotalFreeSpace being negative and there being zero items? If so, the abort will fire in that case (along with the wrong -- but probably irrelevant -- result). ... to be continued, again...
(In reply to Daniel Holbert [:dholbert] from comment #92) > (In reply to David Baron [:dbaron] from comment #88) > > Comment on attachment 646903 [details] [diff] [review] > > patch 6.5 v1: Add flag to nsHTMLReflowState & ComputeSize for measuiring [...] > > I'm wondering whether this patch is really sufficient -- do you need to > > handle more than blocks? [...] > It's probably better if I just make mIsFlexContainerMeasuringHeight forcibly > set mComputed[Min|Max]Height to their extremes, so that they won't have any > effect. Then I won't need this special-case code for blocks, buttons, or Here's an updated patch that does this (drops the nsBlockFrame special-case and instead sets mComputed[Min|Max]Height to their extremes). (This also includes the fix for the missing-paren issue brought up in comment 88)
Attachment #646903 - Attachment is obsolete: true
Attachment #646903 - Flags: review?(dbaron)
Attachment #650692 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] from comment #91) > Comment on attachment 646904 [details] [diff] [review] > patch 7 v2: main flexbox class impl > > >+ eAxis_LR = 0, // Start at 0 so we can use these values as array indices. > > I think you're ok leaving out both the =0 and the comment, though > I suppose it's ok. Yeah -- I just had the =0 there to be extra-explicit. Agreed that it's unnecessary, though -- dropped for simplicity. https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/6aa5b70542d6 > (The "For sizing arrays..." > comment at the end seems more useful, and should perhaps be > duplicated to the next enum.) Fixed this in the cset above, too^ > Probably better to return nscoord than const nscoord&. Yup, that's silly. Fixed (also the other spot you mentioned later on -- so no more "const nscoord&" in the patch at all): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/940e4c991083 > Then again, you could alternatively use the pattern: > nscoord MarginComponentForSide(const nsMargin& aMargin, Side aSide); > nscoord& MarginComponentForSide(nsMargin& aMargin, Side aSide); > instead of the getter/setter pattern. But the way you have it is > fine too. Sounds good -- I changed the static helper-functions at the top of the file to follow that pattern. (I left FlexItem's wrappers for those methods as being named Get/Set, though, since FlexItem uses the Get/Set pattern for its other accessors, and I also hardly ever need to call SetMarginComponentForSide, so I want to be explicit when I do intend to modify things). Fixed this as part of the cset above (940e4c991083). > FlexboxAxisTracker: > > Regarding the PhysicalPositionFromLogicalPosition and > PhysicalSizeFromLogicalSizes methods: I wonder if we should have types > for logical points and logical sizes. Possibly, yeah. Right now I largely handle each axis separately, though -- it's rare that I use both the main-position and the cross-position in the same chunk of code. So I think having them as separate nscoords (rather than bundled in a struct) makes a reasonable amount of sense for now -- I'll leave this as-is, unless you think I should change it. [side comment: I suppose "size" is a bit overloaded -- the flexbox spec & nsFlexContainerFrame.cpp use "size" to mean "a height or width", whereas Gecko often uses "size" it to mean a 2-dimensional nsSize. Perhaps I should change all my "nscoord mMainSize" etc to "mMainLength" to emphasize that I'm talking about a 1-dimensional length, not a 2-dimensional size...?) > SortableFrame: > > The GetFrame() method should probably be called Frame() instead > since it should be assumed to be non-null. (And maybe the constructor > should assert that the frame is non-null.) Fixed -- renamed to Frame() and added an assertion in the constructor. (I did the same for FlexItem::GetFrame(), too.) https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/f5ac93afcec4 > FlexItem: > > As before, silly to return |const nscoord &|. Yup, good point -- fixed, as noted above in this comment. (in 940e4c991083) > Would it make sense for any more of the setters to assert about > frozenness? Yeah -- some of them expect to be called before our main size is frozen, and some expect to be called after our main size is frozen, so they can all assert about frozen-ness. I've added assertions to make these expectations explicit: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c68d0664af7f (I also slightly tweaked how we set/clear the max/min-violation bits, since we only ever _set_ those bits on un-frozen items, but we may _clear_ them on items that have just been frozen. So now the setters unconditionally set its bit to "true" (and asserts that we're un-frozen), and there's a new explicit "clear" function that clears both bits (and doesn't assert)). > >+ // XXXdholbert Is it kosher to say we have unconstrained available width? > > No. [...] > Then again, this method looks unused. Can you remove it? Oops! Sorry about that. That's indeed dead code -- removed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/96da83fe845d > You should have a blank line after the end of > nsFlexContainerFrame::IsHorizontal(). Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c368929f0908 > >+ // Do an unconstrained-height reflow into our container's available space > >+ // to get an intrinsic height for this child. > > It's not really unconstrained height that's the key factor here, since > unconstrained height currently means "not paginating". The interesting > stuff is done by mIsFlexContainerMeasuringHeight combined with > patch 6.5. Right -- I meant to say "auto-height", not "unconstrained-height". I've now clarified that comment to mention mIsFlexContainerMeasuringHeight and auto-height instead: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/b07d882593ae > If you're passing NS_FRAME_NO_MOVE_FRAME, you should also pass > NS_FRAME_NO_MOVE_VIEW. That _VIEW bit is actually already included in NS_FRAME_NO_MOVE_FRAME: 19 #define NS_FRAME_NO_MOVE_FRAME (0x0002 | NS_FRAME_NO_MOVE_VIEW) http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.h#19 ... so I think this is good as-is. (I noticed that we do redundantly "OR" those bitfields together in a few other places, though -- I filed bug 781869 on fixing those.) > What behavior do we expect for percentage heights on children in this > auto-height computation pass -- particularly if we're computing an auto > min-height and there's a specified height? And what does this code do? The intended (and I believe actual) behavior is to compute the auto-height of the item (i.e. the height we'd get, if it had "height:auto" and the entire flex container's width as its available-width). We want to get the same outcome, regardless of what "height" is actually specified as (whether it's a percentage or pixel or whatever). Put another way: we're looking for the flex item's "min-content" height, and that shouldn't depend on what the "height" property is actually set to (just like the min-content width of an element doesn't depend on what its "width" property is set to)). I think this answers your question -- let me know if not though. > >+ // CROSS MIN/MAX SIZE > > For crossMinSize -- do you need to support min-height:auto for the cross > size as well? Nope, we don't need to worry about that. The "min-[size]: auto" keyword only needs to do something special in the flex container's _main axis_. In contrast, the cross-axis's "min-[size]:auto" keyword is just treated as 0 (like it is everywhere else). (The patches in bug 763689 will make it produce 0 in nsHTMLReflowState.mComputedMinHeight, which is why we can just directly use that value.) > FlexItem::FlexItem: > > - initializing to nscoord_MIN feels odd -- is this meaningful or can > you use 0 instead? With one exception (which I added a comment about), it's not meaningful -- I just had it as a debugging "not-yet-set-to-a-meaningful-value" marker, to more easily notice any bugs from forgetting to set things. I could even leave these values completely-uninitialized-until-we-compute-their-correct-values... but that scares me a bit, so I've just changed to initialize them to 0 instead: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/34f5a9365e34 > - in the #ifdef DEBUG block, you can pull styleMargin out of the loop, > but put an additional set of braces around it for scoping Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c1c1cde70e5e > >+ // Resolve "align-self: auto" to parent's "align-items" value. [...] > This isn't quite right -- you want the element parent, so you want > data from what you'd inherit style from. So instead, you want the > assignment to assign > mFrame->GetStyleContext()->GetParent()->GetStylePosition()->mAlignItems; Fixed, including the ua.css tweak we discussed for tables, to deal with their weird style inheritance: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/a5b9e56a9ce5 > FlexItem::GetNumAutoMarginsInAxis > - pull |styleMargin| out of the loop Fixed (and made the same change in a few other spots, too): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c7921869526b
In reply to David Baron [:dbaron] from comment #93) > Comment on attachment 646904 [details] [diff] [review] [diff] [review] > patch 7 v2: main flexbox class impl > > ...continued from comment 91. > > DoesAxisGrowInPositiveDirection could probably be shortened to > AxisGrowsInPositiveDirection Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/741635cfeebd > PositionTracker should probably also declare a protected copy > constructor. Good call. I've actually declared a private copy-constructor (not protected): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/66a4f1cad11e ...because "protected" still leaves the subclasses with working default copy-constructors. I don't want that, because I don't want any XXXPositionTracker instances to get accidentally copied -- they should instead be passed around by reference. So, to break their default copy constructors, I gave the parent class a private copy-constructor, and I confirmed that this prevents the following sample code from compiling (good): MainAxisPositionTracker tmp = mainAxisPosnTracker; > >+ // Largest offset from an item's cross-start margin-box edge to its > >+ // baseline -- computed in ComputeLineCrossSize: > >+ nscoord mCrossStartToFurthestBaseline; > > This (and the other baseline stuff) could use better comments about > how it works when the cross-axis is vertical vs. horizontal. Will do. I haven't addressed this yet, because I want to look back over this code. (I also need to finish off the related code that computes the baseline of a flex container itself -- the patch has an XXXdholbert note for that, I believe.) So -- I'll follow up on this. > >+NS_IMETHODIMP > >+nsFlexContainerFrame::Init(nsIContent* aContent, > >+ nsIFrame* aParent, > >+ nsIFrame* aPrevInFlow) > >+{ > >+ return nsFlexContainerFrameSuper::Init(aContent, aParent, aPrevInFlow); > >+} > > This seems unnecessary. Righto. (This method had more at some point in the past.) Removed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/216d312559fe > So I think the BuildDisplayList method of nsFlexContainerFrame is > correct, but I think that *other* BuildDisplayList methods are going > to need adjustment in two ways: > (1) to act as block-level when they're flex items > (2) to support z-index when they're flex items > > That said, BuildDisplayList should use nsFrameList::Enumerator rather > than walking the frames directly. BuildDisplayList is currently intentionally simple and wrong, because its behavior (and dependence or non-dependence on "order") was under discussion in www-style recently. Now that it's settled, we need to have our display-list built in order of the "order" property. (This wouldn't have had any effect until recently, though, because until Bug 772690 was closed, we had a subsequent re-sorting of the display list by content order, which would erase any special "order" ordering we'd done.) We ultimately might need to reorder the frame tree by "order", but we can't do that at the moment because tab-index depends on the frame-tree, and we don't want the "order" property to affect tab-index. So for now, I'll look into sorting the frames in BuildDisplayList, rather than reordering the frame tree. So -- I'll follow up on this. It might be worth leaving this simple (but with an enumerator) and doing the correct "order" ordering in a followup patch. (Also: once I've made the requisite changes about "what defines a flex item", then all the flex items will *be* block-level (due to the float-like transformation of the "display" property), so I suspect we'll get your part (1) above for free.) > I think that nsFlexContainerFrame::SanityCheckAnonymousFlexItems is > substantially incorrect given recent spec changes, although maybe > I'm misremembering the spec changes. Right -- "patch 2" on this bug, already landed, added our anonymous-flex-item-creation code in nsCSSFrameConstructor, to coalesce runs of inline content into anonymous flex items. That code *was* correct per spec when it landed, but the spec has changed (as you noted), and I intend to update to the new spec behavior in a separate patch, probably layering on top of Patch 7. (Patch 7 is conceptually about laying out the flex items we're given, which is conceptually separate from how to create flex items.) Anyway -- I added a comment noting that SanityCheckAnonymousFlexItems will need to change, once we've fixed the anonymous flex item creation: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/2af66fa25203 > spec changed so that there are no longer any rules for coalescing > things into a single anonymous flex item; you just get one for each > element. (Or piece of text???) Basically, yeah. The new spec behavior is: - Child elements are supposed to directly form flex items (and their "display" value is supposed to compute to its block-ish form, applying the same transformation that "float" applies to the "display" property). - Contiguous runs of text get wrapped in an anonymous flexbox item. (but we don't create any anonymous flexbox items for all-whitespace runs of text) > FreezeOrRestoreEachFlexibleSize should be static. Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/17c9a3ac9f2b > >+ for (PRUint32 i = 0; i < aItems.Length(); i++) { > > maybe avoid calling aItems.Length() each time through the loop? I think the existing code -- iterating using Length() in the loop condition -- is a pretty frequent code-pattern in Gecko for nsTArrays, since the nsTArray Length() method is a one-liner ("return mHdr->mLength") and will almost certainly be inlined. So I don't think there's any benefit from adding an extra variable to reduce Length() calls -- happy to do so if you think it's worth it, though. > Is it possible for ShouldUseFlexGrow to be called with aTotalFreeSpace > being negative and there being zero items? If so, the abort > will fire in that case (along with the wrong -- but probably irrelevant > -- result). Nope, that can't happen. ShouldUseFlexGrow() is called by ResolveFlexibleLengths(), which returns early if there are no flex items.
Comment on attachment 646904 [details] [diff] [review] patch 7 v2: main flexbox class impl ... continued from comment 91 and comment 93 So a few thoughts on things that should be followups after the initial landing: (1) I think it's pretty important to optimize for the case of only one thing being flexible. I think one reasonably common use case will be using flexbox for layouts not unlike a browser window -- a bunch of stuff in the middle taking most of the space, with some UI around the edges. In these cases, we'll want to make sure to skip the intrinsic sizing work for the piece in the middle when it's avoidable. (2) We should make sure 'overflow' works correctly on flex containers and flex items. This is likely to take a bit of work. In ResolveFlexibleLengths: >+ // To avoid rounding issues, we subtract our current flex ratio & >+ // our sizeDelta from the totals (so that at the end, the remaining >+ // free space gets entirely assigned to the final entry, with no >+ // rounding issues.) It's perhaps worth being clear that what really solves the rounding issues is the combination of the subtraction from availableFreeSpace and the use of idxOfLastNonzeroFlexRatio. sumOfFlexRatios being a float means you don't have the accuracy there, and thus all the rounding error goes to the last item. What would avoid that, I think, is to store each floating point sum as you go along (presumably in the FlexItem), and then distribute in reverse order using those sums. Or, even, rather than the sum, store the ratio of the curFlexRatio to sumOfFlexRatios as you go along, and then distribute in reverse order. I'm not sure if it's worth adding another float to FlexItem to do it that way, though it would avoid the need for idxOfLastNonzeroFlexRatio. >+ availableFreeSpace -= sizeDelta; There's an extra space before the -=. >+ NS_ASSERTION(sumOfFlexRatios >= 0.0f, >+ "total flex shouldn't be allowed to drop below 0"); >+ } >+ NS_ABORT_IF_FALSE(sumOfFlexRatios == 0.0f || >+ !NS_finite(sumOfFlexRatios), >+ "Shouldn't have any flexibility left over"); What about accumulation error? Can't that bring it below zero? ... to be continued, starting from BuildSortedChildArray
Blocks: 782440
Blocks: 782441
Flags: sec-review+
(In reply to David Baron [:dbaron] from comment #97) > So a few thoughts on things that should be followups after the initial > landing: Filed bug 782440 and bug 782441 on these. > In ResolveFlexibleLengths: [...] > What would avoid that, I think, is to store each floating point sum as > you go along (presumably in the FlexItem), and then distribute in > reverse order using those sums. Or, even, rather than the sum, store > the ratio of the curFlexRatio to sumOfFlexRatios as you go along, and > then distribute in reverse order. I'm not sure if it's worth adding > another float to FlexItem to do it that way, though it would avoid the > need for idxOfLastNonzeroFlexRatio. I like that! That does remove the need for idxOfLastNonzeroFlexRatio, and it reduces the bad-effects of float accumulation error. I went ahead and added another float member-var "mShareOfFlexWeightSoFar" to FlexItem to handle this. I did this in a patch, here: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/raw-file/ae8f9ad40f5f/flexbox-recordRatioOfWeightVsRunningTotal.patch ...which I folded that patch into the main patch, here: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c938d68396fd NOTE: Since this variable and a few others aren't used after we've resolved the flexible widths, we could use a union{} to share memory with other vars that are *only* used after that point. I haven't made that union-izing change yet, to keep things simple for now, but that's one optimization we could do (and I added a comment near this member-var's declaration, suggesting it). (Also -- before making that change, I renamed all uses of "flex ratio", "flex grow ratio", etc. to "flex weight", in the spirit of your www-style email from a week or so back. I did this because I initially was going to call the new value a "ratio". I ended up calling it "share" instead, but I think the code is still clearer with the existing "ratio" stuff renamed to "weight".) https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/63f6894d00df I also extended the flexbox layout mochitest (previously landed in comment 89) to support custom flex-container sizes, so that we can use container-sizes that make accumulation error easier to see & test. I've added some testcases to that mochitest that test accumulation error, which I'll post in a subsequent patch here. > >+ availableFreeSpace -= sizeDelta; > > There's an extra space before the -=. Thanks, fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/4ab3ff964526 > >+ NS_ASSERTION(sumOfFlexRatios >= 0.0f, > >+ "total flex shouldn't be allowed to drop below 0"); > >+ } > >+ NS_ABORT_IF_FALSE(sumOfFlexRatios == 0.0f || > >+ !NS_finite(sumOfFlexRatios), > >+ "Shouldn't have any flexibility left over"); > > What about accumulation error? Can't that bring it below zero? Hmm... it might've been able to. In any case, this assertion and the potential problem it's checking for are now gone, since I'm now using precomputed (and non-negative) "mShareOfFlexWeightSoFar" values, instead of chipping away at sumOfFlexRatios. (as noted earlier in this comment)
Here's the extension to the test_flexbox_layout.html mochitest that I alluded to in the previous comment. This patch makes the following changes: - Adds support for an optional "container_properties" hash-entry in each testcase, with a hash of properties to apply to the flex container. (see documentation at the top of lexbox_layout_testcases.js) - Creates a helper-method to perform the logic in "test_flexbox_layout.html" for setting a possibly-aliased property, so that we can use it to set properties on flex items and on the flex container itself. - Adds some testcases with custom-sized huge containers, to test float accumulation error. (and to let us detect if our behavior changes)
Attachment #652248 - Flags: review?(dbaron)
Blocks: 783409
Blocks: 783415
Comment on attachment 641820 [details] [diff] [review] patch 99: enable flexbox in builds (obsoleting the "enable flexbox in builds" patch, as I've now filed a dedicated bug for that -- bug 783409)
Attachment #641820 - Attachment is obsolete: true
Blocks: 783470
Blocks: 783474
(In reply to Daniel Holbert [:dholbert] from comment #96) > > This (and the other baseline stuff) could use better comments about > > how it works when the cross-axis is vertical vs. horizontal. > > Will do. I haven't addressed this yet, because I want to look back over this > code. (I also need to finish off the related code that computes the baseline > of a flex container itself -- the patch has an XXXdholbert note for that, I > believe.) So -- I'll follow up on this. FWIW, I partly fixed this up -- now, for any FlexItem instances that have flex-align: baseline in a vertical flexbox, I set mAlignSelf to the FLEX_START enum instead of the BASELINE one, in the FlexItem() constructor. I do this because the spec says that "baseline" should be identical to "flex-start" whenever the cross axis is the same as the inline axis. (and that's true for all flex items in a vertical flexbox, at least until we support "writing-modes") (I also ran across & fixed a few other bugs with vertical flexboxes while fixing/testing this, too.) Updated patch v7 coming in a minute, with these fixes and all the other fixes I've mentioned in review-responses above. > BuildDisplayList is currently intentionally simple and wrong [...] > So -- I'll follow up on this. I filed bug 783474 on this. > > spec changed so that there are no longer any rules for coalescing > > things into a single anonymous flex item; you just get one for each > > element. (Or piece of text???) > > Basically, yeah. The new spec behavior is: > - Child elements are supposed to directly form flex items (and their > "display" value is supposed to compute to its block-ish form, applying the > same transformation that "float" applies to the "display" property). > - Contiguous runs of text get wrapped in an anonymous flexbox item. (but > we don't create any anonymous flexbox items for all-whitespace runs of text) I filed bug 783415 on the implementation side of this spec-change.
Attachment #646904 - Attachment is obsolete: true
Attachment #646904 - Flags: review?(dbaron)
Attachment #652848 - Flags: review?(dbaron)
Comment on attachment 652848 [details] [diff] [review] patch 7 v3: main flexbox class impl ... continued from comment 91 and comment 93 and comment 97, but now reviewing https://bug666041.bugzilla.mozilla.org/attachment.cgi?id=652848 MainAxisPositionTracker constructor: Maybe assert that aReflowState.frame == aFlexContainerFrame? >+ // Step over flex container's own main-start border/padding. >+ EnterMargin(aReflowState.mComputedBorderPadding); Maybe leave a comment here that you should check GetSkipSides() once you start paginating (although I'm sure there are a bunch of other places where I didn't notice this) >+ // XXXdholbert ComputedHeight might be unconstrained...? Seems like you need to do something about this. ResolveAutoMarginsInMainAxis: Pull styleMargin out of the loop (but not the if). >+ nscoord curAutoMarginSize = (mNumAutoMarginsInMainAxis == 1) ? >+ mPackingSpaceRemaining : >+ NSToCoordRound(float(mPackingSpaceRemaining) / >+ mNumAutoMarginsInMainAxis); I don't think there's any reason to do float math here. Yes, integer math will skew the distribution of the extra appunits towards the end, but I don't think that's a problem. TraversePackingSpace: Same comment about float math. >+ // (This is a warning, not an assertion, because it can fire in the valid >+ // case where we only have 1 app unit to divide between 2 packing spaces.) >+ NS_ABORT_IF_FALSE(mPackingSpaceRemaining >= 0, >+ "ran out of packing space earlier than we expected"); Sure doesn't look like a warning. Then again, if you change the float thing it won't be a problem any more.
(In reply to David Baron [:dbaron] from comment #103) Comment 103's notes are addressed in this patch-queue cset: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/00e7575e8ab0 > MainAxisPositionTracker constructor: > > Maybe assert that aReflowState.frame == aFlexContainerFrame? Done. (incidentally, I changed all NS_ASSERTION / NS_ABORT_IF_FALSE to MOZ_ASSERT, too, in a separate patch-queue cset) > >+ // Step over flex container's own main-start border/padding. > >+ EnterMargin(aReflowState.mComputedBorderPadding); > > Maybe leave a comment here that you should check GetSkipSides() Done. > >+ // XXXdholbert ComputedHeight might be unconstrained...? > > Seems like you need to do something about this. Oops, thanks! Fixed to just use 0 packing space in that case (since that means we're just shrinkwrapping). > ResolveAutoMarginsInMainAxis: > Pull styleMargin out of the loop (but not the if). Fixed. > I don't think there's any reason to do float math here. Good point -- fixed. (Added comment to acknowledge the skew from integer math, too.) (I also fixed this in one other spot -- in the MainAxisPositionTracker constructor, when we determine how much leading-edge packing space to step over for "justify-content: space-around". That tweak is in this cset: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/35152679b00a#l1.49 ) > TraversePackingSpace: > > Same comment about float math. Fixed. > Sure doesn't look like a warning. Ha, good point. :) Sorry about that. > Then again, if you change the float thing it won't be a problem any > more. Right -- removed the comment, and kept the assertion (since integer math will bias us towards putting space at the end, so we won't run out as long as we've got a remaining spot for packing-space).
Comment on attachment 652848 [details] [diff] [review] patch 7 v3: main flexbox class impl ... continued from comment 91 and comment 93 and comment 97 and comment 103 SingleLineCrossAxisPositionTracker I think the GetBaselineOffsetFromCrossEnd function should probably be eliminated -- the whole point of the function is to produce the height minus the baseline offset from the start. It seems simpler to eliminate all the code in that function, pull |curOuterCrossSize| outside the if at the caller, and just subtract at the caller. (It also means that if the code is wrong somehow, the failure will be potential misalignment, rather than missizing and potential misalignment.) Given the way you set mAlignSelf in FlexItem::FlexItem, you don't need to handle RTL and LTR yet -- you should probably put in an assertion that you don't get called in those cases, along with a FIXME to fix it when we have vertical text support. (Probably want the fixme in FlexItem::FlexItem for that as well.) Likewise for bottom-to-top, and the FlexboxAxisTracker constructor; just assert that there is no bottom-to-top in the cross axis. (Again, you don't handle it now, but you don't assert either.) In ResolveAutoMarginsInCrossAxis, it seems like you could subtract GetMarginBorderPaddingSizeInAxis instead of GetBorderPaddingSizeInAxis right at the beginning, and avoid having to subtract the margins and do the extra <=0 check later. In EnterAlignPackingSpace, the comment should change from flex-item-align to align-self. So ComputeFlexContainerMainSize is a little odd. I wonder if it really ought to be running during reflow. I realize that normally, right now, we can have NS_UNCONSTRAINEDSIZE as the dimension going into reflow for the Y axis but not for the X axis. But I wonder if that makes sense for flexbox -- I wonder if it would make more sense in the long run for this to run inside ComputeAutoSize. The problem with that, however, is that you'd need a way to preserve all of these temporary data structures between ComputeAutoSize (which may or may not run) and reflow. I guess it's probably ok as it is, although I wonder if you're correctly handling min/max-height on vertical flexboxes with auto heights -- maybe you need to make ComputeFlexContainerMainSize consider them? FlexItem::Set{Main,Cross}Position should have clearer documentation that they're *content-box* positions, since positions are normally border-box. Same for the getters. And same for the Sizes, actually. >+ // Invalidate child's old overflow rect (greedy; once the >+ // display-list-based invalidation patches have landed, this can go away.) >+ aItem.Frame()->InvalidateOverflowRect(); Probably want the string "FIXME" here. It feels odd to me that the reflow code lives in SizeItemInCrossAxis. Doesn't that make sense only when the cross axis is vertical? Or is it because that's the last dimension in which you compute the size? If so, maybe explain with a comment? + // If we need to do baseline-alignment, store the child's ascent. + if (aItem.GetAlignSelf() == NS_STYLE_ALIGN_ITEMS_BASELINE) { + // Cribbed from nsLineLayout::PlaceFrame + // This is a little hacky. Basically: we'll trust our child on its + // GetBaseline(), but not for display:block. Blocks are supposed to use + // the baseline of the first line, but nsBlockFrame::GetBaseline() + // returns the baseline of the last line (which is appropriate for + // inline-block), so we can't use GetBaseline() for display:block. + if (childDesiredSize.ascent == nsHTMLReflowMetrics::ASK_FOR_BASELINE) { + if (NS_STYLE_DISPLAY_BLOCK == + aItem.Frame()->GetStyleDisplay()->mDisplay) { + if (!nsLayoutUtils::GetFirstLineBaseline(aItem.Frame(), + &childDesiredSize.ascent)) { + childDesiredSize.ascent = childDesiredSize.height; + } + } else { + childDesiredSize.ascent = aItem.Frame()->GetBaseline(); + } + } + aItem.SetAscent(childDesiredSize.ascent); + } Why not just use nsLayoutUtils::GetFirstLineBaseline unconditionally, and then call frame->GetBaseline() if it returns false? (I'm particularly skeptical that the NS_STYLE_DISPLAY_BLOCK test is correct.) PositionItemInCrossAxis: >+ "per-line cross-axis posiiton tracker wasn't correctly reset"); posiiton -> position nsFlexContainerFrame::Reflow: >+ // For simplicity, we'll treat NS_SUBTREE_DIRTY(this) as a signal that this >+ // frame and all its children need to be reflowed. (Technically, we might >+ // only need to reflow _some_ children; but the ones that don't need a reflow >+ // can figure that out themselves, since their own NS_FRAME_IS_DIRTY bit >+ // won't be set.) >+ bool shouldReflowAllChildren = >+ NS_SUBTREE_DIRTY(this) || aReflowState.ShouldReflowAllKids(); I think this could use a better explanation. Basically, nsFlexContainerFrame is different from many other frame classes because of the way that one of its children can influence the size of the others -- it can do so in arbitrary ways, not just by moving later children. So it needs to run its full reflow algorithm even if only some of the children are dirty. I think you should rename the variable from shouldReflowAllChildren to shouldReflowChildren, to match the way it's used. (Normally, such variables mean that all children should be reflowed *as opposed to* just the dirty ones.) Also, I'm not sure nsHTMLReflowState::ShouldReflowAllKids is sufficient here. It does tests based on mHResize and mVResize -- but are those correct here? I think you should set NS_FRAME_CONTAINS_RELATIVE_HEIGHT on all nsFlexContainerFrames so that it is. (It would also be good to have tests showing a failure to resize child frames in a case like this.) >+ // For a horizontal single-line flex container, min width is: >+ // sum(child min widths) Probably just drop this comment in both GetMinWidth and GetPrefWidth (since it only covers the horizontal case, and since the code is pretty self-explanatory). >+ // XXXdholbert Optimization: We could cache our intrinsic widths like >+ // nsBlockFrame does (and return it early from this function if it's set). >+ // Whenever anything happens that might change it, set it to >+ // NS_INTRINSIC_WIDTH_UNKNOWN (like nsBlockFrame::MarkIntrinsicWidthsDirty >+ // does) I think you should probably do this -- although at this point I've lost the control flow through which a flex container can depend on the result of GetMinWidth/GetPrefWidth on its child. That happens in a bunch of cases, though, doesn't it? (If it doesn't, then it may not be worth doing this optimization. But I'd be rather puzzled if it doesn't.) I'm a little puzzled by the change in nsHTMLReflowState::InitConstraints to check if the parent is a flex container frame before honoring the flag that they set. Maybe, instead, you need to set this flag explicitly in the nsHTMLReflowState constructor that takes a parent reflow state rather than just copying the value from the parent? (I'm guessing the code adding this flag is in another patch... not sure where right now.) >- // Exclude inline tables from the block margin calculations >- if (isBlock && !IsSideCaption(frame, mStyleDisplay) && >- frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE) >+ // Exclude inline tables and flex items from the block margin calculations >+ if (isBlock && >+ !IsSideCaption(frame, mStyleDisplay) && >+ frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE >+#ifdef MOZ_FLEXBOX >+ && !flexContainerFrame >+#endif // MOZ_FLEXBOX >+ ) This could probably be done using a test of the 'display' value. (And the inline-table check should probably use mStyleDisplay rather than getting frame->GetStyleDisplay() all over again.) r=dbaron given those things are addressed reasonably (including some of the ones I mentioned should be in followups, which can happen later, but probably before we enable this)
Attachment #652848 - Flags: review?(dbaron) → review+
Blocks: 785468
Blocks: 734525
Comment on attachment 646893 [details] [diff] [review] patch 5 v2a: use 'flex-basis' in place of 'width' or 'height' in ComputeSize Landed patch 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/e919c8cdb667
Attachment #646893 - Flags: checkin+
Comment on attachment 646896 [details] [diff] [review] patch 6 v2: ignore min/max-main-size property in ComputeSize Landed patch 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/4959a10d90aa
Attachment #646896 - Flags: checkin+
Comment on attachment 650692 [details] [diff] [review] patch 6.5 v2: Add flag to nsHTMLReflowState & ComputeSize for measuring auto height Still need to look into: nsFrame::ComputeSize -- what about the check just below? (There are other ways result.height can end up not being NS_UNCONSTRAINEDSIZE, via ComputeAutoHeight.) What happens with the ComputeSize implementations that use ComputeSizeWithIntrinsicDimensions
(In reply to Daniel Holbert [:dholbert] from comment #92) > (In reply to David Baron [:dbaron] from comment #88) > > Comment on attachment 646903 [details] [diff] [review] > > (And if you're dealing with anything that uses > > nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, should anything else be > > ignored?) > > I don't think so (other than min-height / max-height, which patch 6 already > ensures we'll ignore for vertical flex items). We're trying to compute the > intrinsic height to get our flex basis, and our normal height-computing code > should all be fine for that, as long as we skip the min/max clamping (since > flexbox applies those constraints later on). So the issue here is that nsLayoutUtils::ComputeSizeWithIntrinsicDimensions looks at the {,min-,max-}{width,height} properties, so your code to ignore those properties still won't apply to the frame classes that use it -- and you probably want to ignore all of them. My gut feeling here is that you probably want to abstract what calls this code into a method on nsIFrame, and then override that method on the same classes that call ComputeSizeWithIntrinsicDimensions, since for those classes, you don't need to do any Reflow or ComputeSize -- you just need to return the intrinsic height, which you already have. But it's not clear to me whether that's what the spec calls for. (In particular, it's clear that you have to ignore the *height properties -- but do you also ignore the *width properties. Ignoring *height without ignoring *width would be a bit harder, for images.) > NOTE: fantasai tells me that the min-content and max-content height of an > element (in the default writing-mode, the only one we support at the moment) > are both simply the height of that element when it's reflowed into its > available width, with unconstrained available height. (This value is _also_ > the element's max-content height, incidentally.) I'm not sure offhand what > spec text defines that, but I think it's in the writing-modes spec... In > any case, that's the height-value that this patch tries to let us measure, > regardless of what the "height" property is actually set to. That still doesn't say whether the specified width is ignored (which, given that definition, makes a difference both for blocks and for images). (It's clear that the specified height is ignored.) I think we need to make sure the spec is clear here; I'm not sure whether it is now.
Comment on attachment 650692 [details] [diff] [review] patch 6.5 v2: Add flag to nsHTMLReflowState & ComputeSize for measuring auto height >diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp > // Compute height > >- if (!nsLayoutUtils::IsAutoHeight(*heightStyleCoord, aCBSize.height)) { >+ if (!nsLayoutUtils::IsAutoHeight(*heightStyleCoord, aCBSize.height) && >+ !(aFlags & nsIFrame::eUseAutoHeight)) { > result.height = > nsLayoutUtils::ComputeHeightValue(aCBSize.height, *heightStyleCoord) - > boxSizingAdjust.height; > } > > if (result.height != NS_UNCONSTRAINEDSIZE) { I think the flag should probably also prevent you from entering this clause (the if in the last line quoted). (Form controls and some other similar things can have a non-NS_UNCONSTRAINEDSIZE result.height at this point because of the result of ComputeAutoSize.) See also the comment above. And the patch could also use a commit message. Once those two issues are addressed (the bigger one being the one in the previous comment), I think this is good. But I'm also ok with you landing this without the one in the previous comment fixed if you file a bug to address that one later (blocking the bug on enabling flexbox), so I'm marking it r=dbaron. But we do need to figure out what the right thing to do for that is.
Attachment #650692 - Flags: review?(dbaron) → review+
Comment on attachment 641818 [details] [diff] [review] patch 8: Treat min-width "auto" as "min-content" in the places we care about it for flex items Shouldn't the nsComputedDOMStyle code also check IsHorizontal? (I think I'm happy with the getComputedStyle result here being a keyword rather than a number. That's also what we do for explicit 'min-content', except on 'width' when there's a frame and calcWidth is true... which I admit is an odd inconsistency.) r=dbaron with that, but it would be good to have a test for both the layout and computed style aspects here
Attachment #641818 - Flags: review?(dbaron) → review+
Comment on attachment 652248 [details] [diff] [review] patch 4.5: add some testcases that trigger float accumulation error to test_flexbox_layout.html Landed patch 4.5 (float accumulation error testcases for mochitest): https://hg.mozilla.org/integration/mozilla-inbound/rev/a32dd4305ba8
Attachment #652248 - Flags: checkin+
Depends on: 790903
Blocks: 791037
No longer depends on: 790903
Blocks: 794267
(In reply to David Baron [:dbaron] from comment #105) > Comment on attachment 652848 [details] [diff] [review] > SingleLineCrossAxisPositionTracker > > I think the GetBaselineOffsetFromCrossEnd function should > probably be eliminated... > It seems simpler to eliminate all the code in that function, pull > |curOuterCrossSize| outside the if at the caller, and just subtract > at the caller. Eliminated GetBaselineOffsetFromCrossEnd here: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/5dbd97d6d010 and pulled curOuterCrossSize out & simplified the arithmetic here (with a pretty diagram now): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/a1ba9670073a > Given the way you set mAlignSelf in FlexItem::FlexItem, you > don't need to handle RTL and LTR yet -- you should probably put > in an assertion that you don't get called in those cases, along > with a FIXME to fix it when we have vertical text support. > (Probably want the fixme in FlexItem::FlexItem for that as well.) > Likewise for bottom-to-top, Added assertion that we're only expecting top-to-bottom (not RTL, not LTR, not bottom-to-top) in 5dbd97d6d010 (linked above), and added FIXMEs in: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/1a76d847bfc6 > and the FlexboxAxisTracker constructor; > just assert that there is no bottom-to-top in the cross axis. > (Again, you don't handle it now, but you don't assert either.) Added that assertion: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/42bf311bea3b Also: I initially thought we couldn't have a RTL cross-axis without writing-modes (and I included that assumption in that^ cset's assertion), but upon re-reading the spec, I realized that I do have to allow RTL cross-axes, for vertical flexbox with "direction: rtl". (Previously, I was ignoring the "direction" property for the cross axis, but I should be honoring it). To support that without adding too much complexity, I revamped the FlexboxAxisTracker constructor to: (A) first, determine the "inlineDimension" and the "blockDimension" (based on "direction" & eventually "writing-modes") (B) Then, determine our flex container's axes based on those combined with the "flex-direction" property. The logic is also set up such that any (highly-unexpected) bogus enum-values will leave us with standard LTR/Top-to-bottom. I also fixed a bug in PositionItemInCrossAxis() that was triggered with a (now-supported) RTL cross-axis. I folded this RTL-cross-axis work into the patch in this cset: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/diff/70779db4fb87/flexbox-mainImpl.patch > In ResolveAutoMarginsInCrossAxis, it seems like you could > subtract GetMarginBorderPaddingSizeInAxis instead of > GetBorderPaddingSizeInAxis right at the beginning, and avoid > having to subtract the margins and do the extra <=0 check later. That's true! That code was there before I was sure that auto-margins would be initialized to 0 in the reflow-state -- I was explicitly skipping over them under the assumption that they might have bogus values. But now I've got checks to ensure that they're 0, so we should be fine adding them. Anyway -- I made that change, and I replaced the remainder of that loop with a GetNumAutoMarginsInAxis() call, since that's all the remaining code did: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/d637043c2c12 > In EnterAlignPackingSpace, the comment should change from > flex-item-align to align-self. Fixed (in a couple other places as well): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/cdec8bbd44ff > So ComputeFlexContainerMainSize is a little odd. I wonder if it really > ought to be running during reflow. I realize that normally, right now, > we can have NS_UNCONSTRAINEDSIZE as the dimension going into reflow for > the Y axis but not for the X axis. But I wonder if that makes sense for > flexbox -- I wonder if it would make more sense in the long run for this > to run inside ComputeAutoSize. The problem with that, however, is that > you'd need a way to preserve all of these temporary data structures > between ComputeAutoSize (which may or may not run) and reflow. > I guess it's probably ok as it is Yeah -- we'd have to do everything up until that point during ComputeAutoSize, including some dummy-reflowing of auto-height child items, to get their flex base size in a vertical flexbox. I don't think (?) we want to be doing any reflowing (including dummy reflows) during ComputeAutoSize. So -- because of that (and per your "I guess it's probably ok"), I won't worry about this for now. > although I wonder if you're correctly > handling min/max-height on vertical flexboxes with auto heights -- maybe > you need to make ComputeFlexContainerMainSize consider them? Ooh, absolutely right -- fixed ComputeFlexContainerMainSize to consider min/max heights: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/8a8536f2e9fb and added reftests for [min|max]-[width|height] on a variety of auto-sized flex containers: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/04a226711bb0#l2.2 > FlexItem::Set{Main,Cross}Position should have clearer documentation > that they're *content-box* positions, since positions are normally > border-box. Same for the getters. And same for the Sizes, actually. Added documentation: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/32887919ea38 > >+ // Invalidate child's old overflow rect (greedy; once the > >+ // display-list-based invalidation patches have landed, this can go away.) > >+ aItem.Frame()->InvalidateOverflowRect(); > > Probably want the string "FIXME" here. Added FIXME (& tweaked existing comment): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/3a025cdc3a77 > It feels odd to me that the reflow code lives in SizeItemInCrossAxis. (To be clear, this isn't actually "the" reflow code -- it's just a tentative reflow, to let us measure the cross-size & baseline of the child.) > Doesn't that make sense only when the cross axis is vertical? Or is > it because that's the last dimension in which you compute the size? > If so, maybe explain with a comment? You're right -- we only need this reflow when the cross axis is vertical. When the cross axis is horizontal, we can just use reflowstate.ComputedWidth() as the cross-size, and we won't need the baseline. I added an early-return "easy" clause for horizontal cross-axis (i.e. for vertical flexbox), to skip this reflow: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/bf07abd33c4b > + // If we need to do baseline-alignment, store the child's ascent. [...] > + if (childDesiredSize.ascent == nsHTMLReflowMetrics::ASK_FOR_BASELINE) { > + if (NS_STYLE_DISPLAY_BLOCK == > + aItem.Frame()->GetStyleDisplay()->mDisplay) { > + if (!nsLayoutUtils::GetFirstLineBaseline(aItem.Frame(), > + &childDesiredSize.ascent)) > { > + childDesiredSize.ascent = childDesiredSize.height; > + } > + } else { > + childDesiredSize.ascent = aItem.Frame()->GetBaseline(); [...] > Why not just use nsLayoutUtils::GetFirstLineBaseline unconditionally, > and then call frame->GetBaseline() if it returns false? > > (I'm particularly skeptical that the NS_STYLE_DISPLAY_BLOCK test > is correct.) IIRC, I had the "DISPLAY_BLOCK" check in there as a hack to distinguish "block" from "inline-block", which (in an older version of the spec) were both supported as flex items, with different baselines. But yeah, that shouldn't matter anymore since inline-block is now supposed to end up being a block. (which will happen in bug 783415) Anyway -- as you suggested, I removed that "DISPLAY_BLOCK" check, and now I just use GetFirstLineBaseline followed by GetBaseline if that fails: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/a317d38942e3#l1.2 > >+ "per-line cross-axis posiiton tracker wasn't correctly reset"); > > posiiton -> position Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/586bf56828c3 > nsFlexContainerFrame::Reflow: > > > >+ // For simplicity, we'll treat NS_SUBTREE_DIRTY(this) as a signal that this > >+ // frame and all its children need to be reflowed [...] > >+ bool shouldReflowAllChildren = > >+ NS_SUBTREE_DIRTY(this) || aReflowState.ShouldReflowAllKids(); > > I think this could use a better explanation. > > Basically, nsFlexContainerFrame is different from many other frame > classes because of the way that one of its children can influence the > size of the others Rewrote that comment, with a better explanation along those lines: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/7b52bebdf9ab > I think you should rename the variable from shouldReflowAllChildren > to shouldReflowChildren, to match the way it's used (Fixed, as part of prev. cset) > Also, I'm not sure nsHTMLReflowState::ShouldReflowAllKids is sufficient > here. It does tests based on mHResize and mVResize -- but are those > correct here? I think you should set NS_FRAME_CONTAINS_RELATIVE_HEIGHT > on all nsFlexContainerFrames so that it is. (It would also be good > to have tests showing a failure to resize child frames in a case like > this.) You're right, we do need NS_CONTAINS_RELATIVE_HEIGHT in some cases. We only need it if *we* (the flex container) are dependent on an ancestor's height, though -- i.e. if we have a percent height. (We might arguably need to set that flag on all flex items, too, but I don't think it matters, since the flex container greedily reflows its children when its size changes anyway.) Fixed to set the flag if our flex container has a percent height: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/76d6baf5d859 and added a test that failed before but passes now (using an iframe whose size changes): https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/f636e0dcb8ef > >+ // For a horizontal single-line flex container, min width is: > >+ // sum(child min widths) > > Probably just drop this comment in both GetMinWidth and GetPrefWidth > (since it only covers the horizontal case, and since the code is > pretty self-explanatory). Fixed: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/8eaa959e559c > >+ // XXXdholbert Optimization: We could cache our intrinsic widths like > >+ // nsBlockFrame does (and return it early from this function if it's set). > >+ // Whenever anything happens that might change it, set it to > >+ // NS_INTRINSIC_WIDTH_UNKNOWN (like nsBlockFrame::MarkIntrinsicWidthsDirty > >+ // does) > > I think you should probably do this -- although at this point I've > lost the control flow through which a flex container can depend on > the result of GetMinWidth/GetPrefWidth on its child. That happens > in a bunch of cases, though, doesn't it? (If it doesn't, then it > may not be worth doing this optimization. But I'd be rather puzzled > if it doesn't.) (Regarding the control flow -- IIRC, a flex container's own GetPrefWidth()/GetMinWidth() methods can determine its reflowState's mComputedWidth (e.g. if it's floated, has "width: -moz-min-content", etc). So its children's GetPrefWidth() / GetMinWidth() methods end up indirectly determining that as well, since their called by the flex container's GetPrefWidth / GetMinWidth.) Anyway -- I filed a followup on this optimization -- Bug 794267 -- since it's an optimization rather than a functional necessity. > I'm a little puzzled by the change in nsHTMLReflowState::InitConstraints > to check if the parent is a flex container frame before honoring the > flag that they set. That change was a (very minor) optimization. Basically: we're *already* adding a check for whether the parent is a flex container, to see whether we should shrinkwrap in ComputeSize. We also happen to know that the (existing) flag "mIsFlexContainerMeasuringHeight" will only ever be set on children of flex containers. So, we might as well shift the flag-check inside of that "is parent a flexbox" clause, so we won't bother checking it for non-flex-items. I've now added an explicit "else" clause (for the "parent not a flexbox" case), to hopefully make this clearer -- the else clause asserts that the flag is *unset* (and hence it's fine that we're ignoring it) https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/8613903f5817 > Maybe, instead, you need to set this flag > explicitly in the nsHTMLReflowState constructor that takes a parent > reflow state rather than just copying the value from the parent? We don't copy that flag from the parent -- it's explicitly initialized to false, and then our flex container frame explicitly turns it on, if it wants to measure our auto-height. (That happens in the patch that adds this flag, patch 6.5. (attachment 650692 [details] [diff] [review])) > (I'm guessing the code adding this flag is in another patch... not > sure where right now.) (Yup, see above.) > >- // Exclude inline tables from the block margin calculations > >- if (isBlock && !IsSideCaption(frame, mStyleDisplay) && > >- frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE) > >+ // Exclude inline tables and flex items from the block margin calculations > >+ if (isBlock && > >+ !IsSideCaption(frame, mStyleDisplay) && > >+ frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE > >+#ifdef MOZ_FLEXBOX > >+ && !flexContainerFrame > >+#endif // MOZ_FLEXBOX > >+ ) > > This could probably be done using a test of the 'display' value. The "display" value doesn't give us what we need there -- we want to check whether our _parent_ is a flex container frame, not whether _we_ are a flex container frame. (We could of course check the parent's "display" value, but it's easier to just check |flexContainer| which we've already set to a non-null value IFF our parent is a flex container.) > (And the inline-table check should probably use mStyleDisplay > rather than getting frame->GetStyleDisplay() all over again.) Cool -- made that change: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/0be91e2050b5 > r=dbaron given those things are addressed reasonably (including some > of the ones I mentioned should be in followups, which can happen > later, but probably before we enable this) Thanks! Updated patch attached, carrying forward r+.
Attachment #652848 - Attachment is obsolete: true
Attachment #665098 - Flags: review+
Blocks: 794660
(In reply to David Baron [:dbaron] from comment #110) > Comment on attachment 650692 [details] [diff] [review] > patch 6.5 v2: Add flag to nsHTMLReflowState & ComputeSize for measuring auto > height > > Still need to look into: > > nsFrame::ComputeSize -- what about the check just below? The check below the modified chunk there is specifically about applying min/max sizes, and that'll all get skipped over if we're a vertical flex-item (which we are, if we're explicitly looking up the auto-height with eUseAutoHeight.) > What happens with the ComputeSize implementations that use > ComputeSizeWithIntrinsicDimensions Ah -- currently, nothing special happens, and that's wrong. With the current set of patches, we'd honor the "height" property (or "flex-base" property, if it's set) in ComputeSizeWithIntrinsicDimensions, and we'll end up using the resulting value for e.g. "min-height: auto". I think we probably *want* to pass down "nsIFrame::eUseAutoHeight" to ComputeSizeWithIntrinsicDimensions. Filed followup bug 794660 on that. (I also added some known-failing reftests to my flexbox-reftests patch, to test this situation -- those will be fixed by that followup bug.)
Blocks: 794748
(In reply to David Baron [:dbaron] from comment #113) > Comment on attachment 641818 [details] [diff] [review] > patch 8: Treat min-width "auto" as "min-content" in the places we care about > it for flex items > > Shouldn't the nsComputedDOMStyle code also check IsHorizontal? Yup -- fixed that a few weeks ago, but hadn't reposted yet: https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/bdec6bc3f68d#l1.59 > (I think I'm happy with the getComputedStyle result here being a keyword > rather than a number. That's also what we do for explicit 'min-content', > except on 'width' when there's a frame and calcWidth is true... which I > admit is an odd inconsistency.) (ok, cool) > r=dbaron with that, but it would be good to have a test for both the layout > and computed style aspects here I've got reftests for the layout effects of "min-width:auto" in a flexbox. I spun off bug 794748 on adding a mochitest for the computed-style aspect.
Blocks: 795087
Landed: Patch 6.5: https://hg.mozilla.org/integration/mozilla-inbound/rev/129629b6106f Patch 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/076d87bf30d0 Patch 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/de6a5c46a8ff Reftests: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a231dc22347 I also made two minor changes to patch 7 before landing: - Removed Invalidate() calls, since DLBI landed in the last day or so - Removed #ifdef MOZ_FLEXBOX around the flexbox frame id in nsFrameIdList.h, since now there's some un-#ifdef-guarded code that relies on "do_QueryFrame()" tests for whether we're a flexbox frame. (That was needed for non-MOZ_FLEXBOX-enabled builds to successfully compile) Note that this is still disabled by default at build-time. The patch in bug 783409 enables it in builds.
Whiteboard: [leave open]
awesome! nice work, everyone! and it even seems to work with columns! (bug 734525)
Summary: CSS Flexbox Layout Level 3 → CSS Flexbox Layout Level 3 (disabled in builds by default)
Should this bug depend or block bug #529761?
No -- that bug is about "display: -moz-box", which has a completely different implementation.
Blocks: 796212
Blocks: 797022
No longer depends on: 692465
FWIW, this is enabled in builds now on mozilla-inbound. (woot!) A few of the dynamic tests are marked as failing on android, IIRC due to a few fuzzy pixels from subpixel antialiasing outside of the font's reported bounds. These annotations were apparently out of date, though, as there are 3 unexpected passes and 1 unexpected fail (with just a few px of difference in the failure). Updated the fails-if(Android) annotations to address to address these: (the fails-if could probably be a fuzzy-if -- I can sort that out on TryServer later, but for now I just want to fix the orange)
(In reply to Daniel Holbert [:dholbert] from comment #124) > Updated the fails-if(Android) annotations to address to address these: Forgot to paste in the cset: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdf4f53d3cb
Depends on: 797953
I'm getting Warning: Unknown property '-moz-align-self'. Declaration dropped. Source File: resource://gre-resources/ua.css Line: 44 from the change in ua.css. Is this expected/known?
Depends on: 798592
I wasn't explicitly expecting that, but that makes sense, yeah. Shouldn't cause problems, but I'm also no fan of needless warning-spam, so I filed bug 798592 on fixing it (using @supports). If anyone wants to take that bug, feel free. :) If not, I'll probably fix it in the next week or so.
Blocks: 798666
Depends on: 799647
Depends on: 799699
Depends on: 799713
Blocks: 799775
Blocks: 799305
Blocks: 799207
Blocks: 798235
Depends on: 801268
Blocks: 804089
Blocks: 807897
Blocks: 808767
Is there planned support for the flex-wrap property? http://www.w3.org/TR/css3-flexbox/#flex-wrap-property I just ran a test and it doesn't look like it's supported as of Nightly 19.0a1 (2012-11-09): http://codepen.io/doctyper/pen/ipqsH In Chrome Stable, each child takes up 100% of the container width, and each child wraps accordingly.
(In reply to Richard Herrera from comment #129) > Is there planned support for the flex-wrap property? > http://www.w3.org/TR/css3-flexbox/#flex-wrap-property According to http://dxr.mozilla.org/mozilla-central/layout/generic/nsFlexContainerFrame.cpp.html#l1736 there is: // FIXME: Once we support "flex-wrap", check if it's "wrap-reverse" // here to determine whether we should reverse mCrossAxis. which might be bug 702508 but I'm not certain.
Yup, flex-wrap is not yet implemented, and is covered by bug 702508.
Blocks: 811024
Blocks: 812822
Blocks: 819536
Having real docs for this stuff is wonderful; thanks Frederic! Worthy of a hacks.mozilla.org post?
(In reply to Dan Mosedale (:dmose) from comment #133) > Worthy of a hacks.mozilla.org post? I think so. I'll ping Robert (in charge of hacks).
Blocks: 821449
Blocks: 821426
Blocks: 821775
Blocks: 821778
No longer blocks: 821775
Depends on: 821775
No longer blocks: 821778
Blocks: 826149
Blocks: 824297
Blocks: 827168
Blocks: 828651
Blocks: 841827
Blocks: 841847
Blocks: 798592
No longer depends on: 798592
Depends on: 854263
In March 29th Nightly, "min-height" and "min-width" turned from green to red at http://css3test.com/, reducing the "Flexible Box Layout" score from 80% to 67%.
(In reply to Markus Popp from comment #135) > In March 29th Nightly, "min-height" and "min-width" turned from green to red > at http://css3test.com/, reducing the "Flexible Box Layout" score from 80% > to 67%. Probably because of bug 848539, which was to update to a recent spec change (so css3test.com is presumably out of date).
Yup, dbaron is right. (If you click the red "min-width" and "min-height", it expands to show the failing test, which is named "auto". And "auto" is no longer a valid value for those keywords, as described/implemented in bug 848539.)
(I filed https://github.com/LeaVerou/css3test/issues/39 on fixing the issue w/ css3test.com, FWIW)
(In reply to Daniel Holbert [:dholbert] from comment #138) > (I filed https://github.com/LeaVerou/css3test/issues/39 on fixing the issue > w/ css3test.com, FWIW) Thanks! I also sent the author of css3test.com a tweet to let her know: https://twitter.com/mpopp75/status/317731522289496064
Blocks: 858332
Depends on: 874718
Depends on: 876985
Blocks: 880104
Daniel, are you aware of #529761? The bugs are not linked yet... Congrats on having this enabled by default!! I'm deeply impressed by the incredible epicness of your work here. This is big, big step for the web, IMO. Thanks...
Marc, that bug is about XUL boxes, not flexboxes, right?
Aw shucks! Boris, you're right, it is. However the latest comment there bug 529761, comment 15 states: "Tested it with FF22 released today, still buggy". Maybe this is already tracked in a different bug here. But I'm unable to find something like that. Just wanted to bring this to your attention, but I haven't been able to test the issue myself. I hope I'm not creating any confusion. :-/ (...Also you should probably change the name of this bug as flexbox is now enabled.)
Summary: CSS Flexbox Layout Level 3 (disabled in builds by default) → CSS Flexbox Layout Level 3 (disabled at build-time, enabled in a subsequent bug)
Commented there. Thanks, Marc!
Blocks: 921522
Blocks: 939901
Blocks: 943459
Blocks: 947820
Blocks: 963247
Blocks: 981135
Blocks: 848539
Blocks: 984711
Blocks: 985242
Depends on: 985304
dholbert, is the subsequent bug for enabling this already filed?
Flags: needinfo?(dholbert)
Yup! This is 100% enabled in release builds, and has been for some time. The most recent history regarding that: - The about:config pref was turned off in release builds in bug 841873 - The pref was then enabled for release builds in bug 841876, - We removed the pref entirely in bug 936100, after it had been default-enabled for a while.
Flags: needinfo?(dholbert)
Whiteboard: [enabled in release builds since landing of bug 841876]
(And for completeness' sake: bug 797022 is where we turned on the build flag (before enabling the pref); and we removed the #ifdefs for this build flag in bug 864553)
Whiteboard: [enabled in release builds since landing of bug 841876] → [enabled in builds in bug 797022, preffed on everywhere in bug 841876]
Awesome; thanks for the info!
Blocks: 1000957
Blocks: 1032922
Blocks: 1088165
Depends on: 1486086
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: