Closed Bug 1174553 Opened 9 years ago Closed 9 years ago

[css-grid] Implement the 'min-content' / 'max-content' sizing functions for Grid layout

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

Attachments

(5 files, 4 obsolete files)

part 1 - Add a new flag (BAIL_IF_REFLOW_NEEDED) for IntrinsicForWM() that makes it return early with a NS_INTRINSIC_WIDTH_UNKNOWN result if a reflow is needed to determine the child's correct BSize
2.49 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 2 - Move the AddPercents function into a static method in the nsLayoutUtils class
6.27 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 3 - [css-grid] Implement the 'min-content' / 'max-content' sizing functions in layout
5.30 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 4 - add INFINITE_ISIZE_COORD and suppress warnings (addendum to part 3)
11.22 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 5 - [css-grid] Replace the ambiguous Dimension with LogicalAxis
14.59 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
Note that this code is inside a "#if 0" block for now to avoid
an "unused function" compilation warning.  It will be removed
when we start using it in bug 1151212.

This is the last part - no tests for now, those will also come
in bug 1151212.
Attachment #8622133 - Flags: review?(dholbert)
Minor update: nsLayoutUtils::IntrinsicForWM was renamed to IntrinsicForAxis
and now takes a PhysicalAxis instead of a WritingMode (bug 1174450).
Attachment #8622133 - Attachment is obsolete: true
Attachment #8622133 - Flags: review?(dholbert)
Attachment #8622710 - Flags: review?(dholbert)
Attached patch addendum to part 3 (obsolete) — — Splinter Review 
Looks like CRAZY_COORD is DEBUG-only so I'm fixing that here by introducing
a local constant INFINITE_ISIZE_COORD instead, and then opting out of the
"crazy width" layout warnings for grid container children.

