1009214 - [css-grid] -moz-anonymous-grid-item that only wraps placeholders should not count as grid items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

I think I need to have a frame state bit (added by frame ctor) for
-moz-anonymous-grid-item that only contains placeholders (i.e. the
wrapper would not exist if the positioned children we're removed)
so that the grid container can ignore those.

Daniel, do we already have something like that for flex items that
I can use?

Comment 1

[Daniel Holbert [:dholbert]]

Nope, not yet, but there should be soon.

(Bug 874718 tracks implementing the flexbox spec's final language on abspos stuff; our current implementation matches older spec language, where placeholders *were* flex items. I deprioritized that bug for a while since the new spec language was marked 'at risk', but it's settled now & is up soon on my list of bugs to fix.)

Adding dependency on bug 874718, but the dependency may end up going the other way if you see fit to fix this before I fix bug 874718. :)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> our current implementation matches older spec language, where
> placeholders *were* flex items

er, s/flex items/wrapped in anonymous flex items/. Per bug 874718 comment 5, the old spec text that we match is http://www.w3.org/TR/2012/WD-css3-flexbox-20120612/#abspos-flex-items

For the new spec text, we probably don't want placeholders to get wrapped in anonymous {flex|grid} items at all anymore. I think they probably want to just be direct children of the {flex|grid} container and get special treatment during reflow.

Comment 3

[Mats Palmgren (inactive)]

I tried that, and it leads to a lot of child->GetType()==nsGkAtoms::placeholderFrame
checks so I figured it might be a performance improvement to wrap them.  In most
cases I suspect they will get wrapped up together with a text node anyway so the
wrapper is free.  Likewise, if there are many in sequence they'll share one wrapper.

Alternatively, we could just add a frame bit for placeholder frames to avoid the
virtual call cost in that case, since it's a quite common check.

Alternatively, we could add some faster (non-virtual) way to check for instances
of classes we know are MOZ_FINAL, like nsPlaceholderFrame.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> I tried that, and it leads to a lot of
> child->GetType()==nsGkAtoms::placeholderFrame
> checks so I figured it might be a performance improvement to wrap them.

Hmm, good point. Maybe they should go in a special grid/flex-specific child-frame list, up-front?  (Or one of your "Alternatively" options.)

> In most
> cases I suspect they will get wrapped up together with a text node anyway so
> the
> wrapper is free.

I don't think I agree.  I think in practice, anonymous flex items should be relatively rare, since they are *only* for text [and placeholders, at the moment], and they're completely unstylable, and they're kind of footguns for authors because simple things like adding a <i> around a section of text will break the layout (since the <i> will get promoted to be its own flex item).

So, in practice, I think it's unlikely that placeholders will have an adjacent anonymous flex item to be pulled into.

> Likewise, if there are many in sequence they'll share one
> wrapper.

This is true, though I'm not sure how common this use-case is, either.

Comment 5

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Hmm, good point. Maybe they should go in a special grid/flex-specific
> child-frame list, up-front?

I suspect that we have code that expects placeholders to be
on the principal/overflow lists only.

> So, in practice, I think it's unlikely that placeholders will have an
> adjacent anonymous flex item to be pulled into.
> 
> > Likewise, if there are many in sequence they'll share one
> > wrapper.
> 
> This is true, though I'm not sure how common this use-case is, either.

OK, so if it's not much of a win then I agree we shouldn't wrap them.
I just thought of a fast way to check the type... assuming we *only*
have nsBlockFrame or nsPlaceholderFrame as normal flow children,
then we can use a frame-specific bit in each that is mutually
exclusively set for those two frame types to tell them apart.
I'll experiment with that a bit and see what falls out.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)
> OK, so if it's not much of a win then I agree we shouldn't wrap them.
> I just thought of a fast way to check the type... assuming we *only*
> have nsBlockFrame or nsPlaceholderFrame as normal flow children,

We don't have that guarantee, in flexbox at least.  All our children must be block *level*, but not necessarily block *frames*. (We could easily have e.g. a nsTableOuterFrame or an nsImageFrame or a nsFlexContainerFrame, or pretty much any other frame that can be block-level.)

Comment 7

[Mats Palmgren (inactive)]

Ah, good point.  I'll just use GetType() for now then.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Don't wrap placeholders in an anonymous grid item.

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Update the reference for a few tests that wrapped placeholders in anonymous grid items.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eedd085af169
https://treeherder.mozilla.org/#/jobs?repo=try&revision=987926f7e062

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8600648 [details] [diff] [review]
part 1 - [css-grid] Don't wrap placeholders in an anonymous grid item.

>@@ -9596,41 +9596,41 @@ nsCSSFrameConstructor::CreateNeededAnonF
>     // entirely whitespace*, then we technically should still skip over it, per
>     // the CSS grid & flexbox specs. I'm not bothering with that at this point,
>     // since it's a pretty extreme edge case.
>     if (!aParentFrame->IsGeneratedContentFrame() &&
>         iter.item().IsWhitespace(aState)) {
>       FCItemIterator afterWhitespaceIter(iter);
>       bool hitEnd = afterWhitespaceIter.SkipWhitespace(aState);
>       bool nextChildNeedsAnonItem =
>-        !hitEnd && afterWhitespaceIter.item().NeedsAnonFlexOrGridItem(aState);
>+        !hitEnd && afterWhitespaceIter.item().NeedsAnonFlexOrGridItem(aState, containerType);

Nit: this line ends up too long. Maybe wrap after "&&" (still puts us over 80 but not as badly)?
(maybe wrap after "aState," as well?)

>+  NeedsAnonFlexOrGridItem(const nsFrameConstructorState& aState,
>+                          nsIAtom* aContainerType)
> {
[...]
>+  // Flex containers still wraps placeholders; Grid containers do not.
>+  if (aContainerType == nsGkAtoms::flexContainerFrame &&
>+      !(mFCData->mBits & FCDATA_DISALLOW_OUT_OF_FLOW) &&

Grammar nit: s/wraps/wrap/

Also, please reference bug 874718 in this comment.

>-  bool AtEnd() const
>+  bool AtEnd()
>   {
>     MOZ_ASSERT(mEnumerator || mArrayIndex <= mArray->Length());
>+    if (!mDidSkipPlaceholders && !mIncludePlaceholders) {
>+      mDidSkipPlaceholders = true;
>+      return SkipPlaceholders();
>+    }
>     return mEnumerator ? mEnumerator->AtEnd() : mArrayIndex >= mArray->Length();
>   }

This makes me uneasy, from a "principle of least surprise" perspective. It seems unexpected that a standard iterator-type function like AtEnd() would advance the iterator.

It seems like we should be able to do the skipping in:
 (a) Next() (which you already do)
 (b) Some initialization step (e.g. the constructor & reset()), to handle the case where of a list that is *entirely* placeholders (which needs to return true from AtEnd() right away, without ever having Next() called).

Then AtEnd() can just remain as-is, and not have surprising iterator-advancing behavior. (And it & operator* can remain 'const'.)

Comment 11

[Daniel Holbert [:dholbert]]

(To do the advancing up-front, the constructor would have to know about our intentions for mIncludePlaceholders.  Maybe this could be done via an enum value that's passed to the constructor and to Reset(), via an optional parameter? e.g.
 enum PlaceholderMode { eSkipPlaceholders, eIncludePlaceholders }
...and then the constructor & Reset() would call SkipPlaceholders() depending on that parameter.

Then SetIncludePlaceholders() would no longer be necessary, of course.)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8600648 [details] [diff] [review]
part 1 - [css-grid] Don't wrap placeholders in an anonymous grid item.

>@@ -1206,25 +1247,32 @@ nsGridContainerFrame::ReflowChildren(Gri
>                                      const nsHTMLReflowState&   aReflowState,
>                                      nsReflowStatus&            aStatus)
> {
>   WritingMode wm = aReflowState.GetWritingMode();
>   const LogicalPoint gridOrigin(aContentArea.Origin(wm));
>   const nscoord containerWidth = aContentArea.Width(wm) +
>     aReflowState.ComputedPhysicalBorderPadding().LeftRight();
>   nsPresContext* pc = PresContext();
>+  aIter.SetIncludePlaceholders();

This comment is probably obsoleted by my previous comment, but to the extent that we keep this "SetIncludePlaceholders() call", I think we should be calling it up one level, right where we reset the iterator. (since it's really an initialization step)

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Don't wrap placeholders in an anonymous grid item.

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Nit: this line ends up too long.

I figured the first step in fixing bug 874718 would be to revert
these nsCSSFrameConstructor changes...

> It seems unexpected that a standard iterator-type function
> like AtEnd() would advance the iterator.

I disagree with that description.  Of course AtEnd() doesn't advance
the iterator (the GridItemCSSOrderIterator that is).

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> > It seems unexpected that a standard iterator-type function
> > like AtEnd() would advance the iterator.
> 
> I disagree with that description.  Of course AtEnd() doesn't advance
> the iterator (the GridItemCSSOrderIterator that is).

(Right, sorry; I meant to say "advance[/modify] the internal state of the iterator")

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> (In reply to Daniel Holbert [:dholbert] from comment #10)
> > Nit: this line ends up too long.
> 
> I figured the first step in fixing bug 874718 would be to revert
> these nsCSSFrameConstructor changes...

(Mm, true. So there'd have been a limited lifespan on the >80-character-ness of this line, at least. Anyway, thanks for rewrapping.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8601704 [details] [diff] [review]
part 1 - [css-grid] Don't wrap placeholders in an anonymous grid item.

>+++ b/layout/generic/nsGridContainerFrame.cpp
>+  /**
>+   * Skip over placeholder children by moving the iterator forward.
>+   * @return true if we reached the end without finding a non-placeholder
>+   */
>+  bool SkipPlaceholders()
>+  {

Nit: Maybe this should just return void now? None of the callers actually check the return value.

(In the previous patch version, the boolean return value was used to help implement AtEnd(), but that's no longer the case.)

r=me either way.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55d911f8cb6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eca79afce17

Comment 18

[Mats Palmgren (inactive)]

> Nit: Maybe this should just return void now?

Did so, thanks.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55d911f8cb6a
https://hg.mozilla.org/mozilla-central/rev/4eca79afce17

Comment 20

[Daniel Holbert [:dholbert]]

(Removing dependency on bug 874718 -- this clearly didn't end up depending on that bug.)