Implement display:contents

RESOLVED FIXED in mozilla36

Status

()

enhancement
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(25 attachments, 53 obsolete attachments)

1.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
438 bytes, text/html
Details
225 bytes, text/html
Details
2.19 KB, patch
mats
: review+
Details | Diff | Splinter Review
621 bytes, text/html
Details
4.93 KB, text/plain
Details
507 bytes, text/html
Details
7.72 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.32 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.17 KB, patch
mats
: review+
Details | Diff | Splinter Review
18.01 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.14 KB, patch
mats
: review+
Details | Diff | Splinter Review
9.17 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
59.00 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
58.35 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
56.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.75 KB, text/plain
Details
29.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
104.82 KB, patch
Details | Diff | Splinter Review
4.59 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
343.78 KB, patch
Details | Diff | Splinter Review
344.54 KB, patch
Details | Diff | Splinter Review
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.
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?
Flags: needinfo?(dbaron)
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.
Blocks: 874238
Yes, I believe that's correct.
(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)?
Flags: needinfo?(dbaron)
Depends on: 914432
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
Comment on attachment 802681 [details] [diff] [review]
Style system support for display:contents.

(requires bug 914432)
Attachment #802681 - Flags: review?(dbaron)
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.
Attachment #802684 - Flags: review?(dholbert)
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.
Attachment #802685 - Flags: review?(dholbert)
Attachment #802686 - Attachment is obsolete: true
Attachment #805966 - Flags: review?(bzbarsky)
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.
Attachment #802687 - Attachment is obsolete: true
Attachment #805978 - Flags: review?(bzbarsky)
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.
Attachment #802688 - Attachment is obsolete: true
Attachment #805980 - Flags: review?(bzbarsky)
Attachment #802689 - Flags: review?(bzbarsky)
Attachment #802692 - Flags: review?(bzbarsky)
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.)
Attachment #802694 - Flags: review?(bzbarsky)
Attachment #802694 - Flags: feedback?(dbaron)
Attachment #802695 - Flags: review?(bzbarsky)
Posted patch reftests (obsolete) — Splinter Review
Blocks: 917313
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.)
Attachment #802685 - Flags: review?(dholbert) → review+
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)
Attachment #802684 - Flags: review?(dholbert) → feedback+
Lovely, that works exactly as I want it, thanks!
Attachment #802684 - Attachment is obsolete: true
Attachment #806173 - Flags: review?(dholbert)
Posted patch reftests (obsolete) — Splinter Review
Tweaked the display-contents-acid-ref.html to match correct flexbox rendering.
Attachment #806002 - Attachment is obsolete: true
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.
("this comment" = the code-comment, not the commit message -- sorry for any ambiguity on that)
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?
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.
(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.
(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.
(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].)
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.)
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.
Attachment #806173 - Flags: review?(dholbert) → review+
(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.
Mats, sorry this is taking so long.  I'm planning to look at these patches on Monday.
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.
Attachment #802689 - Flags: review?(bzbarsky) → review+
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?
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?
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
Attachment #802695 - Flags: review?(bzbarsky) → review+
(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.
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?
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?
Attachment #802694 - Flags: review?(bzbarsky) → review+
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?
Attachment #802692 - Flags: review?(cam)
Attachment #802692 - Flags: review?(bzbarsky)
Attachment #802692 - Flags: review+
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?
Attachment #805966 - Flags: review?(bzbarsky) → review-
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....
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.
Attachment #805978 - Flags: review?(bzbarsky) → review-
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.
Attachment #805980 - Flags: review?(bzbarsky) → review-
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
Attachment #805982 - Flags: review?(bzbarsky) → review+
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.
Attachment #802692 - Flags: review?(cam)
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-.
Attachment #802692 - Flags: feedback-
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
Attachment #802681 - Flags: review?(dbaron) → review+
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?
Attachment #802694 - Flags: feedback?(dbaron) → feedback+
This is now known as 'display-box: contents'; see http://dev.w3.org/csswg/css-display/#the-display-box .
Summary: Implement display:contents → Implement display-box:contents
Blocks: 979782
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.
Attachment #802681 - Attachment is obsolete: true
Comment on attachment 802685 [details] [diff] [review]
Put display:contents support behind a pref - disabled by default.

... and this too.
Attachment #802685 - Attachment is obsolete: true
(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)
Attachment #802689 - Attachment is obsolete: true
Attachment #8392917 - Flags: review+
(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.
Attachment #802694 - Attachment is obsolete: true
Attachment #8392920 - Flags: review+
Attachment #8392917 - Attachment description: 907396-display-contents-RestyleDisplayNoneOrContentsChildren → Implement RestyleUndisplayedDescendants that restyles diplay:none children and display:contents descendants.
(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.
Attachment #805966 - Attachment is obsolete: true
Attachment #8392929 - Flags: review?(bzbarsky)
>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.
Attachment #805978 - Attachment is obsolete: true
Attachment #8392932 - Flags: review?(bzbarsky)
(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.
Attachment #805982 - Attachment is obsolete: true
Attachment #8392934 - Flags: review+
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.
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.
Attachment #805980 - Flags: review- → review?(bzbarsky)
Attachment #806177 - Attachment is obsolete: true
(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.
Attachment #802692 - Attachment is obsolete: true
Attachment #8392953 - Flags: review?(cam)
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.
Comment on attachment 806174 [details] [diff] [review]
reftests

I've moved all the tests into bug 917313.
Attachment #806174 - Attachment is obsolete: true
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.
Attachment #805980 - Flags: review?(bzbarsky) → review+
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?
Flags: needinfo?(matspal)
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.
Attachment #8392929 - Flags: review?(bzbarsky) → review-
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.)
Attachment #8392932 - Flags: review?(bzbarsky) → review-
> 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.
(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).
Attachment #805980 - Attachment is obsolete: true
Attachment #8442107 - Flags: review+
Flags: needinfo?(mats)
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?
Attachment #8442107 - Flags: review+ → review?(bzbarsky)
(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?
(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).
Attachment #8442117 - Flags: review?(bzbarsky)
> 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.
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.
Attachment #8442107 - Flags: review?(bzbarsky)
> 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.
(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?
Attachment #8392929 - Attachment is obsolete: true
Attachment #8442176 - Flags: review?(bzbarsky)
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.
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
Attachment #8442117 - Flags: review?(bzbarsky) → review+
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.
Attachment #8442176 - Flags: review?(bzbarsky) → review-
(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.
Attachment #8442117 - Attachment is obsolete: true
Attachment #8442192 - Flags: review+
(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.
How did those tests pass with this unbind bit, then?
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?
Flags: needinfo?(wchen)
BTW, there's a proposal from fantasai/Tab to change 'display-box':
http://lists.w3.org/Archives/Public/www-style/2014Jul/0023.html
(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
Aha, thank you.  That makes sense.
(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.
(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);
Attachment #8442107 - Attachment is obsolete: true
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.
Attachment #8442176 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #91)
> Because "boxContents" always ends up false.

No, I believe you are mistaken.
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...
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.
Flags: needinfo?(wchen)
Depends on: 1038294
(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.
(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.
Attachment #8392954 - Attachment is obsolete: true
Posted patch Rollup patch (wdiff) (obsolete) — Splinter Review
> 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.  ;)
(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.
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.
(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?
(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.
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
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)
No longer depends on: 1038294
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.  :(
(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.
Here's an up-to-date version of that patch.
Attachment #8392953 - Attachment is obsolete: true
Attachment #8392953 - Flags: review?(cam)
Attachment #8457452 - Flags: review?(cam)
Depends on: 1040283
Depends on: 1040291
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))
Attachment #8492588 - Flags: review?(cam)
No significant changes except for the removal of the
NS_STYLE_DISPLAY_CONTENTS bit pointed out in comment 33.
Attachment #806173 - Attachment is obsolete: true
Attachment #8492589 - Flags: review+
No significant changes since last time... except for the changes
to handle XBL/Shadow root insertion points in the upcoming patch.
Attachment #8455290 - Attachment is obsolete: true
Attachment #8492591 - Flags: review?(bzbarsky)
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()".
Attachment #8408496 - Attachment is obsolete: true
Attachment #8455386 - Attachment is obsolete: true
Attachment #8455502 - Attachment is obsolete: true
Attachment #8455503 - Attachment is obsolete: true
Attachment #8492593 - Flags: review?(bzbarsky)
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.)
Merged in the code sharing patch.  No significant changes otherwise.
Attachment #8492595 - Flags: review+
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.
Attachment #8392934 - Attachment is obsolete: true
Attachment #8442192 - Attachment is obsolete: true
Attachment #8492598 - Flags: review+
Attachment #8492598 - Flags: feedback?(bzbarsky)
Posted patch Rollup patch. (obsolete) — Splinter Review
Posted patch Rollup patch (wdiff). (obsolete) — Splinter Review
Summary: Implement display-box:contents → Implement display:contents
Blocks: 1070507
Attachment #8492598 - Flags: feedback?(bzbarsky) → feedback+
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
Attachment #8492596 - Flags: review?(bzbarsky) → review+
Attachment #8492593 - Attachment is obsolete: true
Attachment #8492593 - Flags: review?(bzbarsky)
Attachment #8493408 - Flags: review?(bzbarsky)
With only part 1, all reftests pass, except the XBL/Shadow DOM tests which fails.
Attachment #8493412 - Flags: review?(bzbarsky)
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
Attachment #8492591 - Attachment is obsolete: true
Attachment #8493408 - Attachment is obsolete: true
Attachment #8493412 - Attachment is obsolete: true
Attachment #8492591 - Flags: review?(bzbarsky)
Attachment #8493408 - Flags: review?(bzbarsky)
Attachment #8493412 - Flags: review?(bzbarsky)
Attachment #8493953 - Flags: review?(bzbarsky)
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.
Attachment #8493956 - Flags: review?(bzbarsky)
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/)
Yep, thank you.  And thank you for the try link; that will let me fold patches as desired and so forth.
Posted file patches.zip (obsolete) —
Here's a zip archive with the current patches.  "my-applied-series" has the order.
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.
Attachment #8493953 - Flags: review?(bzbarsky) → review+
> Please document the relationship between aParentFrame and aContainer.

And how aContainer differs from aChild->GetParent(), for that matter.
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!
Attachment #8493956 - Flags: review?(bzbarsky) → review+
Attachment #8493957 - Flags: review?(bzbarsky) → review+
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...
Attachment #8492588 - Flags: review?(cam) → review+
Actually, what is the plan for landing here given that the property has changed (again) to be box-suppress:discard?
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?
Depends on: 1075247
(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)
(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.
Attachment #8492600 - Attachment is obsolete: true
Attachment #8492600 - Flags: review?(cam)
Attachment #8498158 - Flags: review?(cam)
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.
Attachment #8498173 - Flags: review?(bzbarsky)
(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.
Attachment #8492594 - Attachment is obsolete: true
Attachment #8492602 - Attachment is obsolete: true
Attachment #8492603 - Attachment is obsolete: true
Attachment #8493956 - Attachment is obsolete: true
Attachment #8498181 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #144)
> 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.

aContainer isn't used in this patch, it's just a preparation for changes
in later patches, so I'll document it in a later patch instead.

> >+  parentFrame = insertion.mParentFrame;
> 
> So it looks like we modify parentFrame a few times, then have to go and

Fixed.  I removed the first instance of parentFrame and then made it
a reference to insertion.mParentFrame at the later point.

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

Fixed.

> Please fix the indent on the second line there.

Fixed.

> 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.

Fixed.  I removed it as suggested.

> >     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.

mParentFrame==null signals the consumer shouldn't try to create frames
so mContainer shouldn't be used.  I've documented this on InsertionPoint.

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

Fixed... although "The container for the inserted children." is kind of lame.
As I said, I'll come back to this...
Attachment #8493953 - Attachment is obsolete: true
Attachment #8493977 - Attachment is obsolete: true
Attachment #8498189 - Flags: review+
Posted patch Tests for display:contents (obsolete) — Splinter Review
Added more tests...
Attachment #8492601 - Attachment is obsolete: true
Posted patch interdiff, fctor part 2. (obsolete) — Splinter Review
BTW, here's the interdiff of the significant changes to "part 2 of the Fctor changes" mentioned in comment 153, in case you want to review this bit only.
> although "The container for the inserted children." is kind of lame

It's not just lame, it's useless.

Please document what this thing actually is.  Is it the element the insertion point lives under (so the parent of the kids in the flattened tree)?  Something else?
Comment on attachment 8498173 [details] [diff] [review]
bug fix for FindPrevious[Next]Sibling

Please document what aIter should be and how it's related (or not) to aChild.  Also what aParentFrame should be.

r=me
Attachment #8498173 - Flags: review?(bzbarsky) → review+
Comment on attachment 8498181 [details] [diff] [review]
part 2 of the Fctor changes - the main part, except XBL/Shadow tree fixes

>+AutoDisplayContentsAncestorPusher::AutoDisplayContentsAncestorPusher(

So now that I look at this more carefully, this seems pretty broken: It pushes and pops in the same order.  Shouldn't those be opposite orders?

Specifically, you want to push from the root down, but pop from the leaf up.

Please add testcases that would have caught this bug!

The mPushedAncestors boolean doesn't seem to be needed; it's just mTreeMatchContext.mAncestorFilter.HasFilter() which you can get in the dtor, no?

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

Well, :scope matching in scoped stylesheets (or perhaps scoped stylesheet matching in general), in the !HasFilter() case, right?  Cameron might know for sure what the consequences of not calling PushStyleScope() when it should be called are.

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

I think if you're going to mess with this code you really need to learn either enough XBL or enough shadow DOM or ideally both to write testcases like this... otherwise, how did you test your code so far?

  <div><i>Two</i><b>One</b></div>
  <script>
    var root = document.querySelector("div").createShadowRoot();
    root.innerHTML = '<content select="b"></content><content select="i"></content>';
  </script>
   
and various variants thereon (e.g. some of those kids could be display:contents, the <div> could be display:contents, the <content> elements could be kids of display:contents children of the shadowroot like this:

    root.innerHTML = '<div style="display: contents"><content select="b"></content><content select="i"></content></div>';

Or there could be stuff (possibly display:contents stuff) between the two <content> elements, and so forth.  Basically, try to think of all the evil edge cases that would exercise the various conditionals in the frame constructor code...

XBL example, after setting the "dom.allow_XUL_XBL_for_file" boolean pref to true and maybe restarting:

  <div style="-moz-binding: url(test.xml#foo)"><i>Two</i><b>One</b></div>

where test.xml is:

  <bindings xmlns="http://www.mozilla.org/xbl"
            xmlns:html="http://www.w3.org/1999/xhtml">
    <binding id="foo">
      <content><children includes="b"/><children includes="i"/></content>
    </binding>
  </bindings>

I don't follow the FindFrameForContentSibling business in GetInsertionPrevSibling. In particular, the fact that it never passes aChild anywhere is pretty fishy.  r- on that part, pending a much clearer explanation of why you think it's needed and why you think it's correct.
Attachment #8498181 - Flags: review?(bzbarsky) → review-
Comment on attachment 8498158 [details] [diff] [review]
Make RestyleManager::RestyleElement and nsCSSFrameConstructor::MaybeRecreateFramesForElement deal with display:contents elements.

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

OK, the reorganisation of the methods works for me.

::: layout/base/RestyleManager.cpp
@@ +3283,5 @@
> +  nsChangeHint    aMinHint,
> +  RestyleTracker& aRestyleTracker,
> +  nsRestyleHint   aRestyleHint)
> +{
> +  MOZ_ASSERT(!(mHintsHandled & nsChangeHint_ReconstructFrame), "why call me?");

It's not obvious to me why this is always true, but if it is, then I'm happy for the if statement checks later in the function to not bother checking it.
Attachment #8498158 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #160)
> > +  MOZ_ASSERT(!(mHintsHandled & nsChangeHint_ReconstructFrame), "why call me?");
> 
> It's not obvious to me why this is always true, but if it is, then I'm happy
> for the if statement checks later in the function to not bother checking it.

I'm following the existing pattern in ElementRestyler::RestyleChildren:
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp?rev=41ce29f771b2#3205
MaybeReframeForBeforePseudo, MaybeReframeForAfterPseudo can set
nsChangeHint_ReconstructFrame in mHintsHandled, so that's what checks
are for, AFAICT.

RestyleUndisplayedChildren is a NOP if it's set:
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp?rev=41ce29f771b2#3288
which means RestyleChildren is too, so it's pointless to call it
if we've already decided to reframe.

It's green on Try, so I'd rather leave the assertion in since it says
something about the code.  I can take it out if you think calling
RestyleChildren after we have already decided to reframe should be allowed.

I'm setting a need-info? so you can reply later when you get back.
We can adjust this minor detail post-landing if needed.
Flags: needinfo?(cam)
I have a problem with how Element::CreateShadowRoot works:
http://hg.mozilla.org/mozilla-central/annotate/f0bb13ef0ee4/content/base/src/Element.cpp#l801

It calls RecreateFramesFor, which calls nsCSSFrameConstructor::ContentRemoved,
where I use FlattenedChildIterator to get the child nodes:
https://bugzilla.mozilla.org/attachment.cgi?id=8498181&action=diff#a/layout/base/nsCSSFrameConstructor.cpp_sec23
But since it has already setup a binding, the iterator doesn't give me
the nodes I want, so the frames for them isn't destroyed.

I guess I could work around this by inventing a new iterator that
gives me the child nodes I want, but CreateShadowRoot seems wrong.
Not just the problem described above, but also the chunk that calls
UnbindFromTree in the older shadow.  This seems wrong:
842           child->UnbindFromTree(true, false);
(gdb) p child->GetPrimaryFrame()
$2 = (nsIFrame *) 0x7fffb5cb7470

I tend to think CreateShadowRoot should do the ContentRemoved part
near the top, so that the frames are destroyed before messing with
UnbindFromTree/SetXBLBinding etc, and then do the ContentInserted
part at the end (if we pretend RecreateFramesFor consists of those
two parts for a moment).

Thoughts?
Flags: needinfo?(wchen)
Flags: needinfo?(bzbarsky)
We could do that, but won't you have the same problem with XBL?  See nsXBLBindingRequst::DocumentLoaded, which does a RecreateFramesFor after attaching the binding.  Do we need a similar fix there?
Flags: needinfo?(bzbarsky)
(In reply to Mats Palmgren (:mats) from comment #162)
> I tend to think CreateShadowRoot should do the ContentRemoved part
> near the top, so that the frames are destroyed before messing with
> UnbindFromTree/SetXBLBinding etc, and then do the ContentInserted
> part at the end (if we pretend RecreateFramesFor consists of those
> two parts for a moment).
> 
> Thoughts?
I'm guessing you're running into this issues when creating another shadow root on an element that already has a shadow root. I think you're right, before we set the binding for the new shadow root, we probably need to call ContentRemoved or something similar to remove the frames of the old shadow root because it's leaving the composed document. This may also be a problem in XBL too because it doesn't explicitly call into anything that will clean up frames when a binding is uninstalled but I'm not sure it's actually an issue because every way that I know to uninstall a binding in XBL will trigger something else that removes the frames (although I'm not sure how reliably it does so). When a binding is changed via a DOM mutation (such as removing the bound element from the document), the PresShell calls into nsCSSFrameConstructor::ContentRemoved which eventually removes the frames. When the -moz-binding style rule is changed, the RestyleManager calls into nsCSSFrameConstructor::ContentRemoved as well.
Flags: needinfo?(wchen)
I've run into a crash ('shadow' is null) at line 6843 here:
http://hg.mozilla.org/mozilla-central/annotate/01f04d75519d/content/base/src/nsContentUtils.cpp#l6832
If 'shadow' is non-null we would have already returned on line 6836.
What is the code meant to do here?
Flags: needinfo?(wchen)
(In reply to Mats Palmgren (:mats) from comment #165)
> I've run into a crash ('shadow' is null) at line 6843 here:
> http://hg.mozilla.org/mozilla-central/annotate/01f04d75519d/content/base/src/
> nsContentUtils.cpp#l6832
> If 'shadow' is non-null we would have already returned on line 6836.
> What is the code meant to do here?

The code determines if the shadow DOM distribution algorithm [1] would result in a child of |aContent| being distributed to a insertion point. This just looks like a bug. Line 6843 should be:

return shadowEl->GetOlderShadowRoot();
Flags: needinfo?(wchen)
(In reply to William Chen [:wchen] from comment #166)
> return shadowEl->GetOlderShadowRoot();

That fixed the crash for me.  I filed bug 1083814 about it.
Depends on: 1083814
Thank you both for your feedback.  I have filed bug 1083855 to fix
CreateShadowRoot and DocumentLoaded so that they destroy the frames
before messing with the binding.
Depends on: 1083855
This adds a couple of assertions, and a display:contents check in
nsXBLBindingRequest::DocumentLoaded to avoid creating frames in that case too,
like it does in the non-null frame / display:none cases.
Attachment #8507588 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #159)
> Comment on attachment 8498181 [details] [diff] [review]
> part 2 of the Fctor changes - the main part, except XBL/Shadow tree fixes
> 
> >+AutoDisplayContentsAncestorPusher::AutoDisplayContentsAncestorPusher(
> 
> So now that I look at this more carefully, this seems pretty broken: It
> pushes and pops in the same order.  Shouldn't those be opposite orders?

Good catch, fixed.

> The mPushedAncestors boolean doesn't seem to be needed

OK, I've removed mPushedAncestors.
(I didn't know HasFilter() was invariant over the life time.)

> I don't follow the FindFrameForContentSibling business in
> GetInsertionPrevSibling.

I've removed that bit now; I found that we need a more general
solution: GetInsertionPrevSibling passes an iterator over aChild's
siblings to FindPrevious/NextSibling.  The intent is to find a frame
on one of them and use as the prev-sibling in the frame tree.
Currently the code just skips frame-less content siblings since its
descendants can't have frames (display:none) but a display:contents
node can have descendants with frames, so we need to recurse on
children for display:contents siblings.
Attachment #8498181 - Attachment is obsolete: true
Attachment #8509088 - Flags: review?(bzbarsky)
I've trimmed this slightly to only show the significant parts.
Attachment #8493349 - Attachment is obsolete: true
Attachment #8498197 - Attachment is obsolete: true
A one-line bug fix since the last version (that has r+), interdiff:

< +    nsIContent* parentContent = mContent->GetParent();
---
> +    nsIContent* parentContent = mContent->GetFlattenedTreeParent();
Attachment #8492598 - Attachment is obsolete: true
Attachment #8509095 - Flags: review?(bzbarsky)
Posted patch Tests for display:contents (obsolete) — Splinter Review
Added more tests.  Primarily in the areas of XBL, Shadow DOM, style scopes.
https://tbpl.mozilla.org/?tree=Try&rev=95d6ce9de9f2
Attachment #8498191 - Attachment is obsolete: true
Comment on attachment 8509095 [details] [diff] [review]
Replace GetParentStyleContextFrame with GetParentStyleContext which can return frame-less display:contents style contexts.

r=me based on the interdiff, I think.  Please double-check with wchen that inheritance is in fact supposed to happen into the shadow tree.
Attachment #8509095 - Flags: review?(bzbarsky) → review+
Comment on attachment 8507588 [details] [diff] [review]
display:contents specific additions on top of bug 1083855

>+#if DEBUG

Please make this unconditional.  Otherwise it's too easy to write code that only compiles in a debug build.

r=me with that
Attachment #8507588 - Flags: review?(bzbarsky) → review+
Comment on attachment 8509088 [details] [diff] [review]
Frame constructor changes for display:contents.

Yes, this makes much more sense.

>+++ b/layout/base/nsCSSFrameConstructor.h
>+   * @param aParentFrame the nearest ancestor frame, used inernally for

"internally"

r=me
Attachment #8509088 - Flags: review?(bzbarsky) → review+
Oh, and thank you for the interdiff!
(In reply to Mats Palmgren (:mats) from comment #172)
> Created attachment 8509095 [details] [diff] [review]
> Replace GetParentStyleContextFrame with GetParentStyleContext which can
> return frame-less display:contents style contexts.
> 
> A one-line bug fix since the last version (that has r+), interdiff:
> 
> < +    nsIContent* parentContent = mContent->GetParent();
> ---
> > +    nsIContent* parentContent = mContent->GetFlattenedTreeParent();


(In reply to Boris Zbarsky [:bz] from comment #174)
> r=me based on the interdiff, I think.  Please double-check with wchen that
> inheritance is in fact supposed to happen into the shadow tree.

This is the relevant spec text I think:
http://dev.w3.org/csswg/css-scoping/#inheritance

We implement that correctly (without display:contents) AFAICT and inheritance
works the same with display:contents, so I think we're good.

William?
Flags: needinfo?(wchen)
(In reply to Mats Palmgren (:mats) from comment #178)
> (In reply to Boris Zbarsky [:bz] from comment #174)
> > r=me based on the interdiff, I think.  Please double-check with wchen that
> > inheritance is in fact supposed to happen into the shadow tree.
> 
> This is the relevant spec text I think:
> http://dev.w3.org/csswg/css-scoping/#inheritance
> 
> We implement that correctly (without display:contents) AFAICT and inheritance
> works the same with display:contents, so I think we're good.
> 
> William?

Yes, that's the relevant spec text.
Flags: needinfo?(wchen)
Depends on: 1096635
Added more XBL tests.
Attachment #8509098 - Attachment is obsolete: true
This patch fulfills the remaining requests in your review comments above:
1. add useful documentation for mContainer
2. assert "aParentFrame->GetContent() == aContainer" in ResolveStyleContext()
   (when aParentFrame is non-null)

It turns out that the underlying issue that prevented that is a bug in
GetFlattenedTreeParent() in some XBL cases.  I filed that separately:
bug 1096635.  (This patch depends on that being fixed reasonably.)

(there's no commit message in this patch since I plan to fold it)

https://tbpl.mozilla.org/?tree=Try&rev=7993d699021a
Attachment #8521869 - Flags: review?(bzbarsky)
Posted patch rollup patchSplinter Review
(I filed bug 1098072 on writing <style scoped> tests that would fail with
the wrong push/pop order in AutoDisplayContentsAncestorPusher.)
Comment on attachment 8521869 [details] [diff] [review]
Some tweaks for XBL trees, adding assertions / documentation for mContainer

>+        insertion.mContainer = GetInsertionPoint(parent, child).mContainer;
>+        MOZ_ASSERT(insertion.mContainer == child->GetFlattenedTreeParent());

Why can't we just use GetFlattenedTreeParent() here then?  Is GetInsertionPoint() cheaper?

If not, can we reverse the sense of the assert and use the cheaper function?

r=me.
Attachment #8521869 - Flags: review?(bzbarsky) → review+
> Why can't we just use GetFlattenedTreeParent() here then?  Is
> GetInsertionPoint() cheaper?

Yes, a micro benchmark shows that GetFlattenedTreeParent() is ~4 times faster.

> If not, can we reverse the sense of the assert and use the cheaper function?

Did so.
Blocks: 1102374
Blocks: 1105369
I've documented as a new value of display (display as a shorthand + display-outside will be documented as part of the relevant bug). 

https://developer.mozilla.org/en-US/docs/Web/CSS/display
and
https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS

I've added a ddn on the bugs about activating this feature.
Depends on: 1140579
Depends on: 1144434
Depends on: 1251799
Depends on: 1351588
No longer depends on: 1351588
Depends on: 1381134
Flags: needinfo?(cam)
Depends on: 1265104
You need to log in before you can comment on or make changes to this bug.