(I'll fold this into part 3 before landing.)
Attachment #8625185 - Flags: review?(dholbert)
Comment on attachment 8622131 [details] [diff] [review]
part 1 - Add a new flag (BAIL_IF_REFLOW_NEEDED) for IntrinsicForWM() that makes it return early with a NS_INTRINSIC_WIDTH_UNKNOWN result if a reflow is needed to determine the child's correct BSize

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -4541,16 +4541,19 @@ nsLayoutUtils::IntrinsicForWM(WritingMod
>   } else if (!styleISize.ConvertsToLength() &&
>              !(haveFixedMinISize && haveFixedMaxISize && maxISize <= minISize)) {
> #ifdef DEBUG_INTRINSIC_WIDTH
>     ++gNoiseIndent;
> #endif
>     if (ourWM.IsOrthogonalTo(aWM)) {
>       // We need aFrame's block-dir size, which will become its inline-size
>       // contribution in the container.
>+      if (aFlags & BAIL_IF_REFLOW_NEEDED) {
>+        return NS_INTRINSIC_WIDTH_UNKNOWN;
>+      }

(Note: this has bitrotted slightly, but I don't think it actually affects the logic here. In my tree, the IsOrthogonal check is now "if (MOZ_UNLIKELY(aAxis != ourInlineAxis))", which is equvialent because the only PhysicalAxis options are horizontal or vertical.)

This early-return seems perhaps overly pessimistic -- if this frame has a specified BSize, then I'd expect we *could* resolve & return that here, right?  (Technically we don't "need reflow".)  Similar for cases where we have an intrinsic ratio & a specified width, for example.

Maybe add an XXX comment to this effect.

r=me
Attachment #8622131 - Flags: review?(dholbert) → review+
Comment on attachment 8622132 [details] [diff] [review]
part 2 - Move the AddPercents function into a static method in the nsLayoutUtils class

>+++ b/layout/base/nsLayoutUtils.h
>+  static nscoord AddPercents(IntrinsicISizeType aType, nscoord aCurrent,
>+                             float aPercent)
>+  {
>+    nscoord result = aCurrent;
>+    if (aPercent > 0.0f && aType == nsLayoutUtils::PREF_ISIZE) {
>+      // XXX Should we also consider percentages for min widths, up to a
>+      // limit?
>+      result = MOZ_UNLIKELY(aPercent >= 1.0f) ? nscoord_MAX
>+        : NSToCoordRound(float(result) / (1.0f - aPercent));
>+    }
>+    return result;
>+  }

As long as you're refactoring this a bit: IMO this would be simpler if we just get rid of the |result| local variable, and have two separate return statements here.

r=me regardless
Attachment #8622132 - Flags: review?(dholbert) → review+
Also (for part 2): now that AddPercents is becoming a public method on nsLayoutUtils, please add some documentation for it.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> This early-return seems perhaps overly pessimistic -- if this frame has a
> specified BSize, then I'd expect we *could* resolve & return that here,
> right?

No, I think this is covered by the condition for this block:

  } else if (!styleISize.ConvertsToLength() &&
             !(haveFixedMinISize && haveFixedMaxISize && maxISize <= minISize)) {

The "styleISize" name is perhaps a bit misleading since it's the isize for
the given aAxis, not for aFrame's WM.

I tried this with the following two tests and neither cause a reflow
when calculating the max-content size (styleISize.ConvertsToLength()
is true):

<div style="display:grid; grid-auto-rows:20px; grid-auto-columns:minmax(0,max-content);">
<span style="width:88px; writing-mode:vertical-lr;"></span>
</div>

<div style="display:grid; grid-auto-columns:20px; grid-auto-rows:minmax(0,max-content);">
<span style="height:88px; writing-mode:hozontal-tb;"></span>
</div>

(removing 'width'/'height' respectively makes us take the reflow branch though)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> As long as you're refactoring this a bit: IMO this would be simpler if we
> just get rid of the |result| local variable, and have two separate return
> statements here.

Will do.  Not sure what I should say in the comment though that isn't already
better said by the code itself. ;-)
(In reply to Mats Palmgren (on vacation) (:mats) from comment #9)
> No, I think this is covered by the condition for this block:

You're right - thanks.

> The "styleISize" name is perhaps a bit misleading since it's the isize for
> the given aAxis, not for aFrame's WM.

(Yeah, that's what confused me.)
(In reply to Mats Palmgren (on vacation) (:mats) from comment #10)
> (In reply to Daniel Holbert [:dholbert] from comment #7)
> > As long as you're refactoring this a bit: IMO this would be simpler if we
> > just get rid of the |result| local variable, and have two separate return
> > statements here.
> 
> Will do.

Thanks!

> Not sure what I should say in the comment though that isn't already
> better said by the code itself. ;-)

Just an English description of how the function is meant to be used/interpreted. ("AddPercents" is a vague name -- e.g. it sounds like it might be a function that takes two percentages and adds them. It takes some staring at the function to figure out exactly what it's meant to be used for.)

Maybe something like:
  // This function increases an initial intrinsic size, 'aCurrent', according
  // to the passed-in aPercent, such that the size-increase makes up exactly
  // aPercent of the returned value.
  // (We don't increase the size if MIN_ISIZE is passed in, though.)
Depends on: 1174546
Comment on attachment 8622710 [details] [diff] [review]
part 3 - [css-grid] Implement the 'min-content' / 'max-content' sizing functions in layout

Review of attachment 8622710 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +1280,5 @@
> +                   nsLayoutUtils::BAIL_IF_REFLOW_NEEDED);
> +  if (size == NS_INTRINSIC_WIDTH_UNKNOWN) {
> +    // We need to reflow the child to find its BSize contribution.
> +    WritingMode wm = aChild->GetWritingMode();
> +    nsContainerFrame* parent = aChild->GetParent();

We're assuming |parent| is the grid frame here, I think.

Maybe worth asserting that parent->GetType() == nsGkAtoms::gridContainerFrame, to be sure we're invoking this method correctly (on grid items)?

@@ +1291,5 @@
> +                               nsHTMLReflowState::DUMMY_PARENT_REFLOW_STATE);
> +    }
> +    // XXX this will give mostly correct results for now (until bug 1174569).
> +    nscoord i = CRAZY_COORD - 1; // largest isize we can use without asserting
> +    LogicalSize availableSize(wm, i, NS_UNCONSTRAINEDSIZE);

It looks like you're trying to do a reflow with available bsize & isize both being unconstrained, yes? (but avoid the assertions about unconstrained isize)

The CRAZY_COORD - 1 hack doesn't seem like a great long-term solution. Does the "until bug 1174569" comment mean we'll be removing this hack at that point?

(Alternately: if we need to do this sort of reflow, perhaps we should just soften the unconstrained-isize assertions in question?  (or make them conditional on some flag in the reflow state))

@@ +1295,5 @@
> +    LogicalSize availableSize(wm, i, NS_UNCONSTRAINEDSIZE);
> +    const nsHTMLReflowState* rs =
> +      aReflowState ? aReflowState : dummyParentState.ptr();
> +    LogicalSize childCBSize(wm);
> +    nsHTMLReflowState childRS(pc, *rs, aChild, availableSize, &childCBSize);

Do we really need to create & pass in childCBSize here? That nsHTMLReflowState constructor-arg is an optional parameter, and it's documented as being "used by absolute positioning code to override default containing block sizes".  (And that doesn't seem to apply here.)

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h#640

@@ +1298,5 @@
> +    LogicalSize childCBSize(wm);
> +    nsHTMLReflowState childRS(pc, *rs, aChild, availableSize, &childCBSize);
> +    if (!aReflowState) {
> +      parent->AddStateBits(NS_FRAME_IN_REFLOW);
> +    }

Just before you add this bit, you should probably assert that parent does *not* currently have the bit set

(If it *does* have this bit set, then your RemoveStateBits lower down will be clearing something that we didn't set, which would be bad.)

@@ +1302,5 @@
> +    }
> +    nsHTMLReflowMetrics childSize(childRS);
> +    nsReflowStatus childStatus;
> +    parent->ReflowChild(aChild, pc, childSize, childRS, wm,
> +                        LogicalPoint(wm), 0, 0, childStatus);

A few things, based on my recollections about measuring ReflowChild calls in nsFlexContainerFrame:
 (1) You should pass in the flag NS_FRAME_NO_MOVE_FRAME for this call (since you don't *really* want the child frame to be moved to your dummy LogicalPoint(wm) position).

 (2) I'm pretty sure your ReflowChild() calls here need to be followed by a call to FinishReflowChild(), for robustness, or else you can end up with assertions or weird intermediate states. (I ran into this for flexbox in bug 804089, and added calls to DidReflow after measuring reflows there -- but that turned out to be insufficient in bug 821426, at which point I upgraded to use FinishReflowChild.)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Maybe worth asserting that parent->GetType() ==
> nsGkAtoms::gridContainerFrame, to be sure we're invoking this method
> correctly (on grid items)?

This function (ContentContribution) would actually work perfectly
fine and give correct results for arbitrary frames.  I think I'll
keep it that way in case we need it elsewhere in the future.

> The CRAZY_COORD - 1 hack doesn't seem like a great long-term solution. Does
> the "until bug 1174569" comment mean we'll be removing this hack at that
> point?

No, it will stay.  We need to reflow with an "infinite" available size
according to the spec (css-sizing).  (the "addendum" patch renames it
INFINITE_ISIZE_COORD)

> (Alternately: if we need to do this sort of reflow, perhaps we should just
> soften the unconstrained-isize assertions in question?  (or make them
> conditional on some flag in the reflow state))

I made exceptions specifically for grid items in the "addendum" patch
since it's only used here so far.  I can make it a reflow state flag if
you prefer that.  Note that the added exceptions are in DEBUG-only so
that might be an argument against a reflow state flag.

> > +    nsHTMLReflowState childRS(pc, *rs, aChild, availableSize, &childCBSize);
> 
> Do we really need to create & pass in childCBSize here?

No, that seems unnecessary - removed.

> > +      parent->AddStateBits(NS_FRAME_IN_REFLOW);
> Just before you add this bit, you should probably assert that parent does
> *not* currently have the bit set

Fixed.

> > +    parent->ReflowChild(aChild, pc, childSize, childRS, wm,
> > +                        LogicalPoint(wm), 0, 0, childStatus);
>  (1) You should pass in the flag NS_FRAME_NO_MOVE_FRAME for this call

Good point, fixed.  I added NS_FRAME_NO_SIZE_VIEW too for good measure.

>  (2) I'm pretty sure your ReflowChild() calls here need to be followed by a
> call to FinishReflowChild()

Oops, good catch.  Fixed.
Attachment #8622710 - Attachment is obsolete: true
Attachment #8622710 - Flags: review?(dholbert)
Attachment #8648025 - Flags: review?(dholbert)
(In reply to Mats Palmgren (:mats) from comment #14)
> > The CRAZY_COORD - 1 hack [...]
> 
> No, it will stay.  We need to reflow with an "infinite" available size
> according to the spec (css-sizing).  (the "addendum" patch renames it
> INFINITE_ISIZE_COORD)
[...]
> I made exceptions specifically for grid items in the "addendum" patch
> since it's only used here so far.  I can make it a reflow state flag if
> you prefer that.  Note that the added exceptions are in DEBUG-only so
> that might be an argument against a reflow state flag.

Yeah, I'd prefer a reflow state flag -- or a more explicitly-defined constant (like your INFINITE_ISIZE_COORD), defined somewhere outside of grid (maybe alongside NS_UNCONSTRAINEDSIZE), and whose value probably shouldn't depend on CRAZY_COORD.

CRAZY_COORD is a special thing only used for assertions, I think; so, relying on it for actual sizing behavior (i.e. passing CRAZY_COORD-1 as an actual size) seems unnecessarily hacky. Rather than bending the code to work around the existing CRAZY_* assertions, we should do what makes sense for the code, and adjust the CRAZY_* assertions to accommodate, IMO.

I'd suggest defining INFINITE_ISIZE_COORD as (NS_MAXSIZE - 1) in nsIFrame.h, just below where we define NS_UNCONSTRAINEDSIZE, to make it more obvious that we're (hopefully) not overlapping with another reserved sentinel-value.  And if this trips a CRAZY_COORD/CRAZY_SIZE assertion or two, then we should adjust those assertions to allow for INFINITE_ISIZE_COORD as an exception.
(...oh, maybe we can't *directly* check for INFINITE_ISIZE_COORD in these assertions, though -- because we'll have subtracted off some border/padding/etc., and we'll be left with an available isize that's a bit smaller than INFINITE_ISIZE_COORD.  That's probably why you opted to skip assertions based on the frame type instead of the size in the addendum patch.)
Comment on attachment 8625185 [details] [diff] [review]
addendum to part 3

>+++ b/layout/generic/nsBlockFrame.cpp
> #ifdef DEBUG_blocks
>-  if (CRAZY_SIZE(aMetrics.Width()) || CRAZY_SIZE(aMetrics.Height())) {
>+  if ((CRAZY_SIZE(aMetrics.Width()) || CRAZY_SIZE(aMetrics.Height())) &&
>+      GetParent()->GetType() != nsGkAtoms::gridContainerFrame) {
>     ListTag(stdout);
>     printf(": WARNING: desired:%d,%d\n", aMetrics.Width(), aMetrics.Height());

So, I'm not psyched about these GetType()-based exemptions in block layout, for two reasons:
 (1) They disable these warnings for grid items, *for the normal reflow path* (when we're not inside this intrinsic sizing code). To the extent that we care about keeping these warnings, we should keep them for grid items (during normal reflow).

 (2) They're a little cryptic -- it's unclear at each spot where we have these why grid-ness is relevant to the thing we're asserting about.


I think we could address both of these concerns by setting some temporary, usefully-named state on the grid frame, probably via frame property. (and only in debug builds)

Something like:
 // [somewhere, maybe in nsContainerFrame.h:]
 NS_DECLARE_FRAME_PROPERTY(ReflowingWithInfiniteISize, nullptr)

 // Just before we do this reflow, in ContentContribution:
 parent->Properties().Set(ReflowingWithInfiniteISize(), 1);

 //...and then afterwards that reflow:
 parent->Properties().Remove(ReflowingWithInfiniteISize(), 1);

And then in places where we want to check whether we should opt out of a CRAZY_COORD assertion, we check whether that property is set on the parent frame, instead of checking the frame type.

(I haven't actually tested that this ^ syntax compiles; I'm just semi-cribbing from other frame-property usages in MXR.)
Comment on attachment 8648025 [details] [diff] [review]
part 3 - [css-grid] Implement the 'min-content' / 'max-content' sizing functions in layout

Review of attachment 8648025 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3 (disregarding the CRAZY_COORD-1 thing since that'll be addressed in the addendum), with nits below addressed:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1266,5 @@
> +/**
> + * Return the margin-box contribution of aChild in aDimension.
> + */
> +static nscoord
> +ContentContribution(nsIFrame*                         aChild,

s/Return the/Return the [min|max]-content/ or something to that effect (mentioning min-content/max-content)

(Right now, the documentation says "margin-box contribution", whereas the function name is "ContentContribution" -- and this superficially seems like a contradiction, since "ContentContribution" sounds like maybe "content-box contribution". But if the documentation mentions that this is for min/max-*content* sizing, then the meaning of "Content" in the function name is clearer and that's less confusing.)

@@ +1274,5 @@
> +                    nsGridContainerFrame::Dimension   aDimension,
> +                    nsLayoutUtils::IntrinsicISizeType aConstraint)
> +{
> +  PhysicalAxis axis(((aDimension == nsGridContainerFrame::eColDimension) ==
> +                     aCBWM.IsVertical()) ? eAxisVertical : eAxisHorizontal);

Please make sure it's documented somewhere what it means to measure a size for a particular nsGridContainer::Dimension, in terms of width/height -- i.e. what someone is actually asking for, when they pass eColDimension to one of these measurement functions.

(Maybe document that next to the Dimension decl, and/or in the comment for some of these measurement functions.)

I initially interpreted it backwards, and had trouble parsing this expression as a result. I was thinking that for a LTR (horizontal-WM) grid, eColDimension would mean that we're measuring the height, because columns are vertical and height is the vertical quantity. But it looks like eColDimension really means width [the spread of a column]. This makes sense, but it's not intuitively obvious (without documentation) which length it's expected to mean.

@@ +1293,5 @@
> +    // XXX this will give mostly correct results for now (until bug 1174569).
> +    nscoord i = CRAZY_COORD - 1; // largest isize we can use without asserting
> +    LogicalSize availableSize(wm, i, NS_UNCONSTRAINEDSIZE);
> +    const nsHTMLReflowState* rs =
> +      aReflowState ? aReflowState : dummyParentState.ptr();

We already null-checked aReflowState up higher (where we emplaced dummyParentState). Let's just combine this check/initialization-of-'rs' with that earlier check, to reduce redundant null-checks, and to group the usage of |dummyParentState| alongside its emplacement.

Something like:
  const nsHTMLReflowState* rs = aReflowState;
  if (!rs) {
    dummyParentState.emplace(...);
    rs = dummyParentState.ptr();
  }

@@ +1297,5 @@
> +      aReflowState ? aReflowState : dummyParentState.ptr();
> +    nsHTMLReflowState childRS(pc, *rs, aChild, availableSize);
> +    if (!aReflowState) {
> +      MOZ_ASSERT(!parent->HasAnyStateBits(NS_FRAME_IN_REFLOW));
> +      parent->AddStateBits(NS_FRAME_IN_REFLOW);

Two things:

(1) Thanks for adding the assertion here -- but, after a bit more MXR research, I think we might *really* want to be using MarkInReflow() here, which just asserts & sets the bit. It looks like that's what we normally use to manage this bit. (I only see 2 explicit AddStateBits(NS_FRAME_IN_REFLOW) calls, vs. 55 calls to MarkInReflow().)

(2) So, we're kind of lying here when we set this bit.  'parent' is *not actually* in reflow (if we visit this clause), but we're pretending it is. Why do we have to do this, and are you sure that faking this state isn't going to cause trouble?  ("child" is arguably about to be IN_REFLOW, but "parent" is not really IN_REFLOW.)

(Probably worth including a brief code-comment bringing this up & saying why it's OK, for the benefit of others reading this code & wondering about this.)
Attachment #8648025 - Flags: review?(dholbert) → review+
Comment on attachment 8625185 [details] [diff] [review]
addendum to part 3

Per comment 15 / comment 17, I'd be interested in seeing the following changes to the addendum patch:

 (1) I think INFINITE_ISIZE_COORD should be defined alongside NS_UNCONSTRAINEDSIZE & friends. (It's easier to find these huge reserved values if they're all defined together, and it'll reduce the likelihood that we'll accidentally re-use a particular numerical value in a way that could cause unexpected behavior.)

 (2) If possible, we should give INFINITE_ISIZE_COORD a value that's closer (but not exactly) NS_UNCONSTRAINEDSIZE. nscoord_MAX/2 seems a bit unnecessarily-too-close-to-reasonable-values (though still quite large) -- seems like we should pack these reserved values at the very top of the range as much as we can.

 (3) Set a #ifdef DEBUG frame property to control whether we honor the CRAZY_COORD/CRAZY_SIZE warnings, instead of universally disabling these warnings for grid items.
Attachment #8625185 - Flags: review?(dholbert) → feedback+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Please make sure it's documented somewhere what it means to measure a size
> for a particular nsGridContainer::Dimension, in terms of width/height --
> i.e. what someone is actually asking for, when they pass eColDimension to
> one of these measurement functions.
> 
> (Maybe document that next to the Dimension decl, and/or in the comment for
> some of these measurement functions.)

OK, sure.  But FYI, whenever a *contribution* to a track size is mentioned
in the Grid code (or spec) it's always clear what is meant because a row
or column really just have size in one dimension.  We may want the child's
(margin-box) width or height though depending on its writing-mode is the
same as the container's or not, but that's usually left out since it would
get tedious to write out every time.

Anyway, I clarified the comment and added a @note to the Dimension decl.

> Something like:
>   const nsHTMLReflowState* rs = aReflowState;
>   if (!rs) {
>     dummyParentState.emplace(...);
>     rs = dummyParentState.ptr();
>   }

OK, I refactored the code here a bit.

> (2) So, we're kind of lying here when we set this bit.  'parent' is *not
> actually* in reflow (if we visit this clause), but we're pretending it is.

Correct.

> Why do we have to do this, and are you sure that faking this state isn't
> going to cause trouble?

It used to be required to avoid assertions in some cases.
It may just be cargo cult at this point though, so I removed it.
I'm pretty sure the worst that would happen is a false assertion
and fuzz testing will find that eventually if it's still required.
(I'm aware of bug 949294, but that's special and won't happen here.)
Attachment #8648025 - Attachment is obsolete: true
Attachment #8650036 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> I'd suggest defining INFINITE_ISIZE_COORD as (NS_MAXSIZE - 1)

I'm worried that it's too easy to add a padding/border/margin/outline and
get NS_MAXSIZE (== NS_UNCONSTRAINEDSIZE) and then risk taking a different
code path or causing a false assertion.  I'd like to subtract a bigger
number to have some margin against that happening, so I set it to 1M px.
(NS_MAXSIZE is ~17M px)

I prefixed the name of the frame prop with "Debug" to follow established
naming convention.  And added a IsCrazySizeAssertSuppressed() convenience
method to avoid having to type that out most of the time.  I considered
adding an "Auto" class for setting it as well, but it's just one use
so far so I skipped that.
Attachment #8625185 - Attachment is obsolete: true
Attachment #8650039 - Flags: review?(dholbert)
Comment on attachment 8650036 [details] [diff] [review]
part 3 - [css-grid] Implement the 'min-content' / 'max-content' sizing functions in layout

>+++ b/layout/generic/nsGridContainerFrame.h
>+  // @note when used in a function that measures a child's size, eColDimension
>+  // means we're calculating the child's contribution to the column sizing.
>   enum Dimension { eColDimension, eRowDimension };

Could you add 1 more line here that makes it concrete -- something like:
  // (i.e. the width, if we're in the default LTR writing-mode)

(The "column sizing" clarification, on its own, still feels ambiguous, since it's still open to my backwards initial interpretation from comment 18.  With a bit of thought, it's clear that it probably means width, since columns all need to be the same height, as you explain in comment 3.  But I'm confident that I'll forget this guideline after not looking at this code for a little while :), so a concrete example will be helpful to unambigously clarify things.)
Attachment #8650036 - Flags: review?(dholbert) → review+
Comment on attachment 8650039 [details] [diff] [review]
part 4 - add INFINITE_ISIZE_COORD and suppress warnings (addendum to part 3)

Review of attachment 8650039 [details] [diff] [review]:
-----------------------------------------------------------------

Mm, good point RE values close to NS_UNCONSTRAINEDSIZE. OK, 1 million pixels less than NS_MAXSIZE seems like a reasonable value.

r=me with the following:

::: layout/generic/nsIFrame.h
@@ +149,5 @@
>  //       at least update AdjustComputedHeight/Width and test ad nauseum
>  
> +// 1 million CSS pixels less than our max app unit measure.
> +// This is used to reflow with an "infinite" available space [css-sizing].
> +#define INFINITE_ISIZE_COORD nscoord(NS_MAXSIZE - (1000000*60))

Two comment-tweaks:
 (1) s/available space/available inline space/ or something like that, to make it clear (in the comment) that this is inline-axis-specific. (In the block axis, we use a different value -- NS_UNCONSTRAINEDSIZE -- to indicate infinite available space)

 (2) Please add a parenthetical explanation in this comment (or somewhere) why we can't just use NS_UNCONSTRAINEDSIZE for this purpose.

(I think it's because our code generally assumes that inline size is an actual quantity which we can do arithmetic with (e.g. subtracting out margins/borders/paddings and sizes of elements on a given line), and we disallow arithmetic with NS_UNCONSTRAINEDSIZE. And in contrast, it's basically fine for us to do arithmetic with INFINITE_ISIZE_COORD, because the avialable inline-size should only be used for things like line wrapping, not for resolving percentages or anything (I hope?))
Attachment #8650039 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #22)
> Could you add 1 more line here that makes it concrete -- something like:
>   // (i.e. the width, if we're in the default LTR writing-mode)

Sure, I'll add something like that before landing.

> since columns all need to be the same height

As I see it, a column doesn't really have a meaningful size in that dimension.
That's the row dimension.  The (two-dimensional) grid has a size in both
dimensions of course, and it derives its (intrinsic) isize from the sum of
column sizes and its bsize from the sum of row sizes.  IOW, a column size
is always associated with the container isize.  A column's bsize isn't
a thing in the spec, nor in the code. (ditto for row size being bsize)

The spec uses terms like "track/row/column size" too, without any 'inline-'
or 'block-' prefix since it's unambiguous what is meant.

BTW, when running the Track Sizing Algorithm over the columns, we never
ask for an item's size in the row dimension, and vice versa.
(In reply to Mats Palmgren (:mats) from comment #24)
> As I see it, a column doesn't really have a meaningful size in that
> dimension.
> That's the row dimension.

I agree that this is unambiguous if you've got the code/spec fully paged into your head.

Let me try to explain my initial confusion (& concern for other people's future confusion when stumbling over this) a bit more clearly perhaps. :)  To me, "column dimension" sounds intuitively like it could mean "vertical dimension", for reasons described in [1].  So, measuring something's "size in the column dimension" sounds like it means we're measuring a "vertical size" a.k.a. measuring a "height".  (I understand that this is *not* actually what you mean, and it makes sense with clarification; I'm just explaining why it's not intuitively obvious what it must mean, right off the bat.)

[1] reasons "column dimension" feels like it might mean "vertical dimension":
 * A column is a tall, vertically-oriented thing.
 * In flexbox, "flex-direction: column" means "lay my children out vertically, top-to-bottom".
Oh, I see, thanks for elaborating.  To me, the column dimension is
the dimension in which column numbers are incremented.
I didn't realize this could be ambiguous.

We should use a different word than dimension then (and probably
avoid "spacial" words altogether).  How about,
enum TrackType {eColumns,eRows} ?
Flags: needinfo?(dholbert)
So, one further point of confusion: the Grid spec actually defines some terms "Row Axis" and "Column Axis", which mean exactly opposite what you mean when you say "row dimension" and "column dimension". (It says column lines run *along* the "column axis" -- hence, the column axis is vertical, assuming LTR writing mode:   https://drafts.csswg.org/css-grid-1/#grid-concepts )

So, I tend to think we should stick with the spec's names for these axes (explicitly using e.g. eRowAxis to mean horizontal, eColumnAxis to mean vertical, in LTR) -- or perhaps better, just re-use LogicalAxis, since I think this has an unambiguous mapping to a dimension in the grid.  (e.g. rows stack in the block dimension.) I'm pretty sure your original eColDimension enum-value is effectively eAxisInline, and your eRowDimension is effectively eAxisBlock. As a bonus, this means you can just directly use PhysicalAxisForLogicalAxis() to do the conversion in ContentContribution.

(It does mean callsites might be a bit more opaque -- e.g. to get the sum of the row sizes, you'd call a function and pass in eBlockDimension -- but that's maybe not too bad.)

Alternately, I can see enum TrackType working, though it may get confusing at the point where we'd create a PhysicalAxis from our TrackType (the code quoted in comment 18), given that I think your meaning of eRows is orthogonal to the spec's "row axis" definition.  But, a brief explanatory code-comment (explaining what it means to pass eRows to a size function) could easily clear that up.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #27)
> So, one further point of confusion: the Grid spec actually defines some
> terms "Row Axis" and "Column Axis", which mean exactly opposite what you
> mean when you say "row dimension" and "column dimension". (It says column
> lines run *along* the "column axis" -- hence, the column axis is vertical,
> assuming LTR writing mode:  
> https://drafts.csswg.org/css-grid-1/#grid-concepts )

Hmm, that's a very confusing definition IMO.

Let's take a step back and compare how an axis works in the real world.
https://en.wikipedia.org/wiki/Three-dimensional_space_%28mathematics%29
Here we have the X dimension with an X-axis, where we find x=1, x=2, x=3
and so forth.  By analogy, shouldn't the column-axis be the direction in
which columns are stacked and where we find column 1, 2, 3 and so forth?

Let's try with a more layout-oriented statement:
Xs are stacked in the X dimension, along the X-axis.
The X-size is the distance from its start/end points on the X-axis.

Now substitute X with "inline" or "block" - works fine.  Now try "column":

Columns are stacked in the column dimension, along the column-axis.
The column-size is the distance from its start/end points on the column-axis.

This works fine with *my* definition of column-axis, but not the spec's.
I think the spec's definition is a mistake.  (There's actually no need
to introduce column/row axis in the spec - using block/inline axis instead
works equally well.)

For comparison, here's the above statement using the spec's definitions:
Columns are stacked in the row dimension, along the row-axis.
The column-size is the distance from its start/end points on the row-axis.

Which, as you can see, is just bonkers. :-)

I'll use eLogicalAxisInline/Block instead as you suggested...
I'll land this part a little later in the series if you don't mind, to avoid
having to update a bunch of patches already reviewed/under review.

Thanks for the tip on the PhysicalAxis conversion; that worked out nicely.
Attachment #8651209 - Flags: review?(dholbert)
Attachment #8651209 - Flags: review?(dholbert) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: