243159 - TableProcessChild makes some bogus assumptions

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

TableProcessChild assumes that all frame construction is display-driven and
hence the only thing that matters is the display of the child.  This means that
replaced elements (eg inputs) don't get the right frames constructed for them in
some cases (testcase coming up).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that we construct an nsTableCellFrame for the input in that testcase!

Comment 3

[Bernd]

Attachment: ¡No Pasarán!

Comment 4

[Bernd]

Martijn, this patch might affect some of your crash testcases that use table display types inside table frames on things that would otherwise never honour table display types.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That doesn't seem like it would work right when XBL is involved (since the tag and namespace used for frame construction are affected by XBL)...

I really think the right way to fix this is to eliminate the separate TableProcessChildren stuff and figure out a way to make the pseudo-frame thing efficient in the regular ProcessChildren.

Comment 6

[Bernd]

Isn't this more a general problem of IsSpecialContent?

Comment 7

[Boris Zbarsky [:bzbarsky]]

The other callers pass in the post-XBL-resolution namespace and namespace ID, no?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209222 [details] [diff] [review]
¡No Pasarán!

Given the XBL thing, I don't think this works.

Frankly, if this is for trunk I think we should just remove TableProcessChild and come up with a sane way to do pseudo-frames instead of hacking around on this.

I might be willing to take the proposed patch as a branch-only patch, but note that it won't catch all cases; so it'll catch accident but not malice.

Comment 9

[Bernd]

>Frankly, if this is for trunk I think we should just remove TableProcessChild
>and come up with a sane way to do pseudo-frames instead of hacking around on
>this.

TableProcessChild and its ancestor TableProcessChildren have a second somehow hidden functionality. They preserve the TableCreator. The TableCreator seems to be a mechanism to make MathML tables to reuse the infrastructure of table frame creation. Once you hit a mtable tag you set the MathMLTableCreator which diverts the NS_NEW_xxx functions to the corresponding MAthML counterparts. So if one goes trough the normal frame creation this is reset at

nsresult
nsCSSFrameConstructor::
ConstructFrameByDisplayType(nsFrameConstructorState& aState,
                            const nsStyleDisplay*    aDisplay,
                            nsIContent*              aContent,
                            PRInt32                  aNameSpaceID,
                            nsIAtom*                 aTag,
                            nsIFrame*                aParentFrame,
                            nsStyleContext*          aStyleContext,
                            nsFrameItems&            aFrameItems,
                            PRBool                   aHasPseudoParent)
{
  PRBool    primaryFrameSet = PR_FALSE;
  nsIFrame* newFrame = nsnull;  // the frame we construct
  nsTableCreator tableCreator(mPresShell); // Used to make table frames.

Comment 10

[Bernd]

There is some beauty with the current TableProcessChild method. Instead asking every children to determine when it can be the direct child of its parent we ask the parent if this child can be a direct child or needs some wrapping. This centralizes the handling at one place and removes the performance penalty that would occure if every frame needs to determine if it can be a legitimate of its parent. Table frames implement this check in GetParentFrame() but doing this for all frames seems to me too expensive. In principle we can make a pretty good prediction if the children needs to be wrapped. However this fails with XBL frames. This is probably the deeper meaning of "XBL kills babies".

Comment 11

[Bernd]

Due to the MathML dependency before changing anything, it should be possible to observe regressions, which is currently due to the cairo bustage not the case  	see 334737.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Frankly, the current forked table frame construction codepath sucks for two reasons:

1)  It duplicates lots of code from the normal codepath.
2)  It forces us to have table-specific code in the normal codepath _anyway_, to
    handle dynamic DOM changes.

I think the right flow of control for frame construction should be:

1)  Determine what kind of frame we need to create for the current child (say
    determine the function to call to handle creation and init, as well as any
    args to pass to said function).
2)  If that frame can't be a child of the current parent, do whatever wrapping
    is needed.
3)  Construct the child frame using the data determined in step 1 and the
    adjusted parent.

Right now step 1 and step 3 are lumped together and step 2 happens before either one and relies on guesswork about what will happen when we get to step 1.  We want to eliminate that guessing.

I suspect if we did this, MathML could do whatever it needs to during step 1 here, no?  Where do we need the table creator, exactly?  Constructing the right table pseudo-frames?

Comment 13

[Bernd]

There a couple of MathML tags that look like their html counterparts.
http://lxr.mozilla.org/seamonkey/source/layout/mathml/content/src/mathml.css#271
For those we specify table display types. But we don't want table frames for them we want MathML frames. So instead of using seperate display types we take a cheap ride on the allready existing table frame construction code and fork only when it comes to down to create a new frame. The ticket for the cheap ride is the tablecreator. 

We create a nsMathMLmtableCreator and override the methods from the base.
// Structure used when creating MathML mtable frames
struct nsMathMLmtableCreator: public nsTableCreator {
  virtual nsIFrame* CreateTableOuterFrame(nsStyleContext* aStyleContext);
  virtual nsIFrame* CreateTableFrame(nsStyleContext* aStyleContext);
  virtual nsIFrame* CreateTableRowFrame(nsStyleContext* aStyleContext);
  virtual nsIFrame* CreateTableCellFrame(nsIFrame* aParentFrame, nsStyleContext* aStyleContext);
  virtual nsIFrame* CreateTableCellInnerFrame(nsStyleContext* aStyleContext);

  nsMathMLmtableCreator(nsIPresShell* aPresShell)
    :nsTableCreator(aPresShell) {};
};

for an mtable tag we call 
nsMathMLmtableCreator mathTableCreator(mPresShell);
and start our ride TableProcessChild makes sure the mathTableCreator will not be altered and all tags with a table display type inside the mtable will be diverted to their MathML counter parts. 

This will however fail if you don't wrap a mtr with a mtable  but thats a different story.

>2)  If that frame can't be a child of the current parent, do whatever wrapping
    is needed.
That means all and every frame needs to check that it can be the direct children of its parent? Doesn't it? Is that optimal from a performance point of view?

The finding of the adjusted parent is not trivial, basically thats the handle pseudo content insert/append bug.
<parent frame content based>
  <pseudo 1>
   <pseudo 2> 
    <pseudo 3>
      <child 1 content based>

Now we hit child2 if we look up we would have the same parent but we dont need pseudo 3 to wrap our frame should be sibling.
the correct structure would be:
<parent frame content based>
  <pseudo 1>
   <pseudo 2> 
    <pseudo 3>   <child 2 content based>
      <child 1 content based>

In a cheap lookup (what we have now for the dynamic case) we get

<parent frame content based>
  <pseudo 1>                 <pseudo1 sibling>
   <pseudo 2>                  <pseudo 4>
    <pseudo 3>                   <child 2 content based>
      <child 1 content based>



     

Comment 14

[Boris Zbarsky [:bzbarsky]]

> and fork only when it comes to down to create a new frame.

So if I understand the current code right, if I dynamically insert an <mtd> as a child of an <mtr> we will construct a TableCellFrame for it, but if I insert the pair as a unit we'll construct a mathml frame (which inherits from TableCellFrame and doesn't impl the relevant nsIFrameDebug parts so looks identical in frame dumps).

In any case, ccing rbs, since we're talking about mathml..

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows mathml weirdness

The frame structures for the two <mtable>s should be identical.  They're _so_ not.

Comment 16

[rbs]

The weirdness is resolved by the fix for bug 348153 and the iteration in bug 348811. Here is a dump of the frame tree that I now get with the testcase:

Inline(math)
  Frame(mtable) [= nsMathMLmrowFrame-wrapper to get the inline-table effect]
    Block(mtable) [= block wrapper]
      line 04E38564: count=1 
        TableOuter(mtable)
          Table(mtable)
            TableRowGroup(mtable)
              TableRow(mtr)

  Frame(mtable) [= nsMathMLmrowFrame-wrapper]
    Block(mtable) [= block wrapper]
      line 04E387C0: count=1
        TableOuter(mtable)
          Table(mtable)
            TableRowGroup(mtable)
              TableRow(mtr)

Comment 17

[Bernd]

Attachment: patch

patch that adresses the core problem, it removes TableProcessChild.

Comment 18

[Bernd]

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

Roger, can you please review at least the part that deals with MathML. It follows basically the  idea from your mail (Tue, 26 Sep 2006 14:05:54)
Its huge I know ;-) but if you find wrong things first it would reduce the  burden on boris.

Comment 19

[rbs]

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

There were 3 particular things that the previous code was doing. Care to document how you resolved them in the new code?

1-    if (!aCaption) {  // only allow one caption

2-      return rv; // construct only table columns below colgroups

3-  // for every table related frame except captions and ones with pseudo parents, 
-  // link into the child list


(in particular, it seems like you can have several captions in the new code)
+    nsFrameItems captionItems;
+    nsIFrame *child = childItems.childList;
+    while (child) {
+      nsIFrame *nextSibling = child->GetNextSibling();
+      if (nsLayoutAtoms::tableCaptionFrame == child->GetType()) {
+        childItems.RemoveChild(child);
+        captionItems.AddChild(child);
+      }
+      child = nextSibling;
+    }

Comment 20

[Bernd]

> 1-    if (!aCaption) {  // only allow one caption

this is obsolete as the we can have multiple captions but we display only the first one. See https://bugzilla.mozilla.org/show_bug.cgi?id=309322
This was necessary: as otherwise we first create a caption frame and then arrgh we already  have one  lets destroy it but the destruction during construction has always issues with tearing down the placeholders etc. You know Martijn and Mats what they can put into a innocent looking caption.
this also answers 3

> 2-      return rv; // construct only table columns below colgroups
hmm, we need that and I need to think more about it. The current code is anyway wrong if you take special content and style it with display:table-col then you will bypass the criteria and create a colframe for a <select> :-(

Comment 21

[Bernd]

I think nsCSSFrameConstructor::AdjustParentFrame is the ideal place to do it as it is called in ConstructFrameInternal

Comment 22

[Bernd]

Attachment: colgroup foreign child supression test

Comment 23

[Bernd]

Attachment: rev1

The patch addresses the colgroup child concern but leaves the caption stuff as described by the previous comments.

Comment 24

[rbs]

Comment on attachment 245405 [details] [diff] [review]
rev1

OK, understood. Care to clarify the following:

-  if (aParentFrame->GetType() == nsLayoutAtoms::tableOuterFrame) {
+  nsIAtom* parentType = aParentFrame->GetType();
+  if (parentType == nsLayoutAtoms::tableOuterFrame) {
     childIsSpecialContent = IsSpecialContent(aChildContent, aTag, aNameSpaceID,
                                              aChildStyle);
     if (childIsSpecialContent ||
        (aChildStyle->GetStyleDisplay()->mDisplay !=
        NS_STYLE_DISPLAY_TABLE_CAPTION)) {
       aParentFrame = aParentFrame->GetContentInsertionFrame();
     }
   }
+  else if (parentType == nsLayoutAtoms::tableColGroupFrame) {
+    childIsSpecialContent = IsSpecialContent(aChildContent, aTag, aNameSpaceID,
+                                             aChildStyle);
+    if (childIsSpecialContent ||
+       (aChildStyle->GetStyleDisplay()->mDisplay !=
+        NS_STYLE_DISPLAY_TABLE_COLUMN)) {
+      aSuppressFrame = PR_TRUE;
+      return NS_OK;
+    }
+  }


What happens when aParentFrame *has* changed (as seen above in the first |if|)? Is the |else if| necessary, or should you instead resync the parentType and then use another |if| rather than |else if|. (Note that I am just prompting you to think through this aspect so that you can confirm that the patch does what you expect.)

Comment 25

[Bernd]

If the first if clause is true the parent frame might change from a outer table frame to a inner one, but it would not make it a colgroup. So tried to optimize this but this might be over optimized. The concern is that we call this for every frame that we construct.

Comment 26

[rbs]

Comment on attachment 245405 [details] [diff] [review]
rev1

r=rbs

Comment 27

[Bernd]

Comment on attachment 245405 [details] [diff] [review]
rev1

Boris, this is the next round of "tit for tat" (88k patch size)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Heh.  I'm kinda swamped until Monday; I should hopefully get to this on Monday night.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 245405 [details] [diff] [review]
rev1

>Index: nsCSSFrameConstructor.cpp
> nsCSSFrameConstructor::AdjustParentFrame(nsFrameConstructorState&     aState,

>-
>+  

Don't make the whitespace-only change?

>-  }
>+  } 

The new whitespace here is wrong.

What code is the aSuppressFrame replacing, exactly?

> nsCSSFrameConstructor::ConstructTableFrame(nsFrameConstructorState& aState,

>+        childItems.RemoveChild(child);

Yuck.  RemoveChild should really take a prev sibling hint or something.  I guess file a followup bug on that?

This change allows multiple caption frames, right?  Do we handle that OK?

> nsCSSFrameConstructor::ConstructTableRowGroupFrame(nsFrameConstructorState& aState,
>+    rv = ProcessChildren(aState, aContent, aNewFrame, PR_TRUE, childItems,
>+                         PR_FALSE);

So this is a change to allow generated content for row groups, right?  I don't think we allowed it before.  Has this been tested?  Is it perhaps worth toggling this boolean as a separate patch?  Or not really?

> nsCSSFrameConstructor::ConstructTableColGroupFrame(nsFrameConstructorState& aState,
>+    rv = ProcessChildren(aState, aContent, aNewFrame, PR_TRUE, childItems,
>+                         PR_FALSE);

Same here as for row group frames.  Possibly even more so given the colgroup magic in AdjustParentFrame?

> nsCSSFrameConstructor::ConstructTableRowFrame(nsFrameConstructorState& aState,
>+#ifdef MOZ_MATHML
>+  if (kNameSpaceID_MathML == aNameSpaceID)
>+    aNewFrame = NS_NewMathMLmtrFrame(mPresShell, aStyleContext);
>+  else
>+#endif
>+    aNewFrame = NS_NewTableRowFrame(mPresShell, aStyleContext);

The indent here is different from that in ConstructTableFrame when constructing the outer, but the same as when constructing the inner... Pick one and stick with it?

>+    rv = ProcessChildren(aState, aContent, aNewFrame, PR_TRUE, childItems,
>+                         PR_FALSE);

Usual question about generated content.

> nsCSSFrameConstructor::ConstructTableCellFrame(nsFrameConstructorState& aState,
>+  #ifdef MOZ_MATHML
>+  if ((kNameSpaceID_MathML == aNameSpaceID) && !IsBorderCollapse(parentFrame))
>+    aNewCellOuterFrame = NS_NewMathMLmtdFrame(mPresShell, aStyleContext);
>+  else
>+#endif
>+    aNewCellOuterFrame = NS_NewTableCellFrame(mPresShell, aStyleContext,
>+                                              IsBorderCollapse(parentFrame));

This has indentation issues.  Also, remove the extra parens in the if condition.  And put back in the comment that used to be in nsMathMLmtableCreator::CreateTableCellFrame.

>+#ifdef MOZ_MATHML
>+  if (kNameSpaceID_MathML == aNameSpaceID)
>+    aNewCellInnerFrame = NS_NewMathMLmtdInnerFrame(mPresShell,innerPseudoStyle);
>+  else
>+#endif
>+   aNewCellInnerFrame = NS_NewTableCellInnerFrame(mPresShell, innerPseudoStyle);

Again, indentation issues.  And put in a space after comma in the MathML call.

> nsCSSFrameConstructor::ConstructTableForeignFrame(nsFrameConstructorState& aState,

Isn't this unused now?  Just remove it instead of "fixing" it?  ;)

>@@ -6859,38 +6605,29 @@ nsCSSFrameConstructor::ConstructFrameByD
>+      rv = ConstructTableRowGroupFrame(aState, aContent, aParentFrame,
>+                                       aStyleContext, aNameSpaceID,PR_FALSE,

Space after comma?

>Index: nsCSSFrameConstructor.h

>   nsresult AdjustParentFrame(nsFrameConstructorState&     aState,
>                              nsIContent*                  aChildContent,
>                              nsIFrame* &                  aParentFrame,
>                              nsIAtom*                     aTag,
>                              PRInt32                      aNameSpaceID,
>                              nsStyleContext*              aChildStyle,
>                              nsFrameItems* &              aFrameItems,
>                              nsFrameConstructorSaveState& aSaveState,
>+                             PRBool&                      aSuppressFrame,
>                              PRBool&                      aCreatedPseudo);

Please document the new argument?  And possibly aCreatedPseudo and whatever else you can stand to document while you're here?

This looks excellent!  I can't guarantee that there aren't any issues, esp. with pseudos, but it sure looks to me like it should be fine!

r+sr=bzbarsky after answers to the multiple captions and generated content questions.

Comment 30

[Bernd]

> what code is the aSuppressFrame replacing, exactly?

-  // Resolve the style context and get its display
-  childStyleContext = ResolveStyleContext(aParentFrame, aChildContent);
-  const nsStyleDisplay* childDisplay = childStyleContext->GetStyleDisplay();
-  if (nsLayoutAtoms::tableColGroupFrame == aParentFrameType &&
-      NS_STYLE_DISPLAY_TABLE_COLUMN != childDisplay->mDisplay)
-      return rv; // construct only table columns below colgroups

>Yuck.  RemoveChild ...
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=365087

>So this is a change to allow generated content for row groups, right?  I don't
>think we allowed it before.  Has this been tested?
NO, not a single bit, this is just brain dead copy & paste
> Is it perhaps worth
>toggling this boolean as a separate patch?  Or not really?

OK, lets play safe 
- don't allow generated content as before
- write a couple of test cases that prove the functionality
- enable the bit

I will fix the whitespace issues and add some comments

Comment 31

[Bernd]

> This change allows multiple caption frames, right?  Do we handle that OK?

I think so, I added the support to get previously on trunk to get of the situation where we create a caption and then Oh surprise we have already one lets kill it now and then some insane stuff inside goes on whatever you can imagine child list and will crash shortly after it.

Comment 32

[Bernd]

>  And possibly aCreatedPseudo and whatever else you can stand to document while you're here?
they are already documented 

Comment 33

[Bernd]

Attachment: patch that got checked in

Comment 34

[Bernd]

fix checked in

Comment 35

[Bernd]

backed out due to bug 365191

Comment 36

[Bernd]

Attachment: revised patch

the patch avoids breaking acid2 and yahoo.co the difference to the previous one
is
-      // XXXbz this isn't quite right, but checking for aHasPseudoParent here
-      // would just lead us to ProcessPseudoFrames inside ConstructTableFrame.
-      if (!aState.mPseudoFrames.IsEmpty()) {
-        ProcessPseudoFrames(aState, aFrameItems); 
-      }

Comment 37

[Boris Zbarsky [:bzbarsky]]

So why is that change needed?  And why is it ok?  Is the point is that since we're going through ConstructFrame any pseudo-frames that appear here are due to a table-row parent of the table or something like that, hence shouldn't be processed?

Comment 38

[Bernd]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1292&mark=6824-6828#6806
processes the previous pseudo frames always.

so imagine the following tree (thats the essence of acid2)

<foo style="display:table">
 <bar style="display:table-cell">
 <baz style="display:table">
 <zap style="display:table-cell">

 <baz> should not cause a processing of the pseudo frames. If it does, it causes a append situation as the processing will set the initial childlist on the outer table <foo>.

We never hit that code before with pseudos as the tableprocess child did not have this code.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1290&mark=4209,4210#4200

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 250100 [details] [diff] [review]
revised patch

Right.  That matches what I was thinking.  r+sr=bzbarsky

Comment 40

[Bernd]

fix checked in again

Comment 41

[aaronr]

Any chance of this patch/capability finding its way into FF2?

Comment 42

[Boris Zbarsky [:bzbarsky]]

In my opinion, no.  This is very scary fragile code, and we're not even sure what all the patch depends on.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove some dead code

With this landing, this function became unused...  I've checked this in, rs=roc

Comment 44

[Jesse Ruderman]

Crashtests checked in.

Comment 45

[Jesse Ruderman]

Oops, I meant to check in crashtests for bug 364427, but I checked in crashtests for this bug instead.  I hope these weren't supposed to be reftests or something lower-level.

Comment 46

[Bernd]

They aren't crash tests per se, this is a deep infrastructure bug that YOU can easily convert into bazillions of crashes. Oh, wait now you can't anymore ;-) .
https://bugzilla.mozilla.org/attachment.cgi?id=245396 should be a reftest against about:blank without the alert.

Comment 47

[Boris Zbarsky [:bzbarsky]]

The first testcase for this bug should be a reftest.

In general, we could use some reftests for table pseudo stuff, including testing of all aspects of IsSpecialContent.