Marc Attinasi Reporter
	Description • 24 years ago

The ButtonControlFrame is allowing the CSSFrameConstructor to process its 
children, and it then has to go through those children and reparent them in 
nsHTMLButtonControlFrame::SetInitialChildList. Maybe it should do his the way 
the select does it?

	bsharma
	Comment 1 • 24 years ago

Updating QA contact.

	rods (gone)
	Comment 2 • 24 years ago

This is a cleanup task

	Vladimir Ermakov
	Comment 3 • 24 years ago

QA Contact Update

	Bernd Assignee
	Comment 4 • 19 years ago

Oh man this code is broken,
1.) the button does create a area frame in the init function but does not tell
the frame constructor about it so it needs to reframe on every frame access.
2.) I believe that it would be more correct to create below every button a area
frame and make the functions AppendFrames, InsertFrames, and RemoveFrame
similiar to the nsTableCell variants, e.g. you can't alter the area frame but
only area frame children.

	Bernd Assignee
	Comment 6 • 19 years ago

Mats are you able to review this or should I ask somebody else?

	Mats Palmgren (inactive)
	Comment 7 • 19 years ago

(In reply to comment #6)
> Mats are you able to review this or should I ask somebody else?
> 

Sure, I can have a look at it tomorrow.

	Bernd Assignee
	Comment 8 • 19 years ago

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

Don't hurry, the code is wrong since years, I need this code to fix bug 316026

	Mats Palmgren (inactive)
	Comment 9 • 19 years ago

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

Index: base/nsCSSFrameConstructor.cpp
+      ConstructButtonFrame(aState, aContent, aParentFrame,
+                           aTag, aStyleContext, aFrame,
+                           aStyleDisplay, aFrameHasBeenInitialized,
+                           aFrameItems);
+      aAddedToFrameList = PR_TRUE;
       return NS_UNLIKELY(!*aFrame) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;

I would prefer:
    rv = ConstructButtonFrame(...); ...; return rv;
       

+nsCSSFrameConstructor::ConstructButtonFrame(nsFrameConstructorState& aState,
+                                            nsIContent*              aContent,
+                                            nsIFrame*                aParentFrame,
+                                            nsIAtom*                 aTag,
+                                            nsStyleContext*          aStyleContext,
+                                            nsIFrame**               aNewFrame,
+                                            const nsStyleDisplay*    aStyleDisplay,
+                                            PRBool&                  aFrameHasBeenInitialized,
+                                            nsFrameItems&            aFrameItems)

Return value NS_OK implies aFrameHasBeenInitialized is true so that parameter
is redundant.


+  // Initialize the button frame
+  InitAndRestoreFrame(aState, aContent,
+                      aState.GetGeometricParent(aStyleDisplay, aParentFrame),
+                      aStyleContext, nsnull, buttonFrame);

InitAndRestoreFrame can fail.

+  nsIFrame* areaFrame = NS_NewAreaFrame(mPresShell, NS_BLOCK_SPACE_MGR | NS_BLOCK_SHRINK_WRAP);

Add "NS_ENSURE_TRUE(areaFrame, NS_ERROR_OUT_OF_MEMORY);" here.

+  InitAndRestoreFrame(aState, aContent, buttonFrame, styleContext, nsnull,
+                      areaFrame);

InitAndRestoreFrame can fail.

+  nsresult rv = aState.AddChild(buttonFrame, aFrameItems, aStyleDisplay, aContent,
+                                aStyleContext, aParentFrame);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }

Do we leak buttonFrame (and areaFrame) if AddChild() fails?
(this also applies to nsCSSFrameConstructor::ConstructHTMLFrame)

+  ProcessChildren(aState, aContent, areaFrame, PR_FALSE,
+                  childItems, PR_TRUE);

ProcessChildren can fail.

You changed the 'aCanHaveGeneratedContent' argument from true to false, so
button::before does not work with this patch - is this intentional?

Changing 'aParentIsBlock' from false to true fixes bug 229425, but now it
also applies ::first-letter/line to <button> when it's inline.
CSS 2.1 says:
"The :first-letter pseudo-element applies to block, list-item, table-cell,
table-caption and inline-block elements."
Maybe "buttonFrame->GetStyleDisplay()->IsBlockLevel()" will do?

I don't see any call to CreateViewForFrame() in ConstructButtonFrame(),
we need that in case it's abs.pos. etc.


Index: forms/nsHTMLButtonControlFrame.cpp
-  AddComputedBorderPaddingToDesiredSize(aDesiredSize, aReflowState);
+ 
 
You are leaving three empty lines here, which is two to many ;-)

	Bernd Assignee
	Comment 11 • 19 years ago

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

this is rtest'ed

	Mats Palmgren (inactive)
	Comment 12 • 19 years ago

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

nsCSSFrameConstructor.h:
>+   // ConstructButtonFrame puts the new frame in aFrameItems and
>+  // handles the kids of the button.

indent


nsHTMLButtonControlFrame.cpp:
>+  NS_PRECONDITION(PR_FALSE, "unsupported operation");
>+  return NS_ERROR_NOT_IMPLEMENTED;

Maybe s/NS_PRECONDITION/NS_NOTREACHED and
s/NS_ERROR_NOT_IMPLEMENTED/NS_ERROR_UNEXPECTED would
be more accurate if your intention is that these
methods should never be called?

r=mats

	Bernd Assignee
	Comment 13 • 19 years ago

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

Robert,
 the code that Mats proposes to change is a copy of the cellframe stuff http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableCellFrame.cpp#238
An other way would be to remove these functions at all and assert at the nsFrame level.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 19 years ago

+  // our new frame retured is the top frame which is the list frame.

"button frame"

> InitAndRestoreFrame can fail.
> ProcessChildren can fail.

These comments haven't been addressed yet.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 19 years ago

+  ProcessChildren(aState, aContent, areaFrame, PR_TRUE,
+                  childItems, buttonFrame->GetStyleDisplay()->IsBlockLevel());

Still need to check rv here.

	Bernd Assignee
	Comment 19 • 19 years ago

fix checked in

whoa, a bug filed by my cvs sr 5 years ago

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 19 years ago

Er... why are we calling ProcessChildren() on an <input type="button">?  Doesn't that assert in ProcessChildren (since IsLeaf() is true)?  Not to mention just generally behaving wrong?

Also,

> +    // the anonmyous content is allready parented to the area frame

should say "anonymous" and "already".  But that's minor compared to the other thing.

	Bernd Assignee
	Comment 21 • 19 years ago

Why should it assert? The area frame is not a leaf frame. ;-)

I will fix it soon.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 19 years ago

Er.. you're passing the areaFrame to ProcessChildren?  Doesn't that give kids the wrong style context (just like kids of table cells get the wrong style context)?

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 19 years ago

Comment on attachment 208486 [details] [diff] [review]
patch to address Boris' comments

r+sr=bzbarsky, I guess, but the style context parentage still needs to be fixed.  I guess we could just file a separate bug on that and make it depend on the table cell one and hope we solve the general problem in the 1.9 timeframe...

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 19 years ago

I guess this code was already screwing up the style context parentage, though...  It's still a bug, but we might have it on file already.  Check?

	Bernd Assignee
	Comment 26 • 19 years ago

fix checked in, I had a discussion with Boris on IRC, I will file the necessary followup bugs.

	Bernd Assignee
	Comment 27 • 19 years ago

The broken inheritance is filed under bug 323656

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 18 years ago

Hmm... So how come this removed the |triedFrame = PR_TRUE;| line for <button>?  Why was that no longer needed?

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 18 years ago

Please ignore comment 28 -- I figured it out....