Mats Palmgren (inactive) Assignee
	Description • 11 years ago

Spawned off from bug 874238 comment 3, bzbarsky writes:

display:transparent on its own is not too bad to implement.  Some general thoughts on it:

1)  In the frame constructor, we'd just replace the relevant frame construction item with its kids in the frame construction list before doing any of the normal list processing.  In fact, AddFrameConstructionItemsInternal could just do it directly when asked to add the items for the parent.

2)  We'd probably need to double-check the interaction with the whitespace frame suppression bits.

3)  We would need to replace GetFrameFor() calls in ContentInserted/ContentAppended/ContentRemoved/etc with something that walks up the tree past display:transparent things.... which means we'll need a way to flag the content nodes that need such walking past.  Or something.  Having style attached to frames sucks here.

4)  We'd need to make sure that style reresolution for display changes from "transparent" to something else works.  Perhaps via something like the undisplayed map.  That might work for #3 too.

5)  Obviously we'll need the "style parent frame" bits in style reresolution fixed, since the inheritance will need to happen from the display:transparent node...  That might be a bit fun.

6)  We'd need to ensure that we properly update the Bloom filter for selector matching when we descend into the kids of a display:transparent node during style reresolution; in particular we'll need to make sure the display:transparent node gets added in.  Ccing Blake, since he's having a similar problem with xbl:children.

	Mats Palmgren (inactive) Assignee
	Comment 1 • 11 years ago

I found "CSS Display" has a separate property for this:
http://dev.w3.org/csswg/css-display-3/#the-display-box

David, do you know what the current thinking is in the CSSWG regarding
the 'box' property?  Should I try to implement this as box:contents
directly instead of display:contents?

	Mats Palmgren (inactive) Assignee
	Comment 2 • 11 years ago

Just to confirm I understand the style inheritance correctly -- so for
<span><div style="display:contents; color:red">TEXT</div></span>
the text should be red, right? and the text frame should be a child
of the <span>'s frame.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

Yes, I believe that's correct.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 4 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #1)
> I found "CSS Display" has a separate property for this:
> http://dev.w3.org/csswg/css-display-3/#the-display-box
> 
> David, do you know what the current thinking is in the CSSWG regarding
> the 'box' property?  Should I try to implement this as box:contents
> directly instead of display:contents?

I'm not sure; I don't recall seeing it before, so it probably hasn't gotten much feedback.

Probably best to use display:contents in the working patches until we get more feedback, since that's easiest (at least before landing)?

	Mats Palmgren (inactive) Assignee
	Comment 16 • 11 years ago

First round of patches that are fairly complete, but perhaps not quite
ready for review.  But early feedback is still welcome!
https://tbpl.mozilla.org/?tree=Try&rev=461adeddd82b

	Mats Palmgren (inactive) Assignee
	Comment 17 • 11 years ago

Comment on attachment 802681 [details] [diff] [review]
Style system support for display:contents.

(requires bug 914432)

	Mats Palmgren (inactive) Assignee
	Comment 18 • 11 years ago

Comment on attachment 802684 [details] [diff] [review]
Support for display:contents in a flex container.

This works for simple flexbox cases, but is far from perfect.
I'll illustrate with a testcase.

	Mats Palmgren (inactive) Assignee
	Comment 19 • 11 years ago

In the 2nd flex container, the <span> is wrapped in an anonymous flex item
rather than being promoted to block, i.e. the frame tree looks like this:
FlexContainer(div)
  Block(div):-moz-anonymous-flex-item
    Inline(span)
      Text(0)"2"

I'd like the frame tree to look like as in the 3rd case, but I haven't
figured out how.  I'd like to spawn off this problem as a follow-up bug
unless you see an obvious fix.

	Mats Palmgren (inactive) Assignee
	Comment 21 • 11 years ago

The difference between the undisplayed map for display:none and display:contents
is that the latter contains also nested display:contents, so some methods
needs to be recursive.  display:contents nodes that are descendants of a
display:none node are not registered though.

	Mats Palmgren (inactive) Assignee
	Comment 22 • 11 years ago

The existing frame properties for :after/:before isn't used for anything
other than keep a strong ref on the content until the frame is destroyed.
I changed it to use only one frame property.

	Mats Palmgren (inactive) Assignee
	Comment 24 • 11 years ago

Comment on attachment 802694 [details] [diff] [review]
Don't assert about restyling non-elements directly when they are display:contents children.

I'm getting this assertion in a few cases so I'm muting it for display:contents.
Not sure if this is the right thing to do -- maybe I should tweak:

  if (providerFrame != mFrame->GetParent()) {
    // We don't actually know what the parent style context's
    // non-inherited hints were, so assume the worst.
    mParentFrameHintsNotHandledForDescendants =
      nsChangeHint_Hints_NotHandledForDescendants;
  }

