Closed Bug 1107783 Opened 10 years ago Closed 9 years ago

[css-grid] implement abs.pos. grid item placement

Categories

(Core :: Layout, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

part 2, [css-grid] Reftest for basic placement and layout of abs.pos. grid items.
13.24 KB, patch
Details | Diff | Splinter Review
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.
25.22 KB, patch
dholbert
: review+
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review 
As before (for normal flow) the track sizing is mostly just
throw-away code. (ToPositionAndLengthForAbsPos)

The placement code should be complete, except for a possible
spec issue in an edge case, as noted in the big XXX comment.

The reflow bits are complete, except for fragmentation (overflow
containers).  I'll file a separate bug on that.
Attachment #8578610 - Flags: review?(dholbert)
(filed bug 1144096 on fragmentation)
Comment on attachment 8578610 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

I've made a mistake here: I need to check each abs.pos. child frame if
they are actually a grid item.  They might not be if they are a descendant
of a child element.  So I need check that first, and also pass along
a real CB for the container for any non-grid-item abs.pos. children too
and fix nsAbsoluteContainingBlock.

Feel free to add any comments you might already have on other parts
and I'll address those too in the next version.
Attachment #8578610 - Flags: review?(dholbert)
Actually, the spec says "If an absolutely positioned element’s containing block
is generated by a grid container..." so that seems to include *any* decsendant
element for which the grid container is the containing block, so this might be
right after all.  I need to check what Chrome does ... and get some coffee :-)

http://dev.w3.org/csswg/css-grid/#abspos-items
Comment on attachment 8578610 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

After reading the spec again I think this is actually correct.  Sorry for
my temporary confusion in comment 4.

Chrome agrees with that for the cases I tested.  Also for position:fixed
when the grid container has a transform.

