917386 - Make frame construction handle trees of nsIAnonymousContentCreator::ContentInfo objects, including support for associating CSS pseudo-elements with deeply nested anonymous elements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

For bug 344616 (<input type=number>) I need to create a tree of anonymous content. The structure will be something like:

  <input type=number>
    div      - display:inline-flex wrapper
      input  - text input field
      div    - wrapper for arrows
        div  - up arrow
        div  - down arrow

I need the elements at various depths to have pseudo-elements associated with 
them so that they can be given specific styling in forms.css, and to allow author restyling.

To associate a pseudo-element with a given element I need to give it a special style context. Right now, though, there is no good way to do that when the element is more than one level deep under the anonymous tree since nsCSSFrameConstructor can only handle a flat list of nsIAnonymousContentCreator::ContentInfo objects.

This bug is about making nsCSSFrameConstructor handle a tree of nsIAnonymousContentCreator::ContentInfo objects.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: hacking around

There are so many moving parts and different ways of doing things when it comes to frame construction that I'm finding it very unclear what the best approach is. In fact I've been struggling to find any approach that will work and that doesn't trigger a whole series of assertions. This is my latest attempt at hacking together something that works, but again I'm being thwarted by a flood of assertions which seem to be related to style context parents gone bad (perhaps because in nsNumberControlFrame::CreateAnonymousContent I can't really take account of the contexts between grandchildren anonymous elements and the input element).

Boris, before I sink even more time into this do you have any thoughts on the best approach here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

This generally looks like what I was thinking of.  I think you  want to factor out the duplicated code in ProcessChildren() and AddAnonChildFCItemsRecursively(), and I'm not sure why we need the oldLastItem bits.  Just for the asserts?

The const_cast is clearly bogus; we should store that state in the FrameConstructionItem (and just init from the FrameConstructionData).

I can't tell you more about the asserts you get without seeing your code that creates the style contexts...

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This generally looks like what I was thinking of.  I think you  want to
> factor out the duplicated code in ProcessChildren() and
> AddAnonChildFCItemsRecursively(), and I'm not sure why we need the
> oldLastItem bits.  Just for the asserts?

Yes. That part's just thrown together ATM. I'll clean it up later once this is working.

> The const_cast is clearly bogus; we should store that state in the
> FrameConstructionItem (and just init from the FrameConstructionData).

Sure. It's AddFrameConstructionItemsInternal that creates the FrameConstructionItem objects though. How would you prefer that I tell AddFrameConstructionItemsInternal that it should set this bit? Would adding a new define to the group of defines starting with ITEM_ALLOW_XBL_BASE and passing it to AddFrameConstructionItemsInternal via its aFlags seem reasonable?

> I can't tell you more about the asserts you get without seeing your code
> that creates the style contexts...

That code is in https://bugzilla.mozilla.org/attachment.cgi?id=806681&action=diff - it's the nsNumberControlFrame::MakeAnonymousElement method which just creates a style context using the default nsStyleContext ctor, which is what other MakeAnonymousElement methods seem to do. As I note in XXX comments in my patches I'm not sure quite what I could do to do better than that since the pseudo-element matching parent contexts haven't been created yet.

The assertion I'm currently unclear on is the failure of the MOZ_ASSERT under:

  AncestorFilter::AssertHasAllAncestors
  RuleHash::EnumerateAllRules
  nsCSSRuleProcessor::RulesMatching
  bool EnumRulesMatching<ElementRuleProcessorData>
  nsStyleSet::ResolveStyleFor
  nsCSSFrameConstructor::ResolveStyleContext
  nsCSSFrameConstructor::BuildInlineChildItems
  nsCSSFrameConstructor::AddFrameConstructionItemsInternal
  nsCSSFrameConstructor::AddAnonChildFCItemsRecursively
  nsCSSFrameConstructor::ProcessChildren

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp?mark=3499&ref=d50f590056fd#3495

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Would adding a new define

Yes, exactly.

> The assertion I'm currently unclear on is the failure of the MOZ_ASSERT under:

Ah!  When you go down the tree, you need to put a TreeMatchContext::AutoAncestorPusher on the stack for the element whose kids you're about to look at.  See what the beginning of BuildInlineChildItems does, for example.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Ah!  When you go down the tree, you need to put a
> TreeMatchContext::AutoAncestorPusher on the stack for the element whose kids
> you're about to look at.

Thanks! That helped a lot!

It's really difficult to tell how much else from BuildInlineChildItems and similar functions should be integrated into AddAnonChildFCItemsRecursively. I'm hoping that you have a fairly good idea and can spot anything that's necessary during review.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

This seems to work. There are a few XXXjwatt flags in this patch, but maybe these can be addressed as "r+ if you..." follow-ups (assuming this actually passes review).

Comment 7

[Jonathan Watt [:jwatt]]

For reference, I've updated the WIP patch in bug 635240 (which is the patch that uses on the functionality that is added here).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809538 [details] [diff] [review]
patch

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+ConnectAnonymousTreeDescendants(nsIContent* aParent,

So this is only needed so that the anonymous content provider doesn't have to do it themselves, right?  In principle they could...  Worth documenting that.

Definitely document in nsIAnonymousContentCreator.h that mChildren should not be hooked up to any parents yet and that frame construction will handle it.

>@@ -5214,16 +5239,22 @@ nsCSSFrameConstructor::AddFrameConstruct
>+  // XXXjwatt how best can we avoid this cast? Add an argument to all
>+  // FindXXXData() methods to handle ITEM_USE_CHILD_ITEMS? :(
>+  if (aFlags & ITEM_USE_CHILD_ITEMS) {
>+    const_cast<FrameConstructionData*>(data)->mBits |= FCDATA_USE_CHILD_ITEMS;

"data" is static const data.  It might well be on a readonly page!

The right way to handle this is a patch under this one that does the following:

1)  Add a boolean member to FrameConstructionItem that indicates where to use its
    child items or not.
2)  Change current consumers of "data->mBits & FCDATA_USE_CHILD_ITEMS" to look at this
    member instead.
3)  Pass a value for this member in to the FrameConstructionItem constructor.  Most
    of the callsites would just use "data->mBits & FCDATA_USE_CHILD_ITEMS" as the
    value, but the callsite here would || that with aFlags & ITEM_USE_CHILD_ITEMS.

But see below.

>+nsCSSFrameConstructor::AddAnonChildFCItemsRecursively(

This needs documentation.

>+      AddAnonChildFCItemsRecursively(aState, nullptr, // XXXjwatt pass aFrame?

Hrm.  Right.  This is actually a problem, in theory.  AddFrameConstructionItemsInternal actually uses the parent frame for various stuff, and expects it to be nonnull unless it'll end up being an inline frame.

Here's what it's used for specifically:

* Various SVG things.  I think we should assert if someone has an SVG parent
  element on this codepath.
* Detecting when the parent is a colgroup.  We should assert that its display
  type here is never colgroup or something.  If we can.  If pages can style
  the display prop here, we have a problem.  
* Determining the correct block/inline state for figuring out block-inside-inline
  splits.  This will be a problem.
* Special handling of legends inside fieldsets.

Maybe the right thing to do is to add a new member to FrameConstructionItem that stores a list of nsIAnonymousContentCreator::ContentInfo and add another branch to where we'd pick between using our child items or calling ProcessChildren to instead at _that_ point (where we have a frame) create the FrameConstructionItems for the kids and then construct frames from them.

And we presumably need to teach BuildInlineChildItems about this stuff too, then, since one of our anon items can end up display:inline, in theory. 

I'm sorry I didn't think of this problem earlier.  :(

This should address the issues with AddFrameConstructionItemsInternal adding more than one item too, I think: we'd pass the list of ContentInfo to it and it would handle doing the right thing with them (e.g. giving them to the right FrameConstructionItem).

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

(In reply to Boris Zbarsky [:bz] from comment #8)
> Maybe the right thing to do is to add a new member to FrameConstructionItem
> that stores a list of nsIAnonymousContentCreator::ContentInfo and add
> another branch to where we'd pick between using our child items or calling
> ProcessChildren to instead at _that_ point (where we have a frame) create
> the FrameConstructionItems for the kids and then construct frames from them.

That's the approach that I've taken in this patch, and this time I haven't ended up in assertion failure hell.

> And we presumably need to teach BuildInlineChildItems about this stuff too,
> then, since one of our anon items can end up display:inline, in theory. 

I'm still trying to figure out what I should be doing there, but perhaps you can sanity check this patch in the meantime?

> I'm sorry I didn't think of this problem earlier.  :(

No problem, this stuff is a bit tricky.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809871 [details] [diff] [review]
patch

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -3585,17 +3598,34 @@ nsCSSFrameConstructor::ConstructFrameFro
>+    if (!aItem.mAnonChildren.IsEmpty()) {

I think you could avoid some code duplication here by doing:

  AddFCItemsForAnonContent(aState, newFrame, aItem.mAnonChildren, aItem.mChildItems);
  bits |= FCDATA_USE_CHILD_ITEMS;

and then just taking the normal FCDATA_USE_CHILD_ITEMS codepath.

>@@ -5112,16 +5181,17 @@ nsCSSFrameConstructor::AddFrameConstruct
>+    // XXXjwatt Do anything for aAnonChildren in SetAsUndisplayedContent?

Don't think so.  They just get no frames.

>+nsCSSFrameConstructor::AddFCItemsForAnonContent(
>-      flexItemStyleFixupSkipper(aState.mTreeMatchContext);
>+    flexItemStyleFixupSkipper(aState.mTreeMatchContext);

The new indent is wrong.

So you need to change nsCSSFrameConstructor::BuildInlineChildItems too.  In particular, in that function where it currently walks the kids you want to check for nonempty mAnonChildren and if so use those instead to construct mChildItems.  Note that you'll need to pass some slightly different flags in this case, so your AddFCItemsForAnonContent method might need a flags argument.

>+++ b/layout/base/nsCSSFrameConstructor.h
>     FrameConstructionItem(const FrameConstructionData* aFCData,
>+        mAnonChildren = *aAnonChildren;

This will do a deep-copy on the whole tree of stuff there.  Better to SwapElements(), I think.

This is looking pretty good, modulo the above nits, of which the one about BuildInlineChildItems is the most important.  I'd love to review just an interdiff addressing those, not an entirely new patch.  ;)

This new setup will fail for anything that uses a full-constructor fcdata which is not calling ConstructInline(). I'm not sure whether you've added asserts to all of those; maybe the right place to assert is in the ctor for FrameConstructionItem: assert that the fcdata is not a full-ctor one or the func is ConstructInline?

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: interdiff

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> The new indent is wrong.

(I already spotted that and fixed it before your comments, which is why this change isn't in the interdiff but is in the patch.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

The new patch doesn't seem to reflect the interdiff?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 812749 [details] [diff] [review]
patch

r=me based on the interdiff, but please do post the full patch that includes the changes from that interdiff (which this one does not).

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: patch

Sorry, wrong file.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 812757 [details] [diff] [review]
patch

r=me

Comment 18

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eca799dbebc

>+    NS_ABORT_IF_FALSE(content->HasFlag(NODE_IS_IN_ANONYMOUS_SUBTREE),
>+                      "Content should know it's anonymous");

I had to remove this new assertion since it doesn't hold for SVGUseElement's cloned content.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7eca799dbebc