269023 - XTF insertion points don't work when all ancestor frames are inline

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

Consider an XTF element with a single <span> that should be used as the
insertionPoint.  In this case, explicit children don't get their frames
constructed at all.  Here's why... we construct nsIAnonymousContentCreator
content like this:

1. construct parent frame
2. construct anonymous children
3. ask for insertion point frame
4. construct explicit children with insertion point frame as parent

To get the insertion point frame, we ask the nsIXTFElement for a content node,
and use nsIPresShell::GetPrimaryFrameFor().  The reason this doesn't work for
this example is that we skip adding inline frames to the frame map, as a
footprint optimization.  GetPrimaryFrameFor() tries to walk up to parent frames,
but they aren't in the primary frame map yet either because they're still being
constructed.  So we fail to find any primary frame for the supplied insertion
point node, and bail on creating frames for the explicit children.

dbaron suggested a different approach.  After constructing the XTF frame, we
would do this:

1. Ask for the insertion point as a content node, and stash it in the
FrameConstructorState.
2. Start constructing the frames for the anonymous content.
3. When (if) we reach the anonymous content that's given as the insertion point
node, we construct its frame and then we process the explicit children of the
xtf frame.  No need to do a frame map lookup since we have the frame right there.

Comment 1

[Boris Zbarsky [:bzbarsky]]

That could probably also solve the problem we have now where if the insertion
point is inline and its kids are blocks we don't do an {ib} split...

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

Something like this.  I elected not to mess with the way
nsIAnonymousContentCreator works for non-XTF elements since those can have an
insertion frame that does not correspond to a content node.

Comment 3

[Brian Ryner (not reading)]

Attachment: diff -w of the patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days at best to get to this... I'm very behind on reviews.  :(

Comment 5

[Brian Ryner (not reading)]

Attachment: patch v2

Fixed uninitialized members in nsFrameConstructorState.

Comment 6

[Brian Ryner (not reading)]

Attachment: diff -w of the patch

Comment 7

[Brian Ryner (not reading)]

Attachment: patch v3

We need to use the newly-constructed frame's ContentInsertionFrame as the
parent frame for the explicit children.

Comment 8

[Brian Ryner (not reading)]

Attachment: diff -w of the patch

Comment 9

[Brian Ryner (not reading)]

Attachment: patch v4

Ok, one more set of changes.  The previous change to use
GetContentInsertionFrame needed to be applied to the XUL, SVG, and
ConstructFrameByDisplayType cases.  Also, I realized that if an XTF element was
the insertion node for another XTF element, we wouldn't construct the anonymous
content of the outer element.  Fixed that as well.

Comment 10

[Brian Ryner (not reading)]

Attachment: patch v4 + missing files

Oops, forgot a few files.

Comment 11

[Brian Ryner (not reading)]

Attachment: diff -w of the patch

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 166355 [details] [diff] [review]
patch v4

hoping dbaron can help out here, this is pretty critical for xforms label to
work correctly

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 166522 [details] [diff] [review]
diff -w of the patch

>Index: layout/base/public/nsIFrame.h

>   /**
>+   * Get the child content node whose frame should be used as the parent for
>+   * the frames of child elements.  This is used only during initial frame
>+   * construction, when the node may not have a frame yet.
>+   */
>+  virtual already_AddRefed<nsIContent> GetContentInsertionNode() { return nsnull; }

This should document what returning |nsnull| means.  It's also not clear to me
what happens when we're not in "initial frame construction" or when the content
insertion node is display:none.

In other words, if I'm implementing a frame class, I have no idea whether I
need to override this function and if so, how.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>+class nsFrameConstructorInsertionState {

>+  nsIFrame*   mAnonymousCreator;

So this is a frame implementing nsIAnonymousContentCreator?  Or what?  Can I
stick random frames in here?

>+  nsIContent* mInsertionContent;

Document how that's related to mAnonymousCreator

>+  PRBool      mCreatorIsBlock;

What does "block" mean in this case?  "display: block", or block-level, and if
the latter, in which exact sense (we have two or three different meanings of it
around).

>+  // Insertion point for nsIAnonymousContentCreator.
>+  nsIFrame                 *mAnonymousCreator;
>+  nsIContent               *mInsertionContent;
>+  PRBool                    mCreatorIsBlock;

Similar documentation here, or just point one to the other... (I guess point
the InsertionState to the state).

>+  // Push an nsIAnonymousContentCreator and its insertion node
>+  void PushAnonymousContentCreator(nsIFrame *aCreator,

This should be more clearly documented.  When should this be pushed?  When
should callers ensure it's popped?  Are there times when pushing the same thing
as was already pushed will screw up?  What happens if null is pushed?  What if
a null frame and a non-null content are pushed?  Vice versa?  Add asserts for
illegal combinations?

>+nsFrameConstructorInsertionState::~nsFrameConstructorInsertionState()

This needs to null-check mState.  Otherwise simply declaring a variable like
this on the stack and never pushing into it will crash.

>-    CreateAnonymousFrames(aPresShell, aPresContext, nsnull, aState, aContent, aNewInnerFrame,
>-                          PR_FALSE, childItems);
>+    {
>+      nsFrameConstructorInsertionState saveState;
>+      aState.PushAnonymousContentCreator(nsnull, nsnull, PR_FALSE, saveState);
>+
>+      CreateAnonymousFrames(aPresShell, aPresContext, nsnull, aState, aContent, aNewInnerFrame,

Document somewhere that all calls to CreateAnonymousFrames must be preceded by
an appropriate PushAnonymousContentCreator call?  Or are there
CreateAnonymousFrames calls that don't need such a push?

If all PushAnonymousContentCreator calls _are_ associated with
CreateAnonymousFrames, and all CreateAnonymousFrames calls need to push,
perhaps the push should happen inside CreateAnonymousFrames (and the signature
of that method should change accordingly)?

>@@ -5277,6 +5376,27 @@ nsCSSFrameConstructor::ConstructHTMLFram

>+  if (newFrame && aState.mInsertionContent == aContent) {

It looks like this block of code is being copied in every single frame
construction function. That's very fragile; such copy-paste is part of what
makes the frame constructor so hard to work with.

Case in point is that you don't seem to have changed ConstructMathMLFrame
accordingly...

Is there any way to move this into the frame constructor state?  If you don't
see one, please file a followup bug on this; it'll really need to happen as we
try to make the frame construction code understandable.  But I'd really prefer
it if the code were just factored out appropriately before landing...  At the
very least, perhaps an appropriate function called from all relevant spots?

>+    rv = ProcessChildren(aPresShell, aPresContext, aState, creatorContent,
>+                         insertionFrame, PR_TRUE, insertionItems,
>+                         aState.mCreatorIsBlock);

Why are we passing aCanHaveGeneratedContent == PR_TRUE here?  Did we never
process the children of creatorContent before?	If so, where is that enforced? 
In ConstructXTFFrame?

It looks to me like most nsIAnonymousContentCreators don't make use of this
whole codepath; the naming makes it seem like they do, though.	

Also, this is clobbering an earlier set of rv, it looks like... If we're doing
error propagation, we should probably actually do it.

Similar comments apply to the other places this is copy-pasted, if it doesn't
get refactored.

>+      // Note: we don't worry about insertionFrame here because we know
>+      // that XTF elements always insert into the primary frame of their
>+      // insertion content.

Can this be asserted (with an NS_ASSERTION) somehow?

Comment 14

[Brian Ryner (not reading)]

> >+  PRBool      mCreatorIsBlock;
> 
> What does "block" mean in this case?  "display: block", or block-level, and if
> the latter, in which exact sense (we have two or three different meanings of it
> around).
> 

Whatever it means when passed to ProcessChildren (aParentIsBlock).

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK.  Document that, please?

Comment 16

[Brian Ryner (not reading)]

(In reply to comment #13)
> >+    rv = ProcessChildren(aPresShell, aPresContext, aState, creatorContent,
> >+                         insertionFrame, PR_TRUE, insertionItems,
> >+                         aState.mCreatorIsBlock);
> 
> Why are we passing aCanHaveGeneratedContent == PR_TRUE here?  Did we never
> process the children of creatorContent before?	If so, where is that enforced? 
> In ConstructXTFFrame?

I don't know, I didn't write that, I'm just moving it... I don't know how that
parameter is supposed to be used.

Comment 17

[Boris Zbarsky [:bzbarsky]]

It's supposed to be used to control whether :before/:after generated content is
allowed on the content in question.  If it is, ConstructChildren will construct
frames for it and put them in the frame list.

In your case, this would be the :before/:after content of creatorContent.  I'm
not sure how XTF specifies the interaction of insertion points and
:before/:after content (I suspect it simply pretends that the latter does not
exist), so I don't know what the right thing to pass is.

Comment 18

[Brian Ryner (not reading)]

Attachment: patch v5

I think this addresses all of Boris's comments, except for the generated
content thing, for which I don't know what the right thing to do is.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 167492 [details] [diff] [review]
patch v5

>+  // The nsIAnonymousContentCreator we're currently constructing children for.

I think this probably deserves an "XXX temporary" (even if it ends up being not
all that temporary) comment explaining that XTF should really reuse the same
ChildIterator stuff that XBL uses.

>+  nsIFrame                 *mAnonymousCreator;
>+  // The insertion point node for mAnonymousCreator.
>+  nsIContent               *mInsertionContent;
>+  // Whether the parent is a block (see ProcessChildren's aParentIsBlock)
>+  PRBool                    mCreatorIsBlock;


>+nsIFrame*
>+nsXTFFrameUtils::GetContentInsertionFrame(nsIFrame *aFrame)
...
>+  return insertionFrame;

I think this should be

  return insertionFrame->GetContentInsertionFrame();

That's what all the other implementations of GetContentInsertionFrame do, and
it seems right, since you don't want to try to insert between an inner and
outer table just because it happens to be the insertion point for XTF.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 167492 [details] [diff] [review]
patch v5

>+  inline NS_HIDDEN_(nsresult)
>+    CreateInsertionPointChildren(nsIPresShell *aPresShell,
>+                                 nsPresContext *aPresContext,
>+                                 nsFrameConstructorState &aState,
>+                                 nsIFrame *aNewFrame,
>+                                 nsIContent *aContent,
>+                                 PRBool aUseInsertionFrame = PR_TRUE);

Call this one MaybeCreateInsertionPointChildren, or something to that effect. 
Using overloading for functions with different semantics can lead to confusion.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, as we discussed on the whiteboard (in three colors), the handling of nested
bindings (where there's a binding attached to the insertion point of another
binding) isn't right.  My main objection is that the current handling breaks the
invariant that if B has an XTF binding that says its children should be put
inside a FOO in its binding subtree, it can end up with arbitrary children that
aren't inside the FOO because it was the insertion point of an outer binding.

Comment 22

[Brian Ryner (not reading)]

Ok, so there are basically two ways to avoid this problem:

1. Ignore explicit children from outer bindings.  I think that just means
keeping track of a single insertion node at a time instead of a chain of them.

2. Reorganize things so that when we hit an innermost insertion point (haven't
_quite_ figured out what the test for that would be), we would walk up the chain
of bindings and create the explicit children for each of them.

Approach 2 is closer to how XBL works.  I may punt on this for now and implement
approach 1, which I think is still better than inserting the outer binding's
explicit children at the wrong place.

Comment 23

[Brian Ryner (not reading)]

(In reply to comment #21)
> And, as we discussed on the whiteboard (in three colors), the handling of nested
> bindings (where there's a binding attached to the insertion point of another
> binding) isn't right.  My main objection is that the current handling breaks the
> invariant that if B has an XTF binding that says its children should be put
> inside a FOO in its binding subtree, it can end up with arbitrary children that
> aren't inside the FOO because it was the insertion point of an outer binding.

I gave this some more thought today.  I believe that the existing code has the
same problem, so this wouldn't be a regression (the current code calls
GetContentInsertionFrame on the outermost binding, but doesn't do it
recursively).  And since I'd like to totally redo this to use the binding
manager, I'd like to do the simplest possible change that will get XForms up and
working.

There's one option that may be simpler than this patch and accomplishes that
goal.  We can just track a single nsIAnonymousContentCreator and insertion point
pair.  The current creator and IP remain unchanged if we begin creating
anonyomus children for a non-XTF frame.  If we start creating anonymous children
for an XTF frame, and the content node for that frame is the current insertion
point, then we simply replace our nsIAnonymousContentCreator and insertion point
with the new frame and insertion point.  So, explicit children from outer
bindings would be dropped.

I don't feel strongly about one approach or the other since it's going to be a
temporary situation.  I'd probably prefer to go with what's here since it
doesn't regress anything and is known to otherwise work.

Comment 24

[Brian Ryner (not reading)]

Comment on attachment 167492 [details] [diff] [review]
patch v5

re-requesting review, please see the above comment for rationale

Comment 25

[Allan Beaufour]

(In reply to comment #24)
> (From update of attachment 167492 [details] [diff] [review])
> re-requesting review, please see the above comment for rationale

:( Your patch is out-of-sync with the current tree.

Comment 26

[Brian Ryner (not reading)]

Attachment: updated patch

updated to the trunk

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 168816 [details] [diff] [review]
updated patch

I'd rather see this done right, but sr=dbaron.

Comment 28

[Brian Ryner (not reading)]

checked in.