Closed Bug 1009214 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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?
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. :)
Depends on: 874718
(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.
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.
(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.
(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.
(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.)
Ah, good point.  I'll just use GetType() for now then.
Blocks: 1151243
Assignee: nobody → mats
Attachment #8600648 - Flags: review?(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'.)
(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 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)
(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).
Attachment #8600648 - Attachment is obsolete: true
Attachment #8600648 - Flags: review?(dholbert)
Attachment #8601704 - Flags: review?(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")
(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 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.
Attachment #8601704 - Flags: review?(dholbert) → review+
> Nit: Maybe this should just return void now?

Did so, thanks.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/55d911f8cb6a
https://hg.mozilla.org/mozilla-central/rev/4eca79afce17
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Removing dependency on bug 874718 -- this clearly didn't end up depending on that bug.)
No longer depends on: 874718
You need to log in before you can comment on or make changes to this bug.