I'll add some more reftests for these cases.
Attachment #8578610 - Flags: review?(dholbert)
Added tests for fixed pos grid items, and descendants (other than child).
Attachment #8578611 - Attachment is obsolete: true
[working through my review queue -- I'm planning on getting to this review tomorrow, FWIW.]
Comment on attachment 8578610 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

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

First round of feedback (of 2 rounds, I think):

::: layout/generic/nsGridContainerFrame.cpp
@@ +151,5 @@
>  
> +static nscoord
> +GridTrackPosition(uint32_t aTrack, const nsTArray<TrackSize>& aTrackSizes)
> +{
> +  MOZ_ASSERT(aTrack != 0, "expected a 1-based track number");

At this function's call-sites (in this patch), we're actually passing in a line-number, not a track number. (You pass in a LineRange mStart or mEnd.)

So, I'd think this should be "aLine", not "aTrack", and the assertion should be about a 1-based line number. (not track number)  And perhaps the function should be called "GridLinePosition()"?

@@ +153,5 @@
> +GridTrackPosition(uint32_t aTrack, const nsTArray<TrackSize>& aTrackSizes)
> +{
> +  MOZ_ASSERT(aTrack != 0, "expected a 1-based track number");
> +  const uint32_t end = aTrack - 1;
> +  MOZ_ASSERT(end <= aTrackSizes.Length(), "aTrackSizes isn't large enough");

"end" is a bit confusing as a variable-name here -- the caller passes in "mStart" or "mEnd", and we convert each of those into "end" here, which seems odd. (Really, this code is just using "end" in different sense.)

Maybe replace with:
  const uint32_t trackIdxAfterLine = aTrack - 1;  // 0-based

(I think this variable really represents the "track index after the passed-in line", in the loop where it's used. We iterate through our tracks, and we stop when we get to the track that's after the passed-in line. And that gives us the nscoord-position of the passed-in line.)

@@ +192,5 @@
> +  MOZ_ASSERT((aChild->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
> +             aChild->IsAbsolutelyPositioned());
> +  nsRect* cb =
> +    static_cast<nsRect*>(aChild->Properties().Get(ContainingBlockRect()));
> +  MOZ_ASSERT(cb, "only the grid container should reflow grid items");

I don't immediately see the connection between the assertion-message and the asserted-condition here.  It's unclear why this cb's nullness corresponds to something else having reflowed it. (If this fails, a more likely explanation would be that the grid frame just forgot to set this property when reflowing aChild, or that aChild isn't actually a grid item, or that we're calling this function too early before the property's been set.)

Maybe replace with:
  MOZ_ASSERT(cb, "this method must only be called on grid items, & the grid "
                 "container should've reflowed this item by now & set up cb");

@@ +899,5 @@
>    const nsTArray<TrackSize>& aTrackSizes, nscoord* aPos, nscoord* aLength) const
>  {
> +  //MOZ_ASSERT(mStart != 0 && Extent() > 0, "expected a definite LineRange");
> +  // XXX relaxed it a bit for abs.pos. items for now:
> +  MOZ_ASSERT(mStart != 0 && mEnd != 0, "expected a definite LineRange");

It seems like a bit of a shame to have to relax this assertion, just so we can accommodate an uncommon case (abspos items).  (I suspect you agree, given that you commented the stronger assertion out, instead of removing it entirely.)

Maybe it'd be useful to add an "mIsForAbsPosItem" bool to LineRange, to calibrate our expectations on the invariants here? (It could probably be #ifdef DEBUG; and false by default, but set to true somewhere, if this LineRange is for an abspos grid item.)

Or, maybe that'd make this too complex... I'd just like to be able to continue sanity-checking that our normal [non-abspos] LineRanges look the way we expect them to. Any other ideas/thoughts on this?

::: layout/generic/nsGridContainerFrame.h
@@ +49,5 @@
>      nscoord mBase;
>      nscoord mLimit;
>    };
>  
> +  static const nscoord VERY_LIKELY_A_GRID_CONTAINER = -123456789;

This is a nice hack to save the virtual function call, but it's a bit mysterious here at the declaration. Perhaps worth a short comment hinting at its usage (and importantly, indicating that it's not definitive; it's just a first line of defense against a virtual function call). 

e.g. "nsAbsoluteContainingBlock.cpp uses a virtual GetType() call to enable its special-case code for grid items. To avoid having to make this virtual call in the common (non-grid) case, it checks for this grid-specific sentinel value before doing the actual GetType() check."
Comment on attachment 8578610 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

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

So, just to make sure I understand -- is the goal here than an item with...
  grid-row-start: auto;
  grid-row-end: span 2;
  position: absolute;
...should have its containing block go from the grid's left padding-edge to the second column's right edge? So, it ends up spanning 2 tracks, *and* spanning the grid's left padding as a bonus?

(I think this makes sense, though it's a bit counterintuitive that "span 2" ends up meaning "span 2 as well as the grid padding".)

I'm holding off on marking r+ because I'd ideally like to settle the impact on the implicit grid before this lands.  (That may or may not involve a www-style roundtrip, depending on whether the implicit-grid-impact actually matters to layout. Superficially, it doesn't seem to me like it matters, except from the perspective of maintaining our LineRange invariant. More details below.)

::: layout/generic/nsGridContainerFrame.cpp
@@ +466,5 @@
> +  // XXX   such elements do not take up space or otherwise participate in
> +  // XXX   the layout of the grid."
> +  // XXX   http://dev.w3.org/csswg/css-grid/#abspos-items
> +  // XXX which implies that abs.pos. grid items shouldn't inflate the
> +  // XXX implicit grid (b/c doing so would affect the layout).

This statement (grid items shouldn't inflate the implicit grid because doing so would affect the layout) isn't obviously true to me. Are you sure that adding new lines past the (current) implicit grid for abspos items would really affect layout?

I might not be thinking it through all the way, but it seems to me like it'd just add additional 0-sized tracks at the end of the grid, which wouldn't impact any other grid items.  So I think that means we *can* add tracks to honor the grid-placement properties of abspos items, without them "taking up space or participating in the layout of the grid." (using the spec's language)

Having said that -- given that the additional tracks would be 0-sized, maybe it's fine to just refuse to create them (as your patch currently does), since the layout is the same whether they exist or not. BUT, the downside of this approach is that it breaks our invariants -- as you mention in an XXX comment at the end of this method, it can give us LineRanges where start == end, which breaks our invariants.  From that perspective, it'd be better if we could grow the implicit grid with 0-sized tracks, and end up with e.g. LineRange(n, n+1) instead of LineRange(n,n).

I'd lean towards clarifying this with the CSSWG before going too far down this path (and relaxing too many invariants based on this assumption).  Could you send an email about that?

@@ +924,5 @@
> +  nscoord* aPos, nscoord* aLength) const
> +{
> +  if (mEnd == 0) {
> +    if (mStart == 0) {
> +      // done

Might be worth having a comment at the beginning of this function, to remind what 0 really means here (since this is where we react to it). e.g.:

  // A "0" line represents "auto", which for abspos children, contributes
  // the corresponding edge of the grid's padding-box.

@@ +929,5 @@
> +    } else {
> +      const nscoord endPos = *aPos + *aLength;
> +      nscoord startPos = ::GridTrackPosition(mStart, aTrackSizes);
> +      *aPos = aGridOrigin + startPos;
> +      *aLength = std::max(endPos - *aPos, 0);

Why do we need std::max here? I'd expect we could depend on GridTrackPosition() to return something inside the content-box. Can't we just assert something like:
  NS_ASSERTION(*aPos <= endPos,
               "GridTrackPosition should return a position inside grid content-box");

If that assertion holds up as I expect it should, then we don't need the std::max.

@@ +934,5 @@
> +    }
> +  } else {
> +    if (mStart == 0) {
> +      nscoord endPos = ::GridTrackPosition(mEnd, aTrackSizes);
> +      *aLength = std::max(aGridOrigin + endPos, 0);

As above, I'm not clear why we need the std::max here. I'd expect aGridOrigin and endPos to both be non-negative.

@@ +941,5 @@
> +      ToPositionAndLength(aTrackSizes, &pos, aLength);
> +      *aPos = aGridOrigin + pos;
> +    }
> +  }
> +  MOZ_ASSERT(*aLength >= 0);

This fatal assertion seems like the sort of thing that a fuzzer will be able to violate with bogus huge values.

Technically, your std::max() clamping will mostly save us (but I'm not sure we really want that clamping, as noted above). And there's also no std::max clamping in the final case here -- where we call ToPositionAndLength() -- so I'm pretty sure a fuzzer could already trip this assertion if it takes that code-path, with bogusly huge track sizes.

So maybe this MOZ_ASSERT should be relaxed to NS_ASSERTION? (or removed)

@@ +1022,5 @@
> +  if (IsAbsoluteContainer()) {
> +    nsFrameList children(GetChildList(GetAbsoluteListID()));
> +    if (!children.IsEmpty()) {
> +      LogicalMargin pad(aReflowState.ComputedLogicalPadding());
> +      pad.ApplySkipSides(GetLogicalSkipSides());

I think you want to pass in aReflowState to GetLogicalSkipSides here. It looks like that helps us figure out whether to skip the bottom-padding or not:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSplittableFrame.cpp?rev=2c434107469b&mark=257-277#240

@@ +1023,5 @@
> +    nsFrameList children(GetChildList(GetAbsoluteListID()));
> +    if (!children.IsEmpty()) {
> +      LogicalMargin pad(aReflowState.ComputedLogicalPadding());
> +      pad.ApplySkipSides(GetLogicalSkipSides());
> +      const LogicalPoint padOrigin(wm, pad.IStart(wm), pad.BStart(wm));

This variable is a bit confusingly-named. Superficially, it sounds like it's the origin of the padding-rect, but really it's the origin of the content-box, *with respect to the padding-box*, I think. 

Maybe rename to something like "contentBoxOrigin", and add a comment saying it's the origin of the grid's content-area, with respect to the padding-box (which is our abspos positioning area)?

@@ +1036,5 @@
> +                                                    aColSizes, aRowSizes,
> +                                                    padOrigin, gridCB));
> +        // nsAbsoluteContainingBlock::Reflow uses physical coordinates.
> +        nsRect* cb =
> +          static_cast<nsRect*>(child->Properties().Get(ContainingBlockRect()));

"ContainingBlockRect" is a bit of a generic name, for a property that we're sticking on arbitrary child-frames. It's a grid-item-specific thing, but that's non-obvious from the name.

Consider renaming to something like "GridItemContainingBlockRect".

::: layout/generic/nsGridContainerFrame.h
@@ +111,5 @@
> +     * containing block for this axis before calling this method.
> +     */
> +    void ToPositionAndLengthForAbsPos(const nsTArray<TrackSize>& aTrackSizes,
> +                                      nscoord aGridOrigin,
> +                                      nscoord* aPos, nscoord* aLength)  const;

nit: there's an extra space character before "const" at the end of the function-signature.
Attachment #8578610 - Flags: review?(dholbert) → feedback+
(carrying forward feedback+)

>   NS_ASSERTION(*aPos <= endPos,
>                "GridTrackPosition should return a position inside grid
> 
> If that assertion holds up as I expect it should, then we don't need the
> std::max.

The std::max is necessary.  Imagine you have a grid container with
zero width and some columns with non-zero fixed width, then the
grid will overflow its container.  An abs.pos. item in that
grid with "grid-column:2/auto" will start to the right of the
container (outside its border-box).  In this case we need to
clamp its width to zero rather than being negative.

I added your assertion and ran the tests without anything failing
so I added a new test that does fail.

> As above, I'm not clear why we need the std::max here. I'd expect
> aGridOrigin and endPos to both be non-negative.

At the moment, it can probably not fail, but I expect it will
be possible in the future if for example we center a grid inside
a zero size container, then the start of the grid will be to
the left of the container border-box and we'll have the same
issue on the start side.

> > +  MOZ_ASSERT(*aLength >= 0);
> 
> This fatal assertion seems like the sort of thing that a fuzzer will be able
> to violate with bogus huge values.

I expect this will hold actually.  The two std::max covers the
above cases and the only other assignment is the from the
ToPositionAndLength(aTrackSizes, &pos, aLength);
which should always return a non-negative 'aLength' b/c the spec
requires all tracks' width/height to be non-negative.  The only
case where it might fail is if 'aLength' is already negative
on entry, but I'd like to catch that and fix it if it occurs.

> I think you want to pass in aReflowState to GetLogicalSkipSides here.

Fixed.

> > +      const LogicalPoint padOrigin(wm, pad.IStart(wm), pad.BStart(wm));
> 
> This variable is a bit confusingly-named. Superficially, it sounds like it's
> the origin of the padding-rect, but really it's the origin of the
> content-box, *with respect to the padding-box*, I think. 
> 
> Maybe rename to something like "contentBoxOrigin", and add a comment saying
> it's the origin of the grid's content-area, with respect to the padding-box

It's not the content box exactly.   justify/alignment can make the grid
have an offset wrt the container content-box.  So I added this:

// 'gridOrigin' is the origin of the grid (the start of the first track),
// with respect to the container's padding-box (CB).
const LogicalPoint gridOrigin(wm, pad.IStart(wm), pad.BStart(wm));

> "ContainingBlockRect" is a bit of a generic name, for a property that we're
> sticking on arbitrary child-frames. It's a grid-item-specific thing, but
> that's non-obvious from the name.

It's only used by Grid for now but I don't see why the same property
could be used by other layout schemes where each child needs to have
a specific CB.  We only /check/ for the property on grid items for now,
but that's just an optimization.

> Consider renaming to something like "GridItemContainingBlockRect".

Sure, we can generalize the name again if needed elsewhere.
Attachment #8578610 - Attachment is obsolete: true
Attachment #8582425 - Flags: feedback+
With an additional test per above comment.
Attachment #8579565 - Attachment is obsolete: true
(In reply to Mats Palmgren (:mats) from comment #11)
> The std::max is necessary.  Imagine you have a grid container with
> zero width and some columns with non-zero fixed width, then the
> grid will overflow its container.  An abs.pos. item in that
> grid with "grid-column:2/auto" will start to the right of the
> container (outside its border-box).  In this case we need to
> clamp its width to zero rather than being negative.

Hmm. So what's supposed to happen if you have an abspos grid item whose containing block starts at a particular start-line (say, line 4) and goes up to the right padding-edge, and its start-line (#4) happens to be *outside* the right padding-edge? e.g.:
  <div style="width:100px; display:grid">
    <!-- Tracks are 50px wide. Line #1 is at 0, line #2 is at 50, ..., line #4 is at 150 -->
    <absos item going from line 4 to end of grid>
  </div>

It seems like your patch prioritizes being inside the container's padding-box over alignment to lines. This seems reasonable, but does the spec actually call for that (or say anything about this case)? Regardless, it makes sense to clamp the abspos containing block to 0 width in this case -- but I'm not sure whether its x-position should be at the grid container's right padding-edge vs. at line #4.
(In reply to Mats Palmgren (:mats) from comment #11)
> > > +  MOZ_ASSERT(*aLength >= 0);
> > 
> > This fatal assertion seems like the sort of thing that a fuzzer will be able
> > to violate with bogus huge values.
> 
> I expect this will hold actually. [...] The only
> case where it might fail is if 'aLength' is already negative
> on entry, but I'd like to catch that and fix it if it occurs.

It will *also* fail if your track-sizes are close to nscoord_MAX, and the addition wraps around into negative territory. This is a silly case, of course, & broken layout is probably fine, but we shouldn't abort. (Otherwise this will block fuzzers from exercising this code.)

This is why I think this assertion should be softened.

> > Maybe rename to something like "contentBoxOrigin", and add a comment saying
> > it's the origin of the grid's content-area, with respect to the padding-box
> 
> It's not the content box exactly.   justify/alignment can make the grid
> have an offset wrt the container content-box.  So I added this:
> 
> // 'gridOrigin' is the origin of the grid (the start of the first track),
> // with respect to the container's padding-box (CB).

Thanks, that's clearer. (Maybe clarify a bit further with s/container/grid container/? Otherwise it sounds a bit like "this is the position of the <div style="display:grid"> with respect to its parent-element".

> > Consider renaming to something like "GridItemContainingBlockRect".
> 
> Sure, we can generalize the name again if needed elsewhere.

(Sounds good. I agree it's easily generalizable [though I'm not sure we have cause to generalize it]. For now, I can see someone reading the code and trying to figure out why we have this generic "ContainingBlockRect" thing which we mostly ignore. In contrast, if it's got a grid-specific name, it's a bit clearer why we mostly-ignore it.)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (In reply to Mats Palmgren (:mats) from comment #11)
> Hmm. So what's supposed to happen if you have an abspos grid item whose
> containing block starts at a particular start-line (say, line 4) and goes up
> to the right padding-edge, and its start-line (#4) happens to be *outside*
> the right padding-edge? e.g.:
>   <div style="width:100px; display:grid">
>     <!-- Tracks are 50px wide. Line #1 is at 0, line #2 is at 50, ..., line
> #4 is at 150 -->
>     <absos item going from line 4 to end of grid>
>   </div>

This is the case I was trying to describe.

> It seems like your patch prioritizes being inside the container's
> padding-box over alignment to lines.

I think my patch prioritizes the position actually, because it
clamps the length to zero and keeps the position as is.
Did I miss something?

> This seems reasonable, but does the
> spec actually call for that (or say anything about this case)? Regardless,
> it makes sense to clamp the abspos containing block to 0 width in this case
> -- but I'm not sure whether its x-position should be at the grid container's
> right padding-edge vs. at line #4.

I agree it should be at the position of #4, with zero width.
I'm not sure if the spec says anything about this case.
I'll send a question to www-style if I can't find anything.
(In reply to Mats Palmgren (:mats) from comment #15)
> I think my patch prioritizes the position actually, because it
> clamps the length to zero and keeps the position as is.
> Did I miss something?

Ah, you're right - sorry, I just misremembered what was in the patch.
(In reply to Daniel Holbert [:dholbert] from comment #14)
> It will *also* fail if your track-sizes are close to nscoord_MAX, and the
> addition wraps around into negative territory.

Ah, good point.  I'll remove it then.

> Thanks, that's clearer. (Maybe clarify a bit further with s/container/grid
> container/?

Sure.
Attachment #8582425 - Attachment is obsolete: true
Attachment #8582652 - Flags: feedback+
I posted a few questions to www-style on the edge cases discussed above:
https://lists.w3.org/Archives/Public/www-style/2015Mar/0448.html
I posted a question on clamping the CB width:
https://lists.w3.org/Archives/Public/www-style/2015Mar/0470.html
Comment on attachment 8582652 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

I think Tab's answers confirms our interpretation of the spec, so I think
I'll just remove the big "XXX spec issue" comment and land it, OK?
Attachment #8582652 - Flags: review?(dholbert)
Comment on attachment 8582652 [details] [diff] [review]
part 1, [css-grid] Implement abs.pos. grid item placement and reflow.

Sounds good. Thanks!
Attachment #8582652 - Flags: review?(dholbert) → review+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/791f8195e4dd
https://hg.mozilla.org/mozilla-central/rev/a55d25db9778
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1194888
Depends on: 1194892
Depends on: 1204585
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: