Closed Bug 1151213 Opened 9 years ago Closed 9 years ago

[css-grid] Implement grid layout for align-self and justify-self

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

Testcase: grid item stretch bugs
459 bytes, text/html
Details
part 2 - [css-grid][css-align] Reftests for the 'align-self' and 'justify-self' properties on grid items.
63.61 KB, patch
Details | Diff | Splinter Review
Update grid item child positions after resize (for posterity)
7.43 KB, patch
Details | Diff | Splinter Review
part 1 - [css-grid][css-align] Implement layout for the 'align-self' and 'justify-self' properties on grid items.
19.61 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Blocks: 1000592
Depends on: 1176782
Blocks: css-align-3
align-/justify-items are implemented in the style system in bug 1176782.
Layout code doesn't need to deal with them directly.  (At least not currently
since they affect the corresponding *-self value.  That might change though
if resolving 'auto' is to happen when calculating the used value instead...)
Summary: [css-grid] Implement align-self/items and justify-self/items → [css-grid] Implement grid layout for align-self and justify-self
First a note on a couple of bugs in the existing grid layout code
(see the attached testcase).

For the first item, we reflow the grid item in its grid area (which has
a constrained height).  This causes the block code to report INCOMPLETE
and set the height for the frame to match the given height - it's
expecting the container to eventually create a next-in-flow to pick up
the slack.  The problem is the grid container doesn't know how to deal
with that yet (bug 1144096) so the item is effectively clipped.
So for now, I think it's better to reflow grid items in an unconstrained
CB height.  This makes them overflow their grid areas as needed, which
is the right thing to do anyway when there is no fragmentainer.

(This bug allowed a few errors to creep into our reftests which I'm
correcting in part 2.)

For the second item in the testcase, it fails because our current
(temporary) stretch code simply doesn't account for negative margins.

(see Chrome for the correct layout of this test)
Assignee: nobody → mats
A note regarding reflowing grid items that changed size (due to
*-self:stretch. (*-content:stretch isn't a problem since that occurs
before the items are reflowed)).  Reflowing is the right thing to do
in general since the item might have kids with percentage height,
different writing-mode, 'bottom'-placed abs.pos. descendants etc.
We might be able to optimize this away in some cases in the future
but it seems like a hard problem and I'm not aware of any existing
code that can predict when a reflow could be avoided.  I suspect that
Flexbox suffers the same problem so it might be worth investigating
how to at least detect some simple situations (very common I suspect)
that we could optimize.
Attachment #8666490 - Flags: review?(dholbert)
Comment on attachment 8666490 [details] [diff] [review]
part 1 - [css-grid][css-align] Implement layout for the 'align-self' and 'justify-self' properties on grid items.

See also comment 2, btw.
FWIW, I implemented what Jonathan suggested in bug 1151214 comment 6
to fixup frame positions of children after resizing their container.
Attaching this here for posterity in case it helps with some future
optimization where we can avoid reflowing the item.  (I suspect that
grid items containing children with orthogonal writing-mode is one
of the cases we must reflow though.)
(In reply to Mats Palmgren (:mats) from comment #3)
> A note regarding reflowing grid items that changed size (due to
> *-self:stretch. [...] We might be able to optimize this away in some
> cases in the future but it seems like a hard problem and I'm not aware of any existing
> code that can predict when a reflow could be avoided.  I suspect that
> Flexbox suffers the same problem

In flexbox, we aim to detect and optimize away an unnecessary second reflow by checking:
 (1) Is the stretched size the same as the size we previously reflowed the child at?
 (2) Does the child have a relative bsize? (i.e. will any descendants care if our size goes from being considered indefinite to being considered definite?)

This is approximately the right thing to check, for the block-dimension stretching at least.  Though as noted in bug 1175517 (and the end of bug 1174507 comment 3), NS_FRAME_CONTAINS_RELATIVE_BSIZE may be somewhat bogus in cases with orthogonal flows, which may complicate things.

Anyway, I think it's fine to hold off on trying to optimize this away at the moment.
Comment on attachment 8666490 [details] [diff] [review]
part 1 - [css-grid][css-align] Implement layout for the 'align-self' and 'justify-self' properties on grid items.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +2622,5 @@
>      LogicalPoint childPos =
>        cb.Origin(wm).ConvertTo(childWM, wm,
> +                              containerSize - childSize->PhysicalSize());
> +    if (isGridItem) {
> +      LogicalSize oldSize = childSize->Size(childWM);

Two things:
 (1) This if-check has a lot of code inside it; a one-liner comment just inside to sum it up would be helpful. e.g.:
 // Apply align-self & justify-self, & reflow again if that affects size.

 (2) The variable "oldSize" might want to be renamed -- maybe call it "tentativeSize"?  (At first glance, the name "oldSize" makes me think this would be the size before we entered this round of grid-reflow, or some other semi-stale size. But really, this variable holds the size we *just now* came up with in our first attempt at reflowing the child. That size seems too fresh & likely-correct for us to be calling it "old" at this stage.)

::: layout/generic/nsHTMLReflowState.cpp
@@ +2544,5 @@
>      return;
>    }
>  
>    // The css2 spec clearly defines how block elements should behave
> +  // in section 10.3.3.  This doesn't apply to grid items.

RE your grid-item opt-outs in this function (CalculateBlockSideMargins) -- for flex items & inline tables, we opt out at one callstack-level up from this point, here:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=c32abb6c9e65#2346

Maybe that's what we should be doing for grid items, too? (Seems like it'd be best to have these things opting out of css-block-margin-fixup together, unless they need subtly different behaviors.)
Comment on attachment 8666492 [details] [diff] [review]
part 2 - [css-grid][css-align] Reftests for the 'align-self' and 'justify-self' properties on grid items.

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

Drive-by comments on the test patch:

 * Please make sure we're testing cases where there's negative extra space (as well as cases with positive space), for each of the main alignment values here.  (You might already have this - I didn't look in enough detail to be able to tell.)

::: layout/reftests/css-grid/grid-item-align-001-ref.html
@@ +4,5 @@
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>CSS Grid Test: align-self (part 1 of 2)</title>

Right now this <title> is identical to the <title> in the testcase.  I think it should say something like "CSS Grid Reference Case".

(Generally, we make w3c-testsuite reference files have a dummy <title> that's different than the corresponding testcase, to follow css testsuite conventions.  One benefit of this is that it makes it easy to tell which file is which, when switching back & forth in tabs, when the URL is too long to all fit in your address-bar so you can't see the "-ref" suffix.)

@@ +7,5 @@
> +  <meta charset="utf-8">
> +  <title>CSS Grid Test: align-self (part 1 of 2)</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1151213">
> +  <link rel="help" href="https://drafts.csswg.org/css-align-3/#propdef-align-self">
> +  <link rel="match" href="grid-item-align-001-ref.html">

This <link rel="match"> tag shouldn't be present in a reference case -- let's get rid of it.

@@ +35,5 @@
> +  background: grey;
> +  border-block-start: 2px solid blue;
> +  border-inline-start: 2px solid lime;
> +  margin: 1px 1px 2px 2px;
> +  offset-inline-start: 1px; 

Fix trailing space characters here & elsewhere in this file. (I only noticed them in this file, when skimming with splinter; I think the other files might all be trailing-whitespace-clean.)
Comment on attachment 8666490 [details] [diff] [review]
part 1 - [css-grid][css-align] Implement layout for the 'align-self' and 'justify-self' properties on grid items.

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

Here's a bit more on the main patch here -- haven't quite made it through yet, but here's what I've got so far (combined with comment 8):

::: layout/generic/nsGridContainerFrame.cpp
@@ +852,5 @@
> +
> +  // Map some alignment values to 'start' / 'end'.
> +  switch (aAlignment) {
> +    case NS_STYLE_ALIGN_SELF_START:
> +      aAlignment = MOZ_LIKELY(aSameSide) ? NS_STYLE_ALIGN_START

These enums are *really* easy to misinterpret and get confused about. I initially read ALIGN_SELF_START "align-self: start", and so I was quite confused about why we might end up mapping ALIGN_SELF_START to ALIGN_END.

(Really, ALIGN_SELF_START is meant to be read as "align-*: self-start" -- and this makes sense once I realized that fact.)

To help keep readers from falling into this trap, maybe add comments alongside these case statements, like so:

  case NS_STYLE_ALIGN_SELF_START:  // (align-self: self-start)
  ...
  case NS_STYLE_ALIGN_SELF_END:  // (align-self: self-end)

@@ +887,5 @@
> +    } else {
> +      marginStart = margin.IEnd(wm);
> +      marginEnd = margin.IStart(wm);
> +    }
> +  }

This is a lot of duplicated logic that could be condensed using WritingModes utility functions, I think. Though one utility-function that we need here is missing -- a LogicalMargin accessor. More on that below.

Ideally, this whole block of code (up to the marginStart/marginEnd decls) could be condensed to:

  LogicalSide startSide = MakeLogicalSide(aAxis, eLogicalEdgeStart);
  LogicalSide endSide = MakeLogicalSide(aAxis, eLogicalEdgeEnd);
  if (MOZ_UNLIKELY(!aSameSide)) {
    Swap(startSide, endSide);
  }
  nscoord marginStart = margin.GetSizeOnSide(wm, startSide);
  nscoord marginEnd = margin.GetSizeOnSide(wm, endSide);

...but unfortunately, the "LogicalMargin::GetSizeOnSide" method used in my sample-code here doesn't exist.  It probably should, though. (It should take a "LogicalSide" enum.)

If this makes sense to you, could you file a followup on adding that method, and add an XXX comment here pointing to that bug, so we can use it to clean up this code once we've got this functionality available? (Or better yet, fix the helper bug first and rebase this on top of that.)

(Side note: the 'startSide' & 'endSide' vars come in handy below, too.)

@@ +929,5 @@
> +                styleMargin.GetBStartUnit(wm) != eStyleUnit_Auto &&
> +                styleMargin.GetBEndUnit(wm) != eStyleUnit_Auto)
> +             : (aRS.mStylePosition->ISize(wm).GetUnit() == eStyleUnit_Auto &&
> +                styleMargin.GetIStartUnit(wm) != eStyleUnit_Auto &&
> +                styleMargin.GetIEndUnit(wm) != eStyleUnit_Auto)) {

As above, there's a lot of duplicated logic here to handle the different permutations of writing-modes possibilities, which makes this "if" check a bit unwieldy & harder to read/maintain.

I think this can all be abstracted to remove the duplication like so (untested):

  nsStyleCoord styleSize = aAxis == eLogicalAxisBlock ?
    aRS.mStylePosition->BSize(wm) : aRS.mStylePosition->ISize(wm);
  if (styleSize.GetUnit() == eStyleUnit_Auto &&
      styleMargin.GetUnit(startSide) != eStyleUnitAuto &&
      styleMargin.GetUnit(endSide) != eStyleUnitAuto) {

(startSide and endSide are as defined in other sample code above.)

@@ +942,5 @@
> +          // might not have created any next-in-flow yet.  Use the reflow status
> +          // instead?  Do all fragments stretch? (bug 1144096).
> +          bp.ApplySkipSides(aRS.frame->GetLogicalSkipSides());
> +          nscoord bpInAxis = aAxis == eLogicalAxisBlock ? bp.BStartEnd(wm)
> +                                                        : bp.IStartEnd(wm);

(I wonder if we should expose LogicalMargin::GetSizeInAxis(wm, aAxis), too, maybe as part of the helper-bug suggested above... That would make this slightly simpler.)

@@ +981,5 @@
> +SameSide(WritingMode aContainerWM, LogicalSide aContainerSide,
> +         WritingMode aChildWM, LogicalSide aChildSide)
> +{
> +  return aContainerWM.PhysicalSide(aContainerSide) ==
> +             aChildWM.PhysicalSide(aChildSide);

We should assert here that these two PhysicalSides are in the same physical axis. We want to make sure we're not comparing "top" against "left" or something -- we should only be comparing only-horizontal or only-physical sides here.

(It it seems completely believable that we could end up with a bug where the directions are swapped, since we've got different writing-modes, and special cases in the callers to decide which side to pick.  And such a bug would mess up the logic and potentially give us wrong behavior. This assertion will help us prevent such a bug.)

@@ +997,5 @@
> +  MOZ_ASSERT(alignSelf != NS_STYLE_ALIGN_AUTO,
> +             "invalid 'align-self' computed value for a grid item");
> +  WritingMode childWM = aRS.GetWritingMode();
> +  bool isOrthogonal = aCBWM.IsOrthogonalTo(childWM);
> +  bool sameSide = SameSide(aCBWM, eLogicalSideBStart,

These "sameSide" variables are a bit cryptic at this point in the code -- it's not clear what they represent or what they'll be used for. Maybe add a comment like the following somewhere, to document the meaning, so it's easier to reason about:
  // True if the container's "start" side in this axis is the same as the child's
  // "start" side, in the child's parallel axis:

(This applies both to AlignSelf and JustifySelf.)

@@ +1002,5 @@
> +                           childWM, isOrthogonal ? eLogicalSideIStart
> +                                                 : eLogicalSideBStart);
> +  // XXX revisit after the CSSWG has decided on computed/used values.
> +  if (alignSelf == NS_STYLE_ALIGN_LEFT || alignSelf == NS_STYLE_ALIGN_RIGHT) {
> +    alignSelf = NS_STYLE_ALIGN_START;

The corresponding code at this spot in JustifySelf() is more complex (returning either start or end, depending on if we're RTL).  That's really the only difference between the two methods, actually.  I had to think for a bit about whether the code here really wanted some version of JustifySelf()'s complexity, but then I realized that this is just the same thing discussed in bug 1151214 comment 14 about align-content:left|right & 'start' & parallel/perpendicular axes.

To make what's going on here a little clearer, could you add a version of that same comment here? (but with s/align-content/align-self/ of course)
Blocks: 1209485
(In reply to Daniel Holbert [:dholbert] from comment #7)
>  (2) Does the child have a relative bsize? (i.e. will any descendants care
> if our size goes from being considered indefinite to being considered
> definite?)

Good to know we have a starting point for that.

> This is approximately the right thing to check, for the block-dimension
> stretching at least.  Though as noted in bug 1175517 (and the end of bug
> 1174507 comment 3), NS_FRAME_CONTAINS_RELATIVE_BSIZE may be somewhat bogus
> in cases with orthogonal flows, which may complicate things.

Yeah, I think this bit needs to be true when there is a child with
orthogonal flow.  Also, the Grid spec mentions wrapped column flex
containers and multi-column layout in a similar context here
(in the green note):
https://drafts.csswg.org/css-grid/#algo-overview

> Anyway, I think it's fine to hold off on trying to optimize this away at the
> moment.

I filed bug 1209485 for this.
(In reply to Daniel Holbert [:dholbert] from comment #8)
>  (2) The variable "oldSize" might want to be renamed

(Or just add a comment like "// from ReflowChild above" at the end of the line where this is declared)

> RE your grid-item opt-outs in this function (CalculateBlockSideMargins) --
> for flex items & inline tables, we opt out at one callstack-level up [...]
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=c32abb6c9e65#2346
> Maybe that's what we should be doing for grid items, too?

(We could change the existing "!flexContainerFrame" check to a !IsFlexOrGridItem() call, which would prevent the need for your new IsGridItem accessor, for the moment at least.)
Blocks: 1209710
(In reply to Daniel Holbert [:dholbert] from comment #8)
>  (1) This if-check has a lot of code inside it; a one-liner comment just
> inside to sum it up would be helpful. e.g.:
>  // Apply align-self & justify-self, & reflow again if that affects size.

Added a comment as suggested.

>  (2) The variable "oldSize" might want to be renamed

I added a "from ReflowChild()" comment there.

> RE your grid-item opt-outs in this function (CalculateBlockSideMargins) --
> for flex items & inline tables, we opt out at one callstack-level up from
> this point

OK, I moved it there for now.  (I'm not sure that's correct
for auto-margins, but let's deal with that in bug 1176793.)


(In reply to Daniel Holbert [:dholbert] from comment #10)
> (Really, ALIGN_SELF_START is meant to be read as "align-*: self-start" --
> and this makes sense once I realized that fact.)

Yeah, hopefully it's confusing mostly to us who have seen
NS_STYLE_ALIGN_SELF_ as a prefix.  All of those are removed now
in favor of NS_STYLE_ALIGN_ / NS_STYLE_JUSTIFY_ (and the latter
are just redundant synonyms really, but it avoids confusing people
with "mJustifySelf == NS_STYLE_ALIGN_AUTO" and the like, which
I figured might look like a typo to the unaware).

>   case NS_STYLE_ALIGN_SELF_START:  // (align-self: self-start)

I've added comments there.

> If this makes sense to you, could you file a followup on adding that method,
> and add an XXX comment here pointing to that bug, so we can use it to clean
> up this code once we've got this functionality available?

I filed bug 1209710, but I'm a bit concerned about performance.
It might be OK, but I'd like to compare the assembler code before
making this change.

> > +SameSide(WritingMode aContainerWM, LogicalSide aContainerSide,
> > +         WritingMode aChildWM, LogicalSide aChildSide)
> > +{
> > +  return aContainerWM.PhysicalSide(aContainerSide) ==
> > +             aChildWM.PhysicalSide(aChildSide);
> 
> We should assert here that these two PhysicalSides are in the same physical
> axis. We want to make sure we're not comparing "top" against "left" or
> something -- we should only be comparing only-horizontal or only-physical
> sides here.

Sure.

> These "sameSide" variables are a bit cryptic at this point in the code --
> it's not clear what they represent or what they'll be used for. Maybe add a
> comment like the following somewhere, to document the meaning, so it's
> easier to reason about:
>   // True if the container's "start" side in this axis is the same as the
> child's
>   // "start" side, in the child's parallel axis:

Added those comments.

> > +  if (alignSelf == NS_STYLE_ALIGN_LEFT || alignSelf == NS_STYLE_ALIGN_RIGHT) {
> > +    alignSelf = NS_STYLE_ALIGN_START;
> 
> To make what's going on here a little clearer, could you add a version of
> that same comment here? (but with s/align-content/align-self/ of course)

Sure, I added a comment in both methods.
Attachment #8666490 - Attachment is obsolete: true
Attachment #8666490 - Flags: review?(dholbert)
Attachment #8667519 - Flags: review?(dholbert)
(In reply to Mats Palmgren (:mats) from comment #13)
> I filed bug 1209710, but I'm a bit concerned about performance.
> It might be OK, but I'd like to compare the assembler code before
> making this change.

Thanks!

(stepping back from this specific example: IMO, with new features, we should value readability/maintainability over microoptimizations-that-require-duplicated-code/logic, unless we're sure the microoptimizations are worth it.  My bet is that the perf issues we'll end up caring most about here will really be about how to avoid repeated reflows, rather than how to decide which piece of a LogicalMargin to read from.  If we discover bottlenecks or that a few lines of code are slow, we can tweak stuff as-needed.)

(Anyway, I suppose the duplication here is not too big of a deal; we're only duplicating 2 lines 4 times, and then [further down] 3 lines 2 times. So, I'm fine with revisiting this in bug 1209710.)
Comment on attachment 8667519 [details] [diff] [review]
part 1 - [css-grid][css-align] Implement layout for the 'align-self' and 'justify-self' properties on grid items.

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

r=me with the following addressed as you see fit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +949,5 @@
> +          nscoord bpInAxis = aAxis == eLogicalAxisBlock ? bp.BStartEnd(wm)
> +                                                        : bp.IStartEnd(wm);
> +          size = std::max(0, size - bpInAxis);
> +          const nscoord oldContentSize = size;
> +          size += gap;

Three things:
 (1) Do we actually want this "std::max" expression here?  When it takes effect & clamps (and I'm having a hard time imagining of when that would happen), it seems like this will make us behave as if "gap" were larger, and we'll stretch too much (from an "old" size that's smaller than the container size, to a final size that's larger than the container size).  Am I understanding correctly? And if so, wouldn't it be better to just let ourselves stretch right up to the container size in that scenario, instead of beyond it?

 (2) The variable 'size' is a bit confusing to follow here. It starts out as our unstretched border-box size, and then it's reassigned to our clamped, unstretched content-box size, and then it's adjusted from there to be our stretched content-box size. So it ends up representing three different things, and those three things differ across two categories (box-type & stretchedness).  Perhaps rename the first usage to "borderBoxSize", and declare a new variable "contentBoxSize" at the line with std::max?  (That second variable will only vary on one category -- stretchedness -- which makes things a little easier to follow.)

 (3) If you like, "oldContentSize" might be more clearly-named "unstretchedContentSize". (That makes its meaning vs. the updated size a little clearer IMO.)

@@ +977,5 @@
> +    return didResize;
> +  }
> +  nscoord& pos = aAxis == eLogicalAxisBlock ? aPos->B(wm) : aPos->I(wm);
> +  pos += MOZ_LIKELY(aSameSide) ? offset : -offset;
> +  return didResize;

The early return doesn't seem that useful here -- it's not saving us much indentation and it's not really simplifying the logic at all. I'd suggest just folding these last few lines together, so that we end up with only a single return statement, like so:

  if (offset != 0) {
    nscoord& pos = aAxis == eLogicalAxisBlock ? aPos->B(wm) : aPos->I(wm);
    pos += MOZ_LIKELY(aSameSide) ? offset : -offset;
  }
  return didResize;

@@ +1011,5 @@
> +                                                 : eLogicalSideBStart);
> +  // Grid's 'align-self' axis is never parallel to the container's inline axis,
> +  // so the used value of align-content:left|right is 'start'.
> +  // XXX revisit after the CSSWG has decided on computed/used values.
> +  if (alignSelf == NS_STYLE_ALIGN_LEFT || alignSelf == NS_STYLE_ALIGN_RIGHT) {

s/align-content/align-self/ in the comment here.
Attachment #8667519 - Flags: review?(dholbert) → review+
> (1) Do we actually want this "std::max" expression here?

Hmm, perhaps not.  I removed the std:max as you suggested,
but also left behind a non-fatal assertion so we can verify
that stretch works correctly just in case it does occur.

I fixed the other nits.
Flags: in-testsuite+
Depends on: 1221525
Depends on: 1224634
Depends on: 1225118
Depends on: 1229145
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: