Daniel Holbert [:dholbert] Assignee
	Description • 12 years ago

Per
  http://dev.w3.org/csswg/css3-flexbox/#min-size-auto
we should now make the initial value for min-width and min-height be "auto" now.

Quoting that chunk of spec:
{{
  To provide a more reasonable default minimum size for flex containers, this specification introduces a new ‘auto’ value as the initial value of the ‘min-width’ and ‘min-height’ properties defined in CSS 2.1.

[...]

  On a flex item, this keyword indicates a minimum size of the min-content size.

  It is intended that this will compute to the ‘min-content’ keyword when the specification defining it (Writing Modes Appendix D) is sufficiently mature.

  On any other element, this keyword computes to ‘0’ (unless otherwise defined by a future specification). 
}}

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

This patch makes min-height and min-width accept the "auto" keyword, and it makes that their initial value.

For now, auto always computes to "0" for these properties.  (though one of the flexbox patches will add an "if (parent is a flexbox) { compute to -min-content keyword }" instead.

This passed a local run of the /layout/style mochitests.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

Comment on attachment 632039 [details] [diff] [review]
fix v1

r=me

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/ff0658329dbd

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

This needs a bit more, actually.  Reopening.

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

So, to expand on the previous comment -- we actually need the initial value of nsStylePosition::mMinWidth to be "auto", and we need to handle that value correctly wherever we encounter it when reading it (treating it as '0', generally).

It's a bit tedious / fragile to have these special-cases everywhere, though, so I don't really like this patch.  I'll post another comment with another idea.

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 12 years ago

So RE all the special-cases in patch 2 -- I suspect there's a way to do this more automagically.

I think we could shift mMinWidth (and other related properties whose computed-style depends on the parent's computed 'display') into a new style-struct.  This wouldn't follow our existing "reset" vs "inherited" struct designations -- instead, it'd be flagged as "depends on parent 'display'", with e.g. a keyword like:
  COMPUTE_START_DEPENDS_ON_PARENT_DISPLAY()
instead of
  COMPUTE_START_RESET() / COMPUTE_START_INHERITED()

That would ensure that we have access to its parent "display" computed style and can set our property's computed style accordingly, in nsRuleNode. (e.g. min-width:auto producing a nsStyleCoord of length-0 vs. enumerated-moz-min-content.)

(Side note: "display" itself will actually fall into the COMPUTE_START_DEPENDS_ON_PARENT_DISPLAY category, too.  That's because a recent flexbox spec-change says that all children of a flexbox will need to have their 'display' values compute to blockified forms of themselves (inline-table --> table, etc), as we would if they were floated / positioned.  In order to do this, we need access to the parent 'display' value when we're computing our own 'display' value.)

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

And for all other purposes (e.g. caching storage) it would be treated as an inherited struct?

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

I think so, yeah.

More specifically -- if we group this class of properties (min-width, display, others later on) into a single struct "nsStyleFoo", then I think we could treat that struct as if it were inherited, except for initialization -- a child's nsStyleFoo wouldn't be initialized as a copy of the parent's.  (That happens here for inherited structs:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#2287
)

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

FWIW, I've changed my mind about comment 6 -- there aren't too many special-cases, and at this point it's simpler to just handle "min-width/height:auto" as a special-case when it's read rather than to try to automagically convert out of it in style-computation.

(dbaron suggested something along the same lines -- special-casing whenever it's read -- at the end of bug 756647 comment 3)

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

NOTE: In theory, this bug's patches shouldn't affect our behavior, since min-width:auto is supposed to behave just like min-width:0.

However, patch 2 (for min-width) and the upcoming patch 3 (for min-height) *do* actually affect a few of our crashtests in situations with bogus, extremely large border/padding values, because I skip a few calls (e.g. to nsLayoutUtils::ComputeWidthValue) where we _should_ end up producing 0, but bogus negative border/padding values can influence us to end up picking a different (possibly huge) min-width value.

This sort of bogus situation can happen if we have "padding: nscoord_Max".  Then, when callers pass nsSize(padding.LeftRight(),padding.TopBottom()) into nsFrame::ComputeSize() as the "aPadding" arg, the LeftRight() and TopBottom() calls will both produce nscoord_Max + nscoord_Max, which is INT_MIN (a negative value), which is weird because padding is supposed to be nonnegative.  This can mess with our min-width/min-height computations via boxSizingAdjust (which is subtracted from our min-width/min-height values -- and subtracting INT_MIN makes our min values enormous).

So -- this bug's patches will affect behavior (and tweak crashtest assertion-counts) for a few cases like that, but I claim those cases don't matter, and the new behavior is no weirder than our existing behavior.

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

NeilAway says that XUL assigns a special significance to the (old default) min-width / min-height value of 0.  It treats 0 as something min-content-ish instead of actually treating it as 0.  This lets XUL solve the same sort of problem that css3-flexbox is trying to solve here.  And then XUL content can specify "min-width: 0%" (with a %) if it _really_ wants a min-width of 0.

So, in light of that:
 (a) We need to make sure that the new default (auto) still triggers the min-content-ish behavior in XUL, like the old default (0) did.
 (b) We might want to fix XUL so that "min-width: 0" actually means 0 now instead of "min-content-ish"

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

Here's a cleaned-up increased-lines-of-context version of patch 2 (enabling "min-width:auto" as the default in computed style, and adding special-cases for treating it as "0").

This version adds one more tweak over the previously-posted version, at the end of the patch -- it makes "WidthCoordDependsOnContainer()" only check for 'auto' in its "width"-property-specific function, rather than checking in the helper that's shared between the min-width/max-width functions.  (We want this change because "min-width: auto" does _not_ depend on its container.)

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 12 years ago

...and here's the corresponding patch for handling "min-height:auto".

(This tweaks the crashtest.list assertion-counts for a few tests that fire nscoord_MAX assertions, as noted in comment 11).

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

(In reply to David Baron [:dbaron] from comment #93)
> 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

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 16 • 12 years ago

(Sorry, disregard previous comment -- that was intended for bug 666041. Pasted responses into wrong bugpage, d'oh.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 17 • 12 years ago

Comment on attachment 650658 [details] [diff] [review]
patch 2 v2 (handle min-width:auto)

Seems like pretty much everything in this patch will need to be fixed for flex items in a later patch.

>   bool MinWidthDependsOnContainer() const
>     { return WidthCoordDependsOnContainer(mMinWidth); }

You probably want to add a comment here that this isn't valid for flex items (if that isn't already in said later patch).

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 18 • 12 years ago

Comment on attachment 650659 [details] [diff] [review]
patch 3 v1 (handle min-height:auto)

r=dbaron, with the same comments as the previous patch

	Daniel Holbert [:dholbert] Assignee
	Comment 19 • 12 years ago

(In reply to David Baron [:dbaron] from comment #17)
> Comment on attachment 650658 [details] [diff] [review]
> patch 2 v2 (handle min-width:auto)
> 
> Seems like pretty much everything in this patch will need to be fixed for
> flex items in a later patch.
[...]
> You probably want to add a comment here that this isn't valid for flex items
> (if that isn't already in said later patch).

Actually, we don't have to worry about this -- we're lucky enough to _not_ need special-cases for flex items.  That's because a flex item's min-size property is supposed to be ignored (i.e. "treated as 0") up until the point where the flex container explicitly applies it, during width distribution.

(Bug 666041 patch 8 adds the code for that min-size-ignoring, in the places where it counts, FWIW.)

I've updated the patches to include more extensive comments to explain this, and I'll land them in the next day or so.

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

Landed patches 2 & 3 (with comments added per comment 19):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb37077d615
  https://hg.mozilla.org/integration/mozilla-inbound/rev/82f73bdb2237

Note: The landed version of patch 3 includes one new check (for which dbaron gave me a verbal rubber-stamp), in nsHTMLReflowState::ComputeMinMaxValues, to make us treat "min-height:auto" as "0" there. IIRC, that tweak wasn't needed before bug 776265 landed, but it's needed now. (or else we end up failing an assertion from passing "auto" into a chunk of code that doesn't expect it.)

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/6bb37077d615
https://hg.mozilla.org/mozilla-central/rev/82f73bdb2237

	Jean-Yves Perrier [:teoli]
	Comment 22 • 12 years ago

I've updated:
https://developer.mozilla.org/en-US/docs/CSS/min-width
https://developer.mozilla.org/en-US/docs/CSS/min-height
https://developer.mozilla.org/en-US/docs/Firefox_16_for_developers (for the auto value)
https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers (for the auto value as the initial value)

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

So this change made it so transitioning between an unspecified min-width and a specified one that's a length or percentage no longer works...

I'm not sure there's a good solution to that, unfortunately.

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

I think we could resolve comment 24 by adding some special-case code in nsStyleAnimation::ExtractComputedValue(), in a min-width/min-height special-case.

If we have access to the parent's style (which I think we do, via aStyleContext->GetParent()...?) then we could effectively resolve the "min-width" to either 0 or min-content right then and there.

Or, if I'm missing something & we don't have access to the parent's style for some reason, then I think I'd be OK just unconditionally converting min-width/min-height:auto to 0 there.

(If we ended up needing to go the latter route, it'd mean transitions on min-width between "auto" and specified values in a flex container would snap down to min-width:0 and move smoothly from there, instead of refusing to interpolate -- but I thing that's edge-casey and already-non-functional enough that it doesn't really worry me.)

Having said that, it sounds like the min-width:auto keyword might be removed from the spec entirely, so it may not be worth addressing comment 24 yet. Reference:
 http://lists.w3.org/Archives/Public/www-style/2013Feb/0364.html

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

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Having said that, it sounds like the min-width:auto keyword might be removed
> from the spec entirely, so it may not be worth addressing comment 24 yet.
> Reference:
>  http://lists.w3.org/Archives/Public/www-style/2013Feb/0364.html

Looks like this spec-change is moving forward, so I filed bug 848539 on reacting to it (and effectively backing this bug here out.)