TableProcessChild makes some bogus assumptions

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: bz, Assigned: Bernd)

Tracking

(Blocks: 2 bugs)

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: See comment 46 and following about tests)

Attachments

(7 attachments, 2 obsolete attachments)

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).
Note that we construct an nsTableCellFrame for the input in that testcase!
(Assignee)

Comment 3

12 years ago
Created attachment 209222 [details] [diff] [review]
¡No Pasarán!
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attachment #209222 - Flags: superreview?(bzbarsky)
Attachment #209222 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

12 years ago
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.
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.
(Assignee)

Comment 6

12 years ago
Isn't this more a general problem of IsSpecialContent?
The other callers pass in the post-XBL-resolution namespace and namespace ID, no?
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.
Attachment #209222 - Flags: superreview?(bzbarsky)
Attachment #209222 - Flags: superreview-
Attachment #209222 - Flags: review?(bzbarsky)
Attachment #209222 - Flags: review-
(Assignee)

Comment 9

12 years ago
>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.
(Assignee)

Comment 10

12 years ago
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".
(Assignee)

Comment 11

12 years ago
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.
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?
(Assignee)

Comment 13

12 years ago
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>



     
> 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..
Created attachment 224200 [details]
Testcase that shows mathml weirdness

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

Comment 16

12 years ago
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)

Updated

12 years ago
Blocks: 348811
(Assignee)

Comment 17

12 years ago
Created attachment 241050 [details] [diff] [review]
patch

patch that adresses the core problem, it removes TableProcessChild.
Attachment #209222 - Attachment is obsolete: true
(Assignee)

Comment 18

12 years ago
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.
Attachment #241050 - Flags: review?(rbs)

Comment 19

12 years ago
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;
+    }
(Assignee)

Comment 20

12 years ago
> 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> :-(
(Assignee)

Comment 21

12 years ago
I think nsCSSFrameConstructor::AdjustParentFrame is the ideal place to do it as it is called in ConstructFrameInternal
(Assignee)

Updated

12 years ago
Blocks: 323656
(Assignee)

Comment 22

11 years ago
Created attachment 245396 [details]
colgroup foreign child supression test
(Assignee)

Comment 23

11 years ago
Created attachment 245405 [details] [diff] [review]
rev1

The patch addresses the colgroup child concern but leaves the caption stuff as described by the previous comments.
Attachment #241050 - Attachment is obsolete: true
Attachment #245405 - Flags: review?
Attachment #241050 - Flags: review?(rbs)
(Assignee)

Updated

11 years ago
Attachment #245405 - Flags: review? → review?(rbs)

Comment 24

11 years ago
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.)
(Assignee)

Comment 25

11 years ago
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

11 years ago
Comment on attachment 245405 [details] [diff] [review]
rev1

r=rbs
Attachment #245405 - Flags: review?(rbs) → review+
(Assignee)

Comment 27

11 years ago
Comment on attachment 245405 [details] [diff] [review]
rev1

Boris, this is the next round of "tit for tat" (88k patch size)
Attachment #245405 - Flags: superreview?(bzbarsky)
Heh.  I'm kinda swamped until Monday; I should hopefully get to this on Monday night.
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.
Attachment #245405 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 30

11 years ago
> 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

(Assignee)

Comment 31

11 years ago
> 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.
(Assignee)

Comment 32

11 years ago
>  And possibly aCreatedPseudo and whatever else you can stand to document while you're here?
they are already documented 
(Assignee)

Comment 33

11 years ago
Created attachment 249761 [details] [diff] [review]
patch that got checked in
(Assignee)

Comment 34

11 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 365191
(Assignee)

Comment 35

11 years ago
backed out due to bug 365191
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

11 years ago
Created attachment 250100 [details] [diff] [review]
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); 
-      }
Attachment #250100 - Flags: superreview?(bzbarsky)
Attachment #250100 - Flags: review?(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?
(Assignee)

Comment 38

11 years ago
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 on attachment 250100 [details] [diff] [review]
revised patch

Right.  That matches what I was thinking.  r+sr=bzbarsky
Attachment #250100 - Flags: superreview?(bzbarsky)
Attachment #250100 - Flags: superreview+
Attachment #250100 - Flags: review?(bzbarsky)
Attachment #250100 - Flags: review+
(Assignee)

Comment 40

11 years ago
fix checked in again
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 41

11 years ago
Any chance of this patch/capability finding its way into FF2?
In my opinion, no.  This is very scary fragile code, and we're not even sure what all the patch depends on.

Updated

11 years ago
Blocks: 351236
Created attachment 254124 [details] [diff] [review]
Remove some dead code

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

Updated

11 years ago
No longer blocks: 351236

Comment 44

10 years ago
Crashtests checked in.
Flags: in-testsuite? → in-testsuite+

Comment 45

10 years ago
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.
(Assignee)

Comment 46

10 years ago
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.
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.
Flags: in-testsuite+ → in-testsuite?
Whiteboard: See comment 46 and following about tests
You need to log in before you can comment on or make changes to this bug.