in ElementRestyler::RestyleSelf instead?
If I add "&& !GetDisplayContents(mFrame->GetContent()->GetParent())"
to the condition there then the assertion doesn't occur.
(I'll add a simple testcase that triggers the assertion.)

	Mats Palmgren (inactive) Assignee
	Comment 27 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4f3836aca3f9
https://tbpl.mozilla.org/?tree=Try&rev=a9cffd336f46

	Daniel Holbert [:dholbert]
	Comment 28 • 11 years ago

Comment on attachment 802685 [details] [diff] [review]
Put display:contents support behind a pref - disabled by default.

r=me , with a few optional nits:

>+++ b/layout/base/nsLayoutUtils.cpp
>+static int
>+DisplayContentsEnabledPrefChangeCallback(const char* aPrefName, void* aClosure)
>+{
>+  NS_ASSERTION(strcmp(aPrefName, DISPLAY_CONTENTS_ENABLED_PREF_NAME) == 0,
>+               "Did you misspell " DISPLAY_CONTENTS_ENABLED_PREF_NAME " ?");
>+
>+  static bool sIsDisplayContentsKeywordIndexInitialized;

I'd prefer we explicitly initialize this bool to false. Not technically necessary, but it improves readability IMHO.

>+  static int32_t sIndexOfContentsInDisplayTable;
>+  bool isDisplayContentsEnabled =
>+    Preferences::GetBool(DISPLAY_CONTENTS_ENABLED_PREF_NAME, false);
>+
>+  if (!sIsDisplayContentsKeywordIndexInitialized) {
>+    // First run: find the position of "contents" in kDisplayKTable.
>+    sIndexOfContentsInDisplayTable =
>+      nsCSSProps::FindIndexOfKeyword(eCSSKeyword_contents,
>+                                     nsCSSProps::kDisplayKTable);
>+    sIsDisplayContentsKeywordIndexInitialized = true;

Maybe worth asserting (or at least WARN_IF_FALSE'ing) that sIndexOfContentsInDisplayTable >= 0?  We won't explode if it's negative, but we'd probably like to notice if it's missing from the table...

(IIRC I didn't have this assertion in the equivalent flexbox code because the flexbox table-entries were originally #ifdef MOZ_FLEXBOX, so depending on build config, they might've been present or absent.)

	Daniel Holbert [:dholbert]
	Comment 29 • 11 years ago

Comment on attachment 802684 [details] [diff] [review]
Support for display:contents in a flex container.

(In reply to Mats Palmgren (:mats) from comment #19)
> I'd like the frame tree to look like as in the 3rd case, but I haven't
> figured out how.  I'd like to spawn off this problem as a follow-up bug
> unless you see an obvious fix.

Seems like something like the following would work better & achieve what you're looking for in comment 19:
 - Before ApplyStyleFixups's "parentDisp->mDisplay == NS_STYLE_DISPLAY_FLEX" check, declare "nsStyleContext* parentSkippingDisplayContext = mParent;", and walk it up the parent chain until you find a non-"display:contents" nsStyleContext.
 - Then, treat that as the "real" parent for the purpose of this code -- i.e., check *that* instead of mParent for being a flex container.

I'd think that should get us to hit this promote-to-block code for your second case in comment 19. (and hence, skip anonymous flex item wrapping, since we'll no longer have inline stuff)

	Mats Palmgren (inactive) Assignee
	Comment 30 • 11 years ago

Lovely, that works exactly as I want it, thanks!

	Mats Palmgren (inactive) Assignee
	Comment 31 • 11 years ago

Tweaked the display-contents-acid-ref.html to match correct flexbox rendering.

	Daniel Holbert [:dholbert]
	Comment 33 • 11 years ago

Comment on attachment 806173 [details] [diff] [review]
Support for display:contents in a flex container.

># HG changeset patch
># Parent c7344de6bb700270f5bff7054bb3ef8267b98b3b
># User Mats Palmgren <matspal@gmail.com>
>Bug 907396 - Support for display:contents in a flex container.  r=dholbert
>+    // Skip display:contents ancestors to reach the potential flex container.

Maybe elaborate slightly on this  comment, e.g. adding something along the lines of:
    // (If there are only "display:contents" ancestors between this node and
    // a flex container ancestor, then this node is a flex item, since its
    // parent *in the frame tree* will be the flex container. So we treat
    // it like a flex item here.)

>+    if ((containerDisp->mDisplay == NS_STYLE_DISPLAY_FLEX ||
>+         containerDisp->mDisplay == NS_STYLE_DISPLAY_INLINE_FLEX) &&
>         GetPseudo() != nsCSSAnonBoxes::mozNonElement) {
>       uint8_t displayVal = disp->mDisplay;
>       // Skip table parts.
>       // NOTE: This list needs to be kept in sync with
>       // nsCSSFrameConstructor.cpp's "sDisplayData" array -- specifically,
>       // this should be the list of display-values that have
>-      // FCDATA_DESIRED_PARENT_TYPE_TO_BITS specified in that array.
>+      // FCDATA_DESIRED_PARENT_TYPE_TO_BITS specified in that array, plus
>+      // NS_STYLE_DISPLAY_CONTENTS.

Why do we still need "plus NS_STYLE_DISPLAY_CONTENTS" here?  I'm betting it's because NS_STYLE_DISPLAY_CONTENTS won't survive a call to EnsureBlockDisplay()... which seems like a bug. (Won't this cause us to ignore "display:contents" if you have an element that's "display:contents; position:absolute" or "display:contents;float:left", since both of those will pass their "display" value through EnsureBlockDisplay IIRC?)

If we can avoid adding to this list, I think it'd be nice.

	Daniel Holbert [:dholbert]
	Comment 34 • 11 years ago

("this comment" = the code-comment, not the commit message -- sorry for any ambiguity on that)

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 11 years ago

Comment on attachment 802695 [details] [diff] [review]
DEBUG only; make frame tree dumps print up to 2 style context parents in the unusual case when they are not the parent frame's style context (e.g. display:contents).

Why do we want this?

	Mats Palmgren (inactive) Assignee
	Comment 36 • 11 years ago

It helped me debugging so I figured I'd leave it in given that it won't trigger in most
cases unless display:contents is used.

	Mats Palmgren (inactive) Assignee
	Comment 37 • 11 years ago

(In reply to Daniel Holbert [:dholbert] from comment #33)
> Maybe elaborate slightly on this  comment

Will do.

> Why do we still need "plus NS_STYLE_DISPLAY_CONTENTS" here?  I'm betting
> it's because NS_STYLE_DISPLAY_CONTENTS won't survive a call to
> EnsureBlockDisplay()... which seems like a bug. (Won't this cause us to
> ignore "display:contents" if you have an element that's "display:contents;
> position:absolute" or "display:contents;float:left", since both of those
> will pass their "display" value through EnsureBlockDisplay IIRC?)

Yeah, I'm aware of the EnsureBlockDisplay() problem but I figured I should
address that in bug 917313 instead.  Or rather, when moving this feature
to the separate 'box' property then the problem is moot.  So messing with
EnsureBlockDisplay() in this bug only to revert it later doesn't seem
worth it.  I'll post patches for bug 917313 in a day or two.  It's a more
elegant solution in general IMO.

> If we can avoid adding to this list, I think it'd be nice.

Agreed, I'll look into that and report back.

	Daniel Holbert [:dholbert]
	Comment 38 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #37)
> > Why do we still need "plus NS_STYLE_DISPLAY_CONTENTS" here?
[...]
> > If we can avoid adding to this list, I think it'd be nice.
> 
> Agreed, I'll look into that and report back.

Cool. I'd be OK with leaving it there for now, if you want, as long as you change the comment to make it clear that (a) this is a temporary hack, and (b) it will be removed as part of bug 917313.

	Daniel Holbert [:dholbert]
	Comment 39 • 11 years ago

(In reply to Daniel Holbert [:dholbert] from comment #28)
> >+++ b/layout/base/nsLayoutUtils.cpp
> >+static int
> >+DisplayContentsEnabledPrefChangeCallback(const char* aPrefName, void* aClosure)
> >+{
> >+  NS_ASSERTION(strcmp(aPrefName, DISPLAY_CONTENTS_ENABLED_PREF_NAME) == 0,
> >+               "Did you misspell " DISPLAY_CONTENTS_ENABLED_PREF_NAME " ?");
> >+
> >+  static bool sIsDisplayContentsKeywordIndexInitialized;
> 
> I'd prefer we explicitly initialize this bool to false. Not technically
> necessary, but it improves readability IMHO.

Actually: dbaron says over in bug 918109 comment 1 that "function-scope static variables are more expensive [...] (at least if they require any non-null initialization)".

So, I take back this review comment -- probably best to stick with the null initialization, to be on the safe side, in case any of our compilers are silly and treat the "= false" as triggering the non-null-initialization burden.

(Though maybe worth adding an " // initialized to false" comment, like what I've got in attachment 806955 [details] [diff] [review].)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 40 • 11 years ago

I don't think = false should make a difference, though you could check.  (And, really, for anything initialization that can be computed at compile time, the compiler can hopefully optimize it properly.)

	Daniel Holbert [:dholbert]
	Comment 41 • 11 years ago

Comment on attachment 806173 [details] [diff] [review]
Support for display:contents in a flex container.

Just noticed that I left this patch as r?.

r=me, with comment 37/38 addressed.

	Mats Palmgren (inactive) Assignee
	Comment 42 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #37)
> (In reply to Daniel Holbert [:dholbert] from comment #33)
> > If we can avoid adding to this list, I think it'd be nice.
> 
> Agreed, I'll look into that and report back.

It's needed to make display:contents work in flex containers since otherwise
ApplyStyleFixups will clobber the used display value of <p> in this example:
<div style="display:flex"><p style="display:contents"></p></div>
when resolving the style for <p>.  But yeah, this was a bit inconsistent with
the decision to not fix EnsureBlockDisplay for other cases but I wanted to
flesh out the flexbox case to make sure the frame construction worked.

Anyway, as you said, this line is unnecessary after moving to box:contents
and I have now removed it bug 917313.

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 11 years ago

Mats, sorry this is taking so long.  I'm planning to look at these patches on Monday.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 11 years ago

Comment on attachment 802689 [details] [diff] [review]
Implement RestyleUndisplayedDescendants that restyles diplay:none children and display:contents descendants.

The new methods need documentation....

r=me with that.

	Boris Zbarsky [:bzbarsky]
	Comment 45 • 11 years ago

Comment on attachment 802692 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

Why do we need the extra complexity in RestyleElement?

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 11 years ago

Comment on attachment 802694 [details] [diff] [review]
Don't assert about restyling non-elements directly when they are display:contents children.

How is this case reached?  When the style of the display:contents changes?

	Boris Zbarsky [:bzbarsky]
	Comment 47 • 11 years ago

Comment on attachment 802695 [details] [diff] [review]
DEBUG only; make frame tree dumps print up to 2 style context parents in the unusual case when they are not the parent frame's style context (e.g. display:contents).

r=me

	Mats Palmgren (inactive) Assignee
	Comment 48 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #46)
> How is this case reached?  When the style of the display:contents changes?

See comment 24 and the testcase in comment 25.  There's a style change for
the frame-less <div> element and we need to make that affect the text frame
somehow.  Previously we could just ignore this case because frame-less
meant display:none and the only case we needed to handle was changes that
caused frame creation.  So now that we have frames for content descendants
of frame-less content we need to apply the non-frame-creating changes
somehow.  That's what the added code in RestyleElement is about.

And in the example the child frame happens to be a text frame, which
triggers the assertion.

	Boris Zbarsky [:bzbarsky]
	Comment 49 • 11 years ago

I see.

So it seems to me that the "should not restyle non-elements directly" invariant simply does not hold anymore in the display:contents world.  

Have you verified that all the restyling machinery actually works on textframes?

	Boris Zbarsky [:bzbarsky]
	Comment 50 • 11 years ago

Comment on attachment 802694 [details] [diff] [review]
Don't assert about restyling non-elements directly when they are display:contents children.

r=me with some better line-wrapping, but do verify that none of the style change list processing assumes elements?

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 11 years ago

Comment on attachment 802692 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

r=me on the frame constructor changes.

For the RestyleManager bets it feels like we should be able to share more code with existing stuff somehow... Cameron, thoughts?

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 11 years ago

Comment on attachment 805966 [details] [diff] [review]
Frame constructor changes for display:contents

>+    for (nsIContent* p = aParent; p && fm->GetDisplayContents(p);

GetDisplayContents is very confusing.  Maybe call it GetDisplayContentsStyleFor() or something?  At first glance I thought that this was checking for _kids_ of p with display:contents.

>+AutoDisplayContentsAncestorPusher::~AutoDisplayContentsAncestorPusher()
>+    mTreeMatchContext.PushStyleScope(mAncestors[i]);

PopStyleScope, right?  Please add a test that would have caught this?

>+class MOZ_STACK_CLASS AutoDisplayContentsAncestorPusher MOZ_FINAL

When is this class supposed to be used?  Needs documentation.

>nsCSSFrameConstructor.cpp
>-  items.SetTriedConstructingFrames();

Why are we removing that?  It's at least an optimization; not sure about being needed for correctness...

>+nsCSSFrameConstructor::SetAsDisplayContentsContent(FrameConstructionItemList& aList,
>+  if (aStyleContext->GetPseudo()) {

What is this block of code trying to do?  At the very least, please document.  But I worry about it leaving things in an inconsistent state if it ever runs.

>+  if (bits & (FCDATA_IS_INLINE | FCDATA_IS_CONTENTS)) {

The right way to handle this part, imo, is to detect that we're in the IS_CONTENTS case befire we do AppendItem() and in that case to go ahead and recur onto the element's kids.  That should be a lot less fragile than the manual list-wrangling going on here.

It would also pass through the right parent frame to child items, which is important in this case.  You should be able to come up with fun testcases that are broken because the kids will think they're kids of an inline with the code as-is.

> nsCSSFrameConstructor::GetFrameFor(nsIContent* aContent)

Given the changes in its behavior, this method needs a new name, and probably some documentation.

>+  nsIContent* container = GetDisplayContents(aContainer) ? aContainer : aParentFrame->GetContent();

How well does this play with XBL bindings?  Especially ones attached to the display:contents node?  :(

>+    // XXXmats no lazy frames for display:contents descendants yet.

Reference the followup bug here?

>-#ifdef DEBUG
>-  for (FCItemIterator iter(aItems); !iter.IsDone(); iter.Next()) {
>-    NS_ASSERTION(iter.item().DesiredParentType() == GetParentType(aParentFrame),
>-                 "Needed pseudos didn't get created; expect bad things");

Why is this sanity check being removed?

>+    // Hmm, if "AtStart() && aEnd.IsDone()" is true, i.e. we're moving all of
>+    // mList to aTargetList, shouldn't we append mList.mUndisplayedItems
>+    // to aTargetList.mDesiredParentCounts too?

I don't follow.  mUndisplayedItems is used to make sure we add stuff to the frame manager.... as long as it's still present when FlushUndisplayedItems happens it'll do the right thing, no?

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 11 years ago

Comment on attachment 805966 [details] [diff] [review]
Frame constructor changes for display:contents

One other note.  When searching for insertion prev/next siblings, don't we need to handle display:contents properly?  I'd expect tests to have caught this....

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 11 years ago

Comment on attachment 805978 [details] [diff] [review]
Frame manager support for display:contents

>+nsFrameManager::GetDisplayContents(nsIContent* aContent)

Like I said, this needs a better name.

And you can get bad O(N) behavior here; hopefully less often than for display:none, so let's not worry about it for now.

I'm not that happy with the code duplication here when it's the same code except for the hashtable pointer (or hashtable pointer reference) it uses.  I'd much prefer refactoring the guts of the code to avoid the duplication.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 11 years ago

Comment on attachment 805980 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

Don't we also have to deal with appends (which should happen possibly in _between_ two ::before pseudos in this new world)?

And the ::before/::after handling during restyling (e.g. detecting whether we're supposed to reframe because a ::before or ::after got added/removed needs to be fixed too.

>+        // XXXmats Can we recreate frames only for the ::after/::before content?

Not easily, so far.  We should consider fixing that.

	Boris Zbarsky [:bzbarsky]
	Comment 56 • 11 years ago

Comment on attachment 805982 [details] [diff] [review]
Replace GetParentStyleContextFrame with GetParentStyleContext which can return frame-less display:contents style contexts.

>+++ b/layout/base/RestyleManager.cpp
>@@ -1735,30 +1729,27 @@ RestyleManager::ReparentStyleContext(nsI
>     newParentContext = providerFrame->StyleContext();

Document that this is needed because we just changed its style context in the ReparentStyleContext call?

r=me

	Cameron McCormack (:heycam)
	Comment 57 • 11 years ago

Comment on attachment 802692 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

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

Some comments and questions below; I'll await answers before setting r+.

::: layout/base/RestyleManager.cpp
@@ +829,5 @@
> +                                 treeMatchContext,
> +                                 visibleKidsOfHiddenElement);
> +        restyler.DoRestyleUndisplayedDescendants(nsRestyleHint(0), aElement,
> +                                                 newContext);
> +      }

Do you need to handle accessibility notifications after the DoRestyleUndisplayedDescendants call?  Normally that's called as part of RestyleChildren.  Do we need to worry about ::before/::after in here (I have no idea actually?)?

I wonder if instead of directly calling DoRestyleUndisplayedDescendants, we call Restyle but give it a new restyle hint which means "don't restyle yourself but do restyle undisplayed children".  WDYT?  Then we could pass that down into RestyleChildren, and get the accessibility notifications called from there.  It keeps the interface of ElementRestyler a bit cleaner I think.

@@ +834,5 @@
> +      // Then process child frames for content that is a descendant of aElement.
> +      // XXX perhaps it's better to walk child frames (before reresolving
> +      // XXX undisplayed contexts above) and mark those that has a stylecontext
> +      // XXX leading up to aElement's old context? (instead of the
> +      // XXX ContentIsDescendantOf check below)

My first thought is that we should avoid ContentIsDescendantOf, since for cases where this would return false, we're going to be traversing right up to the root of the document.

But actually, can you explain in what circumstances this happens?  If it's infrequent enough, maybe it doesn't matter too much.  Or perhaps we could easily do some other check that doesn't require traversing all the way up.

@@ +859,5 @@
> +          }
> +        }
> +      }
> +      ProcessRestyledFrames(changeList);
> +    }

So maybe you can split out these changes in RestyleManager::RestyleElement within the big if statement into a separate function?  Right now RestyleElement is kind of a good size to be understandable by itself, and this bulks it out a bit.

And then split that function into three separate ones:

  * one to do the style context re-resolving
  * a small static helper to do the ancestor frame searching
  * one to do the style change computation

I think that would help the overall understandability of the changes here, and would help in the spirit of trying to keep the size of the functions in here manageable. :)

@@ +2070,5 @@
> +  , mVisibleKidsOfHiddenElement(aVisibleKidsOfHiddenElement)
> +#endif
> +{
> +}
> +

Shame we can't use C++11 constructor delegation yet...

::: layout/base/RestyleManager.h
@@ +325,5 @@
>  private:
> + friend class mozilla::RestyleManager;
> +  // For restyling undisplayed content only (mFrame==null).
> +  ElementRestyler(nsPresContext* aPresContext,
> +                  nsIContent* aParentContent,

I wonder if the name aParentContent is confusing.  An ElementRestyle is in charge of restyling a given element, and maybe its children.  And now we want to add behaviour where we restyle undisplayed children, but not the element itself.  In that context, I think it still makes sense to call this aContent, as it's the content of the subject of the restyling.  (Even if we're going to skip restyling it and only look at its children.)  You have it called aContent in the cpp file.

I don't think it's too important to hide this constructor away; after all, the aParentFrameRestyler-taking constructors don't.

	Cameron McCormack (:heycam)
	Comment 58 • 11 years ago

Comment on attachment 802692 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

Hmm, maybe I should have set r- so that it would be clear that bzbarsky's r+ is insufficient.  Bugzilla doesn't let me do this after the fact now, though, so setting f-.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 59 • 11 years ago

Comment on attachment 802681 [details] [diff] [review]
Style system support for display:contents.

IsDisplayTypeInlineOutside could use a comment explaining why you're doing what you're doing.

r=dbaron with that, and sorry for dropping this for so long

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 60 • 11 years ago

Comment on attachment 802694 [details] [diff] [review]
Don't assert about restyling non-elements directly when they are display:contents children.

I presume this is because the code in other patches in this bug that handles style changes for display:contents elements posts the changes for their children?  If so, sounds good, but maybe a comment is in order?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 61 • 10 years ago

This is now known as 'display-box: contents'; see http://dev.w3.org/csswg/css-display/#the-display-box .

	Mats Palmgren (inactive) Assignee
	Comment 62 • 10 years ago

Comment on attachment 802681 [details] [diff] [review]
Style system support for display:contents.

I've folded this into the "Style system changes" patch in bug 917313.

	Mats Palmgren (inactive) Assignee
	Comment 63 • 10 years ago

Comment on attachment 802685 [details] [diff] [review]
Put display:contents support behind a pref - disabled by default.

... and this too.

	Mats Palmgren (inactive) Assignee
	Comment 64 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #44)
> The new methods need documentation....

Fixed. I've added documentation in bug 917313.
(I'll fold this before landing)

	Mats Palmgren (inactive) Assignee
	Comment 65 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #49)
> Have you verified that all the restyling machinery actually works on
> textframes?

I've audited nsStyleChangeList.cpp and RestyleManager.cpp and it looks OK.

	Mats Palmgren (inactive) Assignee
	Comment 66 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #52)
> GetDisplayContents is very confusing.  Maybe call it
> GetDisplayContentsStyleFor() or something?

Fixed as suggested (in bug 917313).

> >+AutoDisplayContentsAncestorPusher::~AutoDisplayContentsAncestorPusher()
> >+    mTreeMatchContext.PushStyleScope(mAncestors[i]);
> 
> PopStyleScope, right?  Please add a test that would have caught this?

Oops, good catch.  (I tried to make a test but could not find one)

> >nsCSSFrameConstructor.cpp
> >-  items.SetTriedConstructingFrames();
> 
> Why are we removing that?  It's at least an optimization; not sure about
> being needed for correctness...

I spawned off the code cleanup part of that change into bug 976411.
So after that, SetTriedConstructingFrames() is only called in one place.
What I did was replacing that flag with pushing out the undisplayed items
to the frame manager at that point, since we know we *will* create frames
at that point.

Anyway, I'm not changing that code anymore - it's simpler to just
register the display-box:contents node directly with the frame manager
in the one place where it's needed.  It needs to be there *before* we
start resolving styles for children.

> >+  if (bits & (FCDATA_IS_INLINE | FCDATA_IS_CONTENTS)) {
> 
> The right way to handle this part, imo, is to detect that we're in the
> IS_CONTENTS case befire we do AppendItem() and in that case to go ahead and
> recur onto the element's kids.

Fixed as requested; and yes that's much better, thanks!

> > nsCSSFrameConstructor::GetFrameFor(nsIContent* aContent)
> 
> Given the changes in its behavior, this method needs a new name, and
> probably some documentation.

OK, I renamed it to GetInsertionFrameFor, and added a doc comment.

> >+  nsIContent* container = GetDisplayContents(aContainer) ? aContainer : aParentFrame->GetContent();
> 
> How well does this play with XBL bindings?
> Especially ones attached to the display:contents node?  :(

I haven't put much thought into that to be honest.  The frames should
become children of the nearest ancestor frame, same as for the non-XBL case.
Is that a problem?

>Comment 53 Boris Zbarsky [:bz] 2013-10-10 13:35:05 PDT
>One other note.  When searching for insertion prev/next siblings,
>don't we need to handle display:contents properly?
>Comment 55 Boris Zbarsky [:bz] 2013-10-10 13:50:50 PDT
>Don't we also have to deal with appends (which should happen possibly
>in _between_ two ::before pseudos in this new world)?

Good points, I've added code to AdjustAppendParentForAfterContent and
GetInsertionPrevSibling to deal with this.  And I've added a bunch of
tests in bug 917313 to test DOM/style changes.

	Mats Palmgren (inactive) Assignee
	Comment 67 • 10 years ago

>Comment 54 Boris Zbarsky [:bz] 2013-10-10 13:37:54 PDT
>>+nsFrameManager::GetDisplayContents(nsIContent* aContent)
>
>I'm not that happy with the code duplication here when it's the same code
>except for the hashtable pointer (or hashtable pointer reference) it uses.

Except for a few simple accessor methods, the DisplayBoxContents methods
are different.  Unlike the display:none case, these methods are recursive.
Mixing display:none/contents nodes in the same hash table is just going
to be messy, and I see no benefit.

	Mats Palmgren (inactive) Assignee
	Comment 68 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #56)
> Document that this is needed because we just changed its style context in
> the ReparentStyleContext call?

Fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 10 years ago

I'm pretty behind on reviews, and would need to page all this back in after all that time.  I doubt I'll get to this review this week.

	Mats Palmgren (inactive) Assignee
	Comment 70 • 10 years ago

Comment on attachment 805980 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

>Comment 55 Boris Zbarsky [:bz] 2013-10-10 13:50:50 PDT
>And the ::before/::after handling during restyling (e.g. detecting
>whether we're supposed to reframe because a ::before or ::after got
>added/removed needs to be fixed too.

Good point, I'm addressing this in a different patch, 
see my response to heycam below.

And I've dealt with the first part of comment 55 in the Frame constructor
patch above, so this patch is the same as before.

	Mats Palmgren (inactive) Assignee
	Comment 71 • 10 years ago

(In reply to Cameron McCormack (:heycam) from comment #57)
> Do you need to handle accessibility notifications after the
> DoRestyleUndisplayedDescendants call?  Normally that's called as part of
> RestyleChildren.  Do we need to worry about ::before/::after in here (I have
> no idea actually?)?

Yes, on both counts.  Good catch!
(I've added tests for the missing RestyleBefore/After/Pseudo calls)

> I wonder if instead of directly calling DoRestyleUndisplayedDescendants, we
> call Restyle but give it a new restyle hint which means "don't restyle
> yourself but do restyle undisplayed children".  WDYT?

That would just mess up the current methods IMO, since they are very
dependent on having a non-null 'mFrame'.  I did split out most of the
code into helper methods though, and tweaked RestyleBefore/After/Pseudo,
and the a11y methods so that I could use them for this code too.

> > +      // XXX perhaps it's better to walk child frames ...
> But actually, can you explain in what circumstances this happens?

I'll file a separate bug on that investigation if you don't mind.

>> +  ElementRestyler(nsPresContext* aPresContext,
>> +                  nsIContent* aParentContent,
>
>I wonder if the name aParentContent is confusing.

I've changed it to aContent.

>I don't think it's too important to hide this constructor away;

OK, I made it public.

	Mats Palmgren (inactive) Assignee
	Comment 72 • 10 years ago

Comment on attachment 8392920 [details] [diff] [review]
Don't assert about restyling non-elements directly when they are display:contents children.

(In reply to David Baron [:dbaron] from comment #60)
> If so, sounds good, but maybe a comment is in order?

Added a comment.

	Mats Palmgren (inactive) Assignee
	Comment 73 • 10 years ago

Comment on attachment 806174 [details] [diff] [review]
reftests

I've moved all the tests into bug 917313.

	Mats Palmgren (inactive) Assignee
	Comment 74 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=0884a4357271
https://tbpl.mozilla.org/?tree=Try&rev=ea86adde5713

	Boris Zbarsky [:bzbarsky]
	Comment 76 • 10 years ago

Comment on attachment 805980 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

>+AddGenConPseudoToFrame(nsIFrame* aOwnerFrame, nsIAtom* aPseudo,

aPseudo is unused, no?

>+    value = new T(2);

Should this be an auto array, then, to avoid the extra heap allocation?

>+        RecreateFramesForContent(ancestorFrame->GetContent(), false);

Why not "p" for that first argument?

r=me with those fixed, I guess.

	Boris Zbarsky [:bzbarsky]
	Comment 77 • 10 years ago

I still don't understand how the restyling bit works.  If I have:

  <div>
    <span style="display: contents"></span>
  </div>

And these styles:

  div::before { content: "aaa"; }
  span.foo::before { content: "bbb"; }

and I dynamically set class="foo" on the span, what makes sure the div is reframed?

	Boris Zbarsky [:bzbarsky]
	Comment 78 • 10 years ago

Comment on attachment 8392929 [details] [diff] [review]
Frame constructor changes for display:contents

> Oops, good catch.  (I tried to make a test but could not find one)

Ask heycam for how to make a test for this.

>+ * This pushes any display:contents nodes onto a TreeMatchContext.

Please write actual documentation that explains the right way/place to use this thing.  Presumably before resolving style for kids of aParent, right?

>+++ b/layout/base/RestyleTracker.cpp
>+            element->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf()||

Space before "||" please.  And yes, I know the preceding line has the same problem; we should fix it too.

>@@ -5359,16 +5367,62 @@ nsCSSFrameConstructor::AddFrameConstruct
>+      if (styleContext->GetPseudo()) {
>+        if (isGeneratedContent) {
>+          aContent->UnbindFromTree();
>+        }

I still don't see what this is trying to do, and my gut feeling is that it's wrong.  Please explain what this code is aiming for.

>+        ResolveStyleContext(styleContext, child, nullptr);

Why do you not need xbl:children wrangling similar to what ProcessChildren does here?

>+      AddFrameConstructionItemsInternal(aState, child, aParentFrame,

Why the internal version and not just AddFrameConstructionItems()?  That seems wrong to me for comment/PI kids, say.

Also, why are we not unsetting the restyle flags here like ProcessChildren would?

Basically, would it make sense to factor this bit out of ProcessChildren into a separate method that we could call from here?

> I haven't put much thought into that to be honest.  

OK, can you talk to Blake or William or both and get that sorted out?

> become children of the nearest ancestor frame, same as for the non-XBL case.
> Is that a problem?

I think so, since that means they'll ignore the insertion points the binding defines.

>+AdjustAppendParentForAfterContent(nsFrameManager* aFrameManager,
>+      aFrameManager->GetDisplayContents(aContainer)) {

I don't see how this works.  For example, in nsCSSFrameConstructor::ContentRangeInserted the thing passed for aContainer is gotten like so:

  nsIContent* container = parentFrame->GetContent();

where parentFrame is something that came from GetContentInsertionFrameFor(aContainer).  So it's already walked out of all the display:contents stuff.

Also, why do we have separate checks for ::after styles and for GetGenConPseudos(aParentFrame)?  ::after style will imply the pseudos anyway in the cases that matter here, right?

>+     (!c->IsInAnonymousSubtree() || c->GetBindingParent() != aContainer) &&

This check is not making sense to me either....  Why aContainer instead of |p| here?  At the very least, needs careful documentation.

>+        } else if (nsContentUtils::PositionIsBefore(c, aChild)) {

I assume we don't end up hitting this all the time because of the other guards on this whole block, right?

>@@ -6132,17 +6237,18 @@ nsCSSFrameConstructor::GetInsertionPrevS
>+  const bool boxContents = GetDisplayContents(aContainer);

Again, that doesn't seem right given what aContainer is.

Sorry this took so long.  It took a while to recall all the details here.  :(

If you can manage to do the next update quicker (and yes, I know now _you_ have to page stuff in), I should be able to do followup reviews quicker as well.

	Boris Zbarsky [:bzbarsky]
	Comment 79 • 10 years ago

Comment on attachment 8392932 [details] [diff] [review]
Frame manager support for display:contents

> Mixing display:none/contents nodes in the same hash table 

Er... why would we do that.

I'm just saying that, for example, SetDisplayContents() and SetUndisplayedContent() are identical except one operates on mUndisplayedMap and one operates on mDisplayContentsMap.  So why not have a single utility method that takes a reference to the UndisplayedMap* to use and call it from both places, instead of duplicating the code.  Same thing for GetDisplayContents() and ChangeDisplayContents() and arguably GetAllDisplayContentsIn.  

Obviously you can't do that for ClearDisplayContentsIn() or ClearAllDisplayContentsIn, which _are_ recursive.  That's fine; I'm not asking you to change those.  But please do share the ones that are shareable.

I'd like to see this patch updated with that change.

(Oh, and when you're merging to tip watch out for the NS_HIDDEN_ bits that need to go away.)

	Boris Zbarsky [:bzbarsky]
	Comment 80 • 10 years ago

> I think so, since that means they'll ignore the insertion points the binding defines.

And in general, it seems to me like we should decide on the interaction model of anonymous content trees and display-box:contents.  I think the obvious answer is that flattened tree construction happens _before_ display-box takes effect, since the flattened tree is technically a DOM-level concept not dependent on styles at all.  So insertion points need to be computed based on the parent before it's adjusted for display-box bits.

	Mats Palmgren (inactive) Assignee
	Comment 81 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #76)
> >+AddGenConPseudoToFrame(nsIFrame* aOwnerFrame, nsIAtom* aPseudo,
> 
> aPseudo is unused, no?

Fixed.

> >+    value = new T(2);
> 
> Should this be an auto array, then, to avoid the extra heap allocation?

Fixed.

> >+        RecreateFramesForContent(ancestorFrame->GetContent(), false);
> 
> Why not "p" for that first argument?

It's a loop variable, so it's not in scope.  I guess I could rewrite it
like so: 
    nsIContent* p = aContainer;
    for (; p; p = p->GetParent()) {
      ancestorFrame = p->GetPrimaryFrame();
      if (ancestorFrame) {
        break;
      }
    }

but that adds code complexity IMO, and "ancestorFrame->GetContent()"
is a simple inlined field access so a change is not motivated for
performance reasons (the block containing this code is only run
for 'display-box:contents' nodes).

	Mats Palmgren (inactive) Assignee
	Comment 82 • 10 years ago

Comment on attachment 8442107 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

r? in case your r+ was conditioned on changing that?

	Mats Palmgren (inactive) Assignee
	Comment 83 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #77)
> I still don't understand how the restyling bit works.  If I have:
> 
>   <div>
>     <span style="display: contents"></span>
>   </div>
> 
> And these styles:
> 
>   div::before { content: "aaa"; }
>   span.foo::before { content: "bbb"; }
> 
> and I dynamically set class="foo" on the span, what makes sure the div is
> reframed?

The <span> is restyled (RecreateFramesForContent) and its new ::before
frame is inserted in the right place as a child of the <div's frame.
The <div> isn't reframed -- why do you think it needs to be reframed?

	Mats Palmgren (inactive) Assignee
	Comment 84 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #79)
> I'm just saying that, for example, SetDisplayContents() and ...

I guess I misunderstood your original comment then...

This patch adds common utility methods for the methods you
indicated.  I'm putting this change on top of the entire
patch queue if you don't mind.  (I'll probably fold most
of these patches before landing anyway).

	Boris Zbarsky [:bzbarsky]
	Comment 85 • 10 years ago

> so a change is not motivated for performance reasons

The issue I'm having with this code is that we do not in general have the invariant that p->GetPrimaryFrame()->GetContent() == p.  So anything that assumes that invariant requires careful auditing to make sure it holds in this case.

So either that audit needs to be performed, and documented in a comment, or you shouldn't rely on this invariant.

	Boris Zbarsky [:bzbarsky]
	Comment 86 • 10 years ago

Comment on attachment 8442107 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

Let's figure out this p vs GetContent() thing.

	Boris Zbarsky [:bzbarsky]
	Comment 87 • 10 years ago

> The <span> is restyled (RecreateFramesForContent)

How do we end up in there?

> The <div> isn't reframed -- why do you think it needs to be reframed?

Hmm.  We generally reframe on dynamic ::before/::after creation to make sure we get the ordering right and because we don't have the capability to just create those bits individually very well.

I guess in this case the latter is not a problem because we're constructing all the child frames for ths kid at once, but I'd need to see some serious testcases for the behavior (including things like anonymous table stuff and whatnot) to make sure it actually works.

	Mats Palmgren (inactive) Assignee
	Comment 88 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #78)
> Please write actual documentation that explains the right way/place to use
> this thing.  Presumably before resolving style for kids of aParent, right?

Fixed.

> Space before "||" please.  And yes, I know the preceding line has the same
> problem; we should fix it too.

Fixed.

> >@@ -5359,16 +5367,62 @@ nsCSSFrameConstructor::AddFrameConstruct
> >+      if (styleContext->GetPseudo()) {
> >+        if (isGeneratedContent) {
> >+          aContent->UnbindFromTree();
> >+        }
> 
> I still don't see what this is trying to do, and my gut feeling is that it's
> wrong.  Please explain what this code is aiming for.

I'm not really sure myself - it's taken from
nsCSSFrameConstructor::SetAsUndisplayedContent
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=fffcb4bbc8b1#5270
and I think it applies for display-box:contents too.


> >+        ResolveStyleContext(styleContext, child, nullptr);
> 
> Why do you not need xbl:children wrangling similar to what ProcessChildren
> does here?

Added that.

> >+      AddFrameConstructionItemsInternal(aState, child, aParentFrame,
> 
> Why the internal version and not just AddFrameConstructionItems()?  That
> seems wrong to me for comment/PI kids, say.
>...
> Basically, would it make sense to factor this bit out of ProcessChildren
> into a separate method that we could call from here?

Ah, good catch.  I've broken up AddFrameConstructionItems() in two
parts, ShouldCreateItemsForChild() that checks if we should create
frames at all, then ResolveStyleContext() as usual, then a call
to DoAddFrameConstructionItems that contructs the items.

I'm re-using the ShouldCreateItemsForChild() and
DoAddFrameConstructionItems() parts for the display-box:contents case,
but resolving the style context is different.

> > become children of the nearest ancestor frame, same as for the non-XBL case.
> > Is that a problem?
> 
> I think so, since that means they'll ignore the insertion points the binding
> defines.

OK, how about we split out the display-box:contents for XBL trees
to a follow-up bug?  it doesn't affect ordinary web pages does it?

> >+AdjustAppendParentForAfterContent(nsFrameManager* aFrameManager,
> >+      aFrameManager->GetDisplayContents(aContainer)) {
> 
> I don't see how this works.  For example, in
> nsCSSFrameConstructor::ContentRangeInserted the thing passed for aContainer
> is gotten like so:
> 
>   nsIContent* container = parentFrame->GetContent();
> 
> where parentFrame is something that came from
> GetContentInsertionFrameFor(aContainer).  So it's already walked out of all
> the display:contents stuff.

Yes, 'parentFrame' is the actual container frame where we will insert the
child frames.  There may be an arbitrary amount of (frame-less) content
nodes between the child node and the container node.  I'm sorry, I don't
understand what you think isn't working.

> Also, why do we have separate checks for ::after styles and for
> GetGenConPseudos(aParentFrame)?
> ::after style will imply the pseudos anyway
> in the cases that matter here, right?

The check for ::after pseudo is needed because the container frame
may NOT be the primary frame, and GetGenConPseudos for a frame's *own*
::after/::before frames is stored on the primary.  The GetGenConPseudos
test is needed because ::after/::before frames that has "bubbled up"
from descendant display-box:contents nodes can be stored on
continuations too.  I guess it might be slightly more performant to
check GetGenConPseudos first though, in case it's the primary frame,
which I assume is the common case.  Assuming frame property lookup
is faster than checking for ::after pseudo style.  Is it?

> >+     (!c->IsInAnonymousSubtree() || c->GetBindingParent() != aContainer) &&
> 
> This check is not making sense to me either....  Why aContainer instead of
> |p| here?  At the very least, needs careful documentation.

The IsInAnonymousSubtree is redundant there so I removed it.
The "c->GetBindingParent() != aContainer" is correct since we
only want to "step over" ::after frames that is for aContainer.
We should stop at an ::after frame that is PositionIsBefore
aChild.  Does that clarify it?

> >+        } else if (nsContentUtils::PositionIsBefore(c, aChild)) {
> 
> I assume we don't end up hitting this all the time because of the other
> guards on this whole block, right?

Correct.

> >@@ -6132,17 +6237,18 @@ nsCSSFrameConstructor::GetInsertionPrevS
> >+  const bool boxContents = GetDisplayContents(aContainer);
> 
> Again, that doesn't seem right given what aContainer is.

I don't see what's wrong with it.  Can you explain _why_
you think it's wrong and perhaps suggest what I should
use instead?

	Boris Zbarsky [:bzbarsky]
	Comment 89 • 10 years ago

Another related question.  With this markup:

  <div>
    <span style="display: contents"></span>
  </div>

And these styles:

  span::before { content: "xyz"; }

what happens if I later set "display: none" on the span?  Or just remove it from the DOM?  The changes to ContentRemoved seem to transform this into a ContentRemoved on its flattened kids, but the ::before is not one of its kids in the flattened tree.

	Boris Zbarsky [:bzbarsky]
	Comment 90 • 10 years ago

Comment on attachment 8442117 [details] [diff] [review]
Make a few similar nsFrameManager methods share a common utility method.

The GetStyleContextInMap/GetAllUndisplayedNodesInMapFor/SetStyleContextInMap/ChangeStyleContextInMap methods should probably be static, to make sure they don't poke the wrong member.

r=me with that

	Boris Zbarsky [:bzbarsky]
	Comment 91 • 10 years ago

Comment on attachment 8442176 [details] [diff] [review]
Frame constructor changes for display:contents.

> and I think it applies for display-box:contents too.

That code in SetAsUndisplayedContent is to handle the case when we have a pseudo-element like ::before that we're not going to actually create a frame for.  Really, the only case that happens in is a non-display-column child of a display:column-group.  The point is that we're just suppressing this thing, and we're suppressing it pretty permanently, so there's no reason to add it to the undisplayed map.

But in the display:contents case, we're not suppressing the thing altogether; we still want to render its kids.  So unbinding it seems wrong. Did you write any tests for ::before { display-box: contents; }?


> I've broken up AddFrameConstructionItems() in two
> parts, ShouldCreateItemsForChild() that checks if we should create
> frames at all, then ResolveStyleContext() as usual, then a call
> to DoAddFrameConstructionItems that contructs the items.

That's basically a standalone patch right?  Mind either posting that standalone or at least posting an interdiff vs the previous patch revision here?

> OK, how about we split out the display-box:contents for XBL trees
> to a follow-up bug?

I'm not really ok with that, because I think the current setup in the patch will lead to violated invariants and crashes.

More importantly, I would _really_ like us to have a clear idea of how the various pieces here are supposed to interact before we land a bunch of broken code and then forget about it for months...

> it doesn't affect ordinary web pages does it?

It does if we ever want to ship webcomponents.

> I'm sorry, I don't understand what you think isn't working.

aFrameManager->GetDisplayContents(aContainer) can't ever return anything, because aContainer is something that's already been adjusted by walking upwards out of all the display:contents stuff.  So whatever code is guarded by that check is not going to run.  Either that code is not needed and needs to be removed or (more likely) we're not checking the right thing here.

> The GetGenConPseudos test is needed because ::after/::before frames that has
> "bubbled up" from descendant display-box:contents nodes can be stored on
> continuations too. 

This all needs documenting in the code.  And I'll have to triple-check that this handles continuations right in general...

> Assuming frame property lookup is faster than checking for ::after pseudo style.

It generally is.

> since we only want to "step over" ::after frames that is for aContainer.

No, we want to "step over" ::after frames that are for any ancestor of aChild, I'd think.

> We should stop at an ::after frame that is PositionIsBefore aChild

Yes, but I'm not sure what aContainer has to do with that.

> Does that clarify it?

No.  And again, this needs careful documentation in the code.  In fact, some careful documentation somewhere of what the general frame structure that's produced here looks like would be good.

> Can you explain _why_ you think it's wrong

Because "boxContents" always ends up false.

> and perhaps suggest what I should use instead?

I can't, because I'm not sure what this code is trying to do, in general.  Starting with that written down somewhere in English, not C++, would be very helpful.

	Mats Palmgren (inactive) Assignee
	Comment 92 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #90)
> Comment on attachment 8442117 [details] [diff] [review]
> Make a few similar nsFrameManager methods share a common utility method.
> 
> The
> GetStyleContextInMap/GetAllUndisplayedNodesInMapFor/SetStyleContextInMap/
> ChangeStyleContextInMap methods should probably be static, to make sure they
> don't poke the wrong member.

Fixed.  There was a use of mPresShell in one of the assertions
that I replaced with aStyleContext->PresContext()->PresShell(),
and "aMap == mUndisplayedMap" in the DEBUG_*_MAP logging code
which I simplified so it's not needed.

	Mats Palmgren (inactive) Assignee
	Comment 93 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #91)
> But in the display:contents case, we're not suppressing the thing
> altogether; we still want to render its kids.  So unbinding it seems wrong.

I see, thanks for explaining it, I'll remove that bit then.

> Did you write any tests for ::before { display-box: contents; }?

Yes, both display-box-contents-generated-content.html and
display-box-contents-generated-content-2.html (with mutations)
in the test patch tests ::before/::after with 'display-box:contents'
extensively.

	Boris Zbarsky [:bzbarsky]
	Comment 94 • 10 years ago

How did those tests pass with this unbind bit, then?

	Jet Villegas (inactive)
	Comment 95 • 10 years ago

William: per our lunch chat, this is the bug that has open implementation questions re: XBL Trees. Can you help Mats flesh out what's needed to enable support for that?

	Mats Palmgren (inactive) Assignee
	Comment 96 • 10 years ago

BTW, there's a proposal from fantasai/Tab to change 'display-box':
http://lists.w3.org/Archives/Public/www-style/2014Jul/0023.html

	Mats Palmgren (inactive) Assignee
	Comment 98 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #89)
> what happens if I later set "display: none" on the span?  Or just remove it
> from the DOM? 

The answer in both cases is that the ancestor <div> is reframed because
it's hosting child frames for generated content (GetGenConPseudos).
Here are the stacks for each case.

https://bugzilla.mozilla.org/attachment.cgi?id=8442107&action=diff#a/layout/base/nsCSSFrameConstructor.cpp_sec4

	Boris Zbarsky [:bzbarsky]
	Comment 99 • 10 years ago

Aha, thank you.  That makes sense.

	Mats Palmgren (inactive) Assignee
	Comment 100 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #78)
> >+AdjustAppendParentForAfterContent(nsFrameManager* aFrameManager,
> >+      aFrameManager->GetDisplayContents(aContainer)) {
> 
> I don't see how this works.  For example, in
> nsCSSFrameConstructor::ContentRangeInserted the thing passed for aContainer
> is gotten like so:
> 
>   nsIContent* container = parentFrame->GetContent();
> 
> where parentFrame is something that came from
> GetContentInsertionFrameFor(aContainer).  So it's already walked out of all
> the display:contents stuff.

Here's a testcase where GetDisplayContents(aContainer) is non-null
in AdjustAppendParentForAfterContent.  The call is from
nsCSSFrameConstructor::ContentAppended.

	Mats Palmgren (inactive) Assignee
	Comment 101 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #85)
> The issue I'm having with this code is that we do not in general have the
> invariant that p->GetPrimaryFrame()->GetContent() == p.  So anything that
> assumes that invariant requires careful auditing to make sure it holds in
> this case.

OK, I've change it to use 'p' as you suggested, although I renamed it to
'ancestor' to associate it with 'ancestorFrame'.  Interdiff:

< +    for (nsIContent* p = aContainer; p && !ancestorFrame; p = p->GetParent()) {
< +      ancestorFrame = p->GetPrimaryFrame();
---
> +    nsIContent* ancestor = aContainer;
> +    for (; ancestor; ancestor = ancestor->GetParent()) {
> +      ancestorFrame = ancestor->GetPrimaryFrame();
> +      if (ancestorFrame) {
> +        break;
> +      }
117c121
< +        RecreateFramesForContent(ancestorFrame->GetContent(), false);
---
> +        RecreateFramesForContent(ancestor, false);

	Mats Palmgren (inactive) Assignee
	Comment 102 • 10 years ago

Removed the bogus UnbindFromTree() bit I had earlier.  Interdiff:

< +      if (styleContext->GetPseudo()) {
< +        if (isGeneratedContent) {
< +          aContent->UnbindFromTree();
< +        }
< +      } else {
< +        MOZ_ASSERT(!isGeneratedContent, "Should have had pseudo type");
< +        aState.mFrameManager->SetDisplayContents(aContent, styleContext);
< +      }
---
> +      MOZ_ASSERT(styleContext->GetPseudo() || !isGeneratedContent,
> +                 "Should have had pseudo type");
> +      aState.mFrameManager->SetDisplayContents(aContent, styleContext);


And reordered the GetGenConPseudos() / nsLayoutUtils::HasPseudoStyle
calls per:

(In reply to Boris Zbarsky [:bz] from comment #91)
> > Assuming frame property lookup is faster than checking for ::after pseudo style.
> 
> It generally is.

	Mats Palmgren (inactive) Assignee
	Comment 103 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #91)
> Because "boxContents" always ends up false.

No, I believe you are mistaken.

	Boris Zbarsky [:bzbarsky]
	Comment 104 • 10 years ago

OK, can you explain to me why my claim about ContentRangeInserted in comment 78 is not true?  I'll admit that at this point I have no clear mental model of what your code is trying to do in the latest version of the patches...

	William Chen [:wchen]
	Comment 105 • 10 years ago

Comment on attachment 8455290 [details] [diff] [review]
Frame constructor changes for display:contents.

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6344,5 @@
>    // the preceding frame.
>  
>    NS_PRECONDITION(aParentFrame, "Must have parent frame to start with");
> +  const bool boxContents = GetDisplayContents(aContainer);
> +  nsIContent* container = boxContents ? aContainer : aParentFrame->GetContent();

As bz pointed out, this is probably not what we want here. In the case of XBL and Shadow DOM, if the container is a node with a binding, it may be the case that the child is rendered as a child of some other node (wherever the binding puts the content insertion point, in the case of XBL this is a <xbl:children/> element), thus the sibling doesn't necessarily come from the container, but rather the flattened parent of the insertion point where it is rendered.

I haven't looked at where else this issue may affect display:contents since I don't understand everything going on in these patches, but I can certainly help answer questions about it.

	Mats Palmgren (inactive) Assignee
	Comment 106 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #104)
> OK, can you explain to me why my claim about ContentRangeInserted in comment
> 78 is not true?

Your claim about the call from ContentRangeInserted *is* true.
It's just that there are other calls to GetInsertionPrevSibling
where "boxContents" is true, see comment 103.

	Mats Palmgren (inactive) Assignee
	Comment 107 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #94)
> How did those tests pass with this unbind bit, then?

Because 'display-box:contents' is ignored for pseudos:
https://bugzilla.mozilla.org/attachment.cgi?id=8392956&action=diff#a/layout/style/nsRuleNode.cpp_sec3

Allowing ::before/::after to pass there leads to the problem you
pointed out - it fatally asserts in nsIContent::SetPrimaryFrame:
MOZ_ASSERT(IsInDoc() || HasFlag(NODE_IS_IN_SHADOW_TREE), "This will end badly!");

Does that make sense?

Removing the Unbind bit fixed that, but I'm not sure if supporting
'display-box:contents' for ::before/::after is a good idea.
It adds complexity and I'm not sure what the benefit is.

	Boris Zbarsky [:bzbarsky]
	Comment 110 • 10 years ago

> Your claim about the call from ContentRangeInserted *is* true.

OK, then don't we get wrong behavior when coming via that codepath?

> Does that make sense?

Yes, that's much clearer.  ;)

	Mats Palmgren (inactive) Assignee
	Comment 111 • 10 years ago

(In reply to William Chen [:wchen] from comment #105)
Thanks, that's helpful.  I think it makes much more sense if someone
who are familiar with these things makes the necessary changes to 
GetInsertionPrevSibling and AdjustAppendParentForAfterContent so that they
work correctly for the XBL and Shadow DOM cases though.

	Mats Palmgren (inactive) Assignee
	Comment 112 • 10 years ago

I'm not going to work more on this until the CSS spec situation is clear,
(see comment 96).  I suspect we're going to have to do bug 1038294 first.

	Jet Villegas (inactive)
	Comment 113 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #112)
> I'm not going to work more on this until the CSS spec situation is clear,
> (see comment 96).  I suspect we're going to have to do bug 1038294 first.

AIUI, the spec changes proposed are only style-system syntax changes and won't affect the actual Layout of display:xxx. Other than the changes required to make GetInsertionPrevSibling and AdjustAppendParentForAfterContent work, what else is this patch series missing to implement the _current_ spec?

	Mats Palmgren (inactive) Assignee
	Comment 114 • 10 years ago

(In reply to Jet Villegas (:jet) from comment #113)
> AIUI, the spec changes proposed are only style-system syntax changes and
> won't affect the actual Layout of display:xxx. Other than the changes
> required to make GetInsertionPrevSibling and
> AdjustAppendParentForAfterContent work,

Making those work for XBL / Shadow DOM trees, yes.
(They currently work fine for normal content.)

Fixing this part isn't dependent on the CSS syntax (if that's
what you meant).

> what else is this patch series
> missing to implement the _current_ spec?

For Layout? nothing else, as far as I know.

But the current spec:
http://dev.w3.org/csswg/css-display/#the-display-properties
says 'contents' is now a value of 'display-outside' (that can
also be used through the 'display' shorthand).

Shipping display-box:contents is not an option as I see it,
since 'contents' was removed from there:
http://dev.w3.org/csswg/css-display/#the-display-box

It seems like asking the CSSWG for a decision on the proposal
would be a good first step.  Maybe dbaron can help decide the
CSS syntax we should implement here?

Once that's clear, I can fixup the style system bits after
I come back from vacation, if no one else gets to it first.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 115 • 10 years ago

I think you should try to get this done sooner rather than later; which property the value is on is easy to change when the WG decides, and you shouldn't delay getting the code in (which will just make it bitrot, or make other people's changes bitrot).

Yes, getting the spec situation sorted out is a prerequisite for turning it on.  But I think actually doing the display-inside/display-outside split (bug 1038294) is not a prerequisite.

	Mats Palmgren (inactive) Assignee
	Comment 116 • 10 years ago

OK, let's land it as 'display-box:contents' then with the pref turned off by default.

What remains here is to add support for XBL / Shadow trees; I did suggest to spawn
that off as a follow-up bug, but bz rejected that.  Since I simply don't know how
to implement that part I think I've taken this bug as far as I can.
(see comment 105 / 111 for what needs to be fixed)

	Boris Zbarsky [:bzbarsky]
	Comment 117 • 10 years ago

What remains here is to get some sort of handle on a principled way this stuff should work.  I currently have pretty much zero confidence that I can sanely review the code and catch basic stuff like exploitable security bugs in it.  :(

	Mats Palmgren (inactive) Assignee
	Comment 118 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #116)
> What remains here ...

Oh, and the review request for :heycam in comment 71 is still pending.

	Mats Palmgren (inactive) Assignee
	Comment 119 • 10 years ago

Here's an up-to-date version of that patch.

	Mats Palmgren (inactive) Assignee
	Comment 120 • 10 years ago

dbaron did review most of this in an earlier patch but
I've made one minor addition to address dholbert's comment 33:
> Why do we still need "plus NS_STYLE_DISPLAY_CONTENTS" here? ...

I've removed that bit and made EnsureBlockDisplay ignore
display:contents (same as display:none).  I think this is
what the spec calls for (1st paragraph):
http://dev.w3.org/csswg/css-display/#transformations

(I didn't address it earlier b/c when 'contents' lived on
a separate property ('box' or 'display-box') it wasn't
necessary (comment 37))

	Mats Palmgren (inactive) Assignee
	Comment 121 • 10 years ago

No significant changes except for the removal of the
NS_STYLE_DISPLAY_CONTENTS bit pointed out in comment 33.

	Mats Palmgren (inactive) Assignee
	Comment 122 • 10 years ago

Updated to tip.

	Mats Palmgren (inactive) Assignee
	Comment 123 • 10 years ago

No significant changes since last time... except for the changes
to handle XBL/Shadow root insertion points in the upcoming patch.

	Mats Palmgren (inactive) Assignee
	Comment 124 • 10 years ago

New patch to handle XBL/Shadow tree insertion points properly.
I've made nsCSSFrameConstructor::Get[Range]InsertionPoint return
not just the parent frame but also the content insertion point
in a POD struct "InsertionPoint" and propagate this result and
use its content container instead of "parentFrame->GetContent()".

	Mats Palmgren (inactive) Assignee
	Comment 125 • 10 years ago

Here's the above two patches combined as one - feel free to
use this patch instead when reviewing.  (I'll fold these two
before landing.)

	Mats Palmgren (inactive) Assignee
	Comment 126 • 10 years ago

Merged in the code sharing patch.  No significant changes otherwise.

	Mats Palmgren (inactive) Assignee
	Comment 127 • 10 years ago

See comment 101.

	Mats Palmgren (inactive) Assignee
	Comment 128 • 10 years ago

Updated to tip.

	Mats Palmgren (inactive) Assignee
	Comment 129 • 10 years ago

One minor change since last time:  I changed the nsIFrame** out-param
on ElementRestyler::RestyleSelf to nsStyleContext** instead to
account for the display:contents case.  Feel free to review if
you wish, otherwise I'll assume the last r+ is carried forward.

	Mats Palmgren (inactive) Assignee
	Comment 130 • 10 years ago

Updated to tip.

	Mats Palmgren (inactive) Assignee
	Comment 131 • 10 years ago

Added numerous XBL/Shadow tree tests.

https://tbpl.mozilla.org/?tree=Try&rev=29456222bcd9
https://tbpl.mozilla.org/?tree=Try&rev=a3130e475f0c

	Boris Zbarsky [:bzbarsky]
	Comment 134 • 10 years ago

Comment on attachment 8492596 [details] [diff] [review]
Frame constructor support for multiple ::before/::after per frame; deal with them when display:contents is involved.

r=me

	Mats Palmgren (inactive) Assignee
	Comment 137 • 10 years ago

With only part 1, all reftests pass, except the XBL/Shadow DOM tests which fails.

	Mats Palmgren (inactive) Assignee
	Comment 138 • 10 years ago

As requested, I've reordered the Fctor patches slightly.
The first part is idempotent and pass Try tests *by itself*:
https://tbpl.mozilla.org/?tree=Try&rev=b295f8bd1839

	Mats Palmgren (inactive) Assignee
	Comment 139 • 10 years ago

I think you've reviewed most (if not all) of this before, except now
it's on top of the refactoring so it may look slightly different.
This passes the relevant reftests locally, except the added XBL/Shadow
tree tests.

	Mats Palmgren (inactive) Assignee
	Comment 140 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=8f781250e66e

	Mats Palmgren (inactive) Assignee
	Comment 141 • 10 years ago

Again, feel free to review the "Frame constructor patches combined" instead of these
three separate patches if you find that easier.  (It's identical except for a param
name typo I fixed: s/insertion/aInsertion/)

	Boris Zbarsky [:bzbarsky]
	Comment 142 • 10 years ago

Yep, thank you.  And thank you for the try link; that will let me fold patches as desired and so forth.

	Mats Palmgren (inactive) Assignee
	Comment 143 • 10 years ago

Here's a zip archive with the current patches.  "my-applied-series" has the order.

	Boris Zbarsky [:bzbarsky]
	Comment 144 • 10 years ago

Comment on attachment 8493953 [details] [diff] [review]
part 1 of the Fctor changes - the InsertionPoint refactoring (idempotent)

>+++ b/layout/base/nsCSSFrameConstructor.cpp
> nsCSSFrameConstructor::ResolveStyleContext(nsIFrame*         aParentFrame,
>-                                           nsIContent*       aContent,
>+                                           nsIContent*       aContainer,

Please document the relationship between aParentFrame and aContainer.

>@@ -6820,19 +6842,20 @@ nsCSSFrameConstructor::ContentAppended(n
>+  parentFrame = insertion.mParentFrame;

So it looks like we modify parentFrame a few times, then have to go and change insertion.mParentFrame to match.

Either those modifications should modify both, or better yet we should make parentFrame an |nsIFrame*&| which is a reference to insertion.mParentFrame, so the two are always in sync.

This does require hoisting "insertion" up a bit higher, or renaming the parentFrame uses that come before it (to candidateParentFrame?), or scoping down that earlier parentFrame, but any of those is OK by me, I think.

>@@ -7244,111 +7268,114 @@ nsCSSFrameConstructor::ContentRangeInser
>+  {
>+    nsContainerFrame* parentFrame = ::GetContentInsertionFrameFor(aContainer);

Add a comment about how this is a scope for parentFrame so it won't be visible to the rest of the function?

>+  nsIFrame* prevSibling = GetInsertionPrevSibling(&insertion, aStartChild,
>                             &isAppend, &isRangeInsertSafe);

Please fix the indent on the second line there.

>+nsCSSFrameConstructor::GetInsertionPoint(nsIContent* aContainer,
>+                                         nsIContent* aChildContent,
>+                                         bool        aGetMultiple)

Why is the aGetMultiple argument needed?  Seems like we can always just set mMultiple in the InsertionPoint to true when we want to; the only reason this used to be conditional is to avoid forcing callers  to declare a bool they don't care about and a pointer to it.

As in, I think it should be fine to remove aGetMultiple and simplify the code by setting aGetMultiple to true everywhere and then getting rid of things as needed.

>     if (nsContentUtils::HasDistributedChildren(aContainer)) {
>+      return InsertionPoint(nullptr, nullptr, true);

Why not aContainer for the second arg?  If there's a reason, please document ut here.

>     if (multiple) {
>+      return InsertionPoint(nullptr, nullptr, aGetMultiple);

Same here.

>+++ b/layout/base/nsCSSFrameConstructor.h
>+  struct InsertionPoint

Please document this.  In particular, how mParentFrame and mContainer are related and what mMultiple means.

r=me.  Thank you for splitting this out!  That makes this much simpler.

	Boris Zbarsky [:bzbarsky]
	Comment 145 • 10 years ago

> Please document the relationship between aParentFrame and aContainer.

And how aContainer differs from aChild->GetParent(), for that matter.

	Boris Zbarsky [:bzbarsky]
	Comment 146 • 10 years ago

Comment on attachment 8493956 [details] [diff] [review]
part 2 of the Fctor changes - the main part, except XBL/Shadow tree fixes

> AutoDisplayContentsAncestorPusher

So looking at this again, I'm confused.

Why are we only pushing style scopes when HasFilter()?  Shouldn't we always do that?

Is there a test that tests this codepath when !HasFilter()?

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -3690,29 +3668,31 @@ nsCSSFrameConstructor::ConstructFrameFro
>+  if (adcp.IsEmpty() && parent && nsContentUtils::IsContentInsertionPoint(parent)) {

Please document that if adcp.IsEmpty() is false that means parent must have been pushed already by AutoDisplayContentsAncestorPusher?

> nsCSSFrameConstructor::ResolveStyleContext(nsIFrame*         aParentFrame,
>+  if (MOZ_LIKELY(!parentStyleContext)) {

Then can we assert aParentFrame->GetContent() == aContainer?  If not, why not?

I'd think we better be able to, if aContainer is supposed to be "the thing we inherit style from" and we plan to use aParentFrame's style context as the parentStyleContext.

>@@ -5577,16 +5594,76 @@ nsCSSFrameConstructor::AddFrameConstruc
>+      nsRefPtr<nsStyleContext> childContext =
>+        ResolveStyleContext(styleContext, child, nullptr);

Why nullptr for the last arg?  If there's a reason, document.

>+AdjustAppendParentForAfterContent(nsFrameManager* aFrameManager,
>+  // frames to find the first one that is either a ::after frame for an
>+  // ancestor of aChild or a frame that is for a node later in the
>+  // document than aChild and return that in aAfterFrame.

OK.  So the key is that we never really get to this code when we're doing something that involves insertion points that can reorder kids, right?

If so, please document that here, because otherwise it's not obvious why those PositionIsBefore checks are OK.

>@@ -6363,29 +6488,38 @@ nsCSSFrameConstructor::GetInsertionPrevS
>+    if (GetDisplayContentsStyleFor(aInsertion->mContainer)) {
>+      if (nextSibling) {

If both of these are true, why is it OK to return without setting *aIsRangeInsertSafe?

Please document the goal of this block.  Specifically, why it is that we have to reset aInsertion->mParentFrame if we have a nextSibling and that we have to look for what would have been our parent's nextSibling if we don't have one and our parent has "display:contents".

That said, can we common up the two cases when nextSibling is non-null, whether GetDisplayContentsStyleFor(aInsertion->mContainer) is true or not?  That depends on what we want to set *aIsRangeInsertSafe to in this case, right?

r=me on the combination of part 2 and part 3 of the frame constructor changes with the above addressed.  Thank you for your patience!

	Cameron McCormack (:heycam)
	Comment 147 • 10 years ago

Comment on attachment 8492588 [details] [diff] [review]
Style system support for display:contents.

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

::: layout/style/nsRuleNode.cpp
@@ +5515,5 @@
> +    nsIAtom* pseudo = aContext->GetPseudo();
> +    if (pseudo && display->mDisplay == NS_STYLE_DISPLAY_CONTENTS) {
> +      // We don't want to create frames for anonymous content using a parent
> +      // frame that is for content above the root of the anon tree.
> +      // (XXX what we realy should check here is not GetPseudo() but if there's

"really"

Can you make sure to file a followup bug to work out what we want to do for pseudos?  We could also store a bit on the style context if we create it for the root of an anonymous tree.

@@ +5517,5 @@
> +      // We don't want to create frames for anonymous content using a parent
> +      // frame that is for content above the root of the anon tree.
> +      // (XXX what we realy should check here is not GetPseudo() but if there's
> +      //  a 'content' property value that implies anon content but we can't
> +      //  check that here since that's a different struct(?))

Yeah, I'm not sure how difficult that would be to get working.  We compute the Font struct first which lets us resolve em units etc. in all the other structs.  Would swapping the order of Display and Content in layout/style/generate-stylestructlist.py work?  I don't really know...

	Cameron McCormack (:heycam)
	Comment 148 • 10 years ago

Actually, what is the plan for landing here given that the property has changed (again) to be box-suppress:discard?

	Cameron McCormack (:heycam)
	Comment 149 • 10 years ago

Comment on attachment 8492600 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

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

Firstly, sorry for the delay in reviewing.  I'm just looking at the RestyleManager changes again.

::: layout/base/RestyleManager.cpp
@@ +3184,5 @@
> +  nsStyleContext* aNewContext,
> +  nsChangeHint    aMinHint,
> +  RestyleTracker& aRestyleTracker,
> +  nsRestyleHint   aRestyleHint,
> +  RestyleManager* aRestyleManager)

I'm not sure I really like that we're doing some work in this function that the RestyleManager normally does, such as calling ComputeStyleChangeFor; no other methods on ElementRestyler deal with the RestyleManager currently.  Could it make sense to create an ElementRestyler for each child and do the ComputeStyleChangeFor calls up in ElementRestyler::RestyleElement?  I think ideally, as I mentioned in an earlier comment, we should be able to create a single ElementRestyler for the display:contents node that would know how to deal with its children.

@@ +3187,5 @@
> +  nsRestyleHint   aRestyleHint,
> +  RestyleManager* aRestyleManager)
> +{
> +  const bool mightReframePseudos = aRestyleHint & eRestyle_Subtree;
> +  InitializeAccessibilityNotifications(aNewContext);

Should this InitializeAccessibilityNotifications call go in an |if (!(mHintsHandled & nsChangeHint_ReconstructFrame))| too?

::: layout/base/RestyleManager.h
@@ +468,5 @@
> +   * Called from RestyleManager::RestyleElement to restyle children of
> +   * a display:contents element.
> +   */
> +  void RestyleChildrenOfDisplayContentsElement(nsIFrame* aParentFrame,
> +                                               nsStyleContext* aNewContext,

Line up all argument names or none.

@@ +523,5 @@
>     */
>    void RestyleUndisplayedDescendants(nsRestyleHint aChildRestyleHint);
>    void DoRestyleUndisplayedDescendants(nsRestyleHint aChildRestyleHint,
> +                                       nsIContent* aParent,
> +                                       nsStyleContext* aParentStyleContext);

Can you add a comment somewhere here explaining why we need to pass in the style contexts?

	Mats Palmgren (inactive) Assignee
	Comment 150 • 10 years ago

(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #148)
> Actually, what is the plan for landing here given that the property has
> changed (again) to be box-suppress:discard?

As I see it, 'box-suppress' is orthogonal to display:contents, so the plan is
to land it as soon as I have r+ on all the patches here.  'box-suppress' will
be implemented separately in bug 1070507. (box-suppress:discard is tied to
display:none, not display:contents)

	Mats Palmgren (inactive) Assignee
	Comment 151 • 10 years ago

(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #149)
> Comment on attachment 8492600 [details] [diff] [review]
> Make RestyleManager::RestyleElement and
> nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with
> display:contents elements.
> 
> I'm not sure I really like that we're doing some work in this function that
> the RestyleManager normally does, such as calling ComputeStyleChangeFor

OK, it turns out that ComputeStyleChangeFor only use mPresContext from
RestyleManager, which indicates to me it didn't really belong here in
the first place.  I've moved it to ElementRestyler, verbatim, except
for "nsPresContext* presContext = aFrame->PresContext();" and using
that instead of mPresContext.

I also moved the RestyleElement part into a separate method, overloading
ComputeAndProcessStyleChange, so there is one for the "nsIFrame*" case
and one for the "nsStyleContext* + Element*" (display:contents) case.

> ideally, ... we should be able to create a
> single ElementRestyler for the display:contents node that would know how to
> deal with its children.

OK, that is *exactly* what the new ComputeAndProcessStyleChange does:
it creates 'ElementRestyler r' for the display:contents node and then
calls r.RestyleChildrenOfDisplayContentsElement to deal with its children.

> > +  nsRestyleHint   aRestyleHint,
> > +  RestyleManager* aRestyleManager)
> > +{
> > +  const bool mightReframePseudos = aRestyleHint & eRestyle_Subtree;
> > +  InitializeAccessibilityNotifications(aNewContext);
> 
> Should this InitializeAccessibilityNotifications call go in an |if
> (!(mHintsHandled & nsChangeHint_ReconstructFrame))| too?

At the start of this method, that condition should always be true.
(I now MOZ_ASSERT that at the top.)  But yeah, I moved that call
a bit later in this method, under an 'if' that tests that.

I've fixed your other nits.

It may look like this patch is very different from the last one, but
that's not true, I've mostly shuffled code around.  The one thing
that is significant, and need review, is that I discovered some new
"mReframingStyleContexts" stuff in ComputeAndProcessStyleChange that
wasn't there when I originally wrote this patch.  I'm assuming that
I should discard that state in the same way for the display:contents
path.

	Mats Palmgren (inactive) Assignee
	Comment 152 • 10 years ago

I wrote a bunch more tests and discovered a case that I missed:
inserting content next to a display:contents node that has
descendants with frames.  FindPrevious[Next]Sibling shouldn't
just skip siblings that have no frame, it needs to recurse
descendants if it's a display:contents node (unlike display:none).

I thought you might want to review this fix standalone, since
it's not really touching other code that is changed.  I'll fold
this into the Frame constructor patches later.

	Mats Palmgren (inactive) Assignee
	Comment 153 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #146)
> Comment on attachment 8493956 [details] [diff] [review]
> part 2 of the Fctor changes - the main part, except XBL/Shadow tree fixes
> 
> > AutoDisplayContentsAncestorPusher
> 
> Why are we only pushing style scopes when HasFilter()?  Shouldn't we always
> do that?

I changed it to always push style scopes, but ancestors only when
HasFilter() is true.

> Is there a test that tests this codepath when !HasFilter()?

Probably not, I'm happy to write one if you can think of something
that would have been broken by the last version.

> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> >@@ -3690,29 +3668,31 @@ nsCSSFrameConstructor::ConstructFrameFro
> >+  if (adcp.IsEmpty() && parent && nsContentUtils::IsContentInsertionPoint(parent)) {
> 
> Please document that if adcp.IsEmpty() is false that means parent must have
> been pushed already by AutoDisplayContentsAncestorPusher?

Fixed.

> > nsCSSFrameConstructor::ResolveStyleContext(nsIFrame*         aParentFrame,
> >+  if (MOZ_LIKELY(!parentStyleContext)) {
> 
> Then can we assert aParentFrame->GetContent() == aContainer?

No, we can't.  I'm still investigating this bit... let me come back
to it in a later comment.

> >@@ -5577,16 +5594,76 @@ nsCSSFrameConstructor::AddFrameConstruc
> >+      nsRefPtr<nsStyleContext> childContext =
> >+        ResolveStyleContext(styleContext, child, nullptr);
> 
> Why nullptr for the last arg?  If there's a reason, document.

Don't remember if there ever was a reason.  I changed it to &aState.

> >+AdjustAppendParentForAfterContent(nsFrameManager* aFrameManager,
> >+  // frames to find the first one that is either a ::after frame for an
> >+  // ancestor of aChild or a frame that is for a node later in the
> >+  // document than aChild and return that in aAfterFrame.
> 
> OK.  So the key is that we never really get to this code when we're doing
> something that involves insertion points that can reorder kids, right?

I don't know, can you provide a testcase that does that so I can investigate?

> >@@ -6363,29 +6488,38 @@ nsCSSFrameConstructor::GetInsertionPrevS
> >+    if (GetDisplayContentsStyleFor(aInsertion->mContainer)) {
> >+      if (nextSibling) {
> 
> If both of these are true, why is it OK to return without setting
> *aIsRangeInsertSafe?

It's not.  Fixed.

> Please document the goal of this block.  Specifically, why it is that we
> have to reset aInsertion->mParentFrame if we have a nextSibling and that we
> have to look for what would have been our parent's nextSibling if we don't
> have one and our parent has "display:contents".

I assigned aInsertion->mParentFrame to pick up the result from the
recursive call in case it might be different.  I can't find a case
where it is though, so I'm now asserting it's the same.

> That said, can we common up the two cases when nextSibling is non-null,
> whether GetDisplayContentsStyleFor(aInsertion->mContainer) is true or not? 
> That depends on what we want to set *aIsRangeInsertSafe to in this case,
> right?

I rewrote the code a bit so the 'nextSibling' case now falls through.
Thinking about that case more, I think simply returning
nextSibling->GetPrevSibling() as I did might be wrong.  I added a call
to FindFrameForContentSibling to give it a chance to veto it.

Please take a look at GetInsertionPrevSibling and see if the new code
(and comments) makes sense to you.