1. Load the page 2. Look at the left table 3. In Moz it is thiner as it should Opera and IE shows OK. moz 2002011403

Over to tables....

looks pretty much like the fieldset is not returning the correct min width.

taking

Comment on attachment 66174 [details] [diff] [review] patch if maxElementSize is not being used, change + nsSize maxElementSize; + nsSize* pMaxElementSize = aDesiredSize.maxElementSize; + if (NS_UNCONSTRAINEDSIZE==aReflowState.availableWidth) + pMaxElementSize = &maxElementSize; to + nsSize* pMaxElementSize = (NS_UNCONSTRAINEDSIZE == aReflowState.availableWidth) ? nsnull : aDesiredSize.maxElementSize;

*** Bug 60375 has been marked as a duplicate of this bug. ***

patch including Chris comments

Comment on attachment 66903 [details] [diff] [review] patch r=karnaze

Comment on attachment 66903 [details] [diff] [review] patch No need to introduct whitespace here: >@@ -432,6 +442,7 @@ > // reflow the content frame only if needed > if (mContentFrame) { > if (reflowContent) { >+ > availSize.width = aReflowState.mComputedWidth; > > #if 0 nsHTMLReflowMetrics ctor will zero width, height, ascent, and descent. No need to do it again, is there? >@@ -445,9 +456,10 @@ > availSize); > > kidReflowState.reason = reason; >- >- nsHTMLReflowMetrics kidDesiredSize(0,0); > >+ nsHTMLReflowMetrics kidDesiredSize(aDesiredSize.maxElementSize, aDesiredSize.mFlags); >+ kidDesiredSize.width = kidDesiredSize.height = kidDesiredSize.ascent = kidDesiredSize.descent =0; >+ > // Reflow the frame > ReflowChild(mContentFrame, aPresContext, kidDesiredSize, kidReflowState, > kidReflowState.mComputedMargin.left, kidReflowState.mComputedMargin.top, Funky indenting on the first `if', add a space. Dangling space at the end of your condition. This code is very hairy, so explain why we're doing what we're doing. E.g., something like ``if we've been given more space in which to flow ourselves than the content area needed, position the legend frame using the expanded width.'' (Did I get it right?) Also, why _don't_ we expand the contentRect here? In the |else| case, we shrink the content rect. (Why is it okay to drop the border+padding?) >@@ -509,19 +521,24 @@ > } > > if (mLegendFrame) { >- if (contentRect.width > mLegendRect.width) { >+ nscoord fieldwidth = contentRect.width; >+ if (aReflowState.mComputedWidth != NS_INTRINSICSIZE && aReflowState.mComputedWidth > fieldwidth ) >+ fieldwidth = aReflowState.mComputedWidth; >+ if (fieldwidth > mLegendRect.width) { > PRInt32 align = ((nsLegendFrame*)mLegendFrame)->GetAlign(); > > switch(align) { > case NS_STYLE_TEXT_ALIGN_RIGHT: >- mLegendRect.x = contentRect.width - mLegendRect.width + borderPadding.left; >+ mLegendRect.x = fieldwidth - mLegendRect.width + borderPadding.left; > break; > case NS_STYLE_TEXT_ALIGN_CENTER: >- mLegendRect.x = contentRect.width/2 - mLegendRect.width/2 + borderPadding.left; >+ float p2t; >+ aPresContext->GetPixelsToTwips(&p2t); >+ mLegendRect.x = NSIntPixelsToTwips((nscoord) NSToIntRound((float)(fieldwidth/2 - mLegendRect.width/2 + borderPadding.left) / p2t),p2t); > } > > } else { >- contentRect.width = mLegendRect.width + borderPadding.left + borderPadding.right; >+ contentRect.width = mLegendRect.width; > aDesiredSize.width = contentRect.width; > } > // place the legend Since we add the borderPadding either way, why not hoist that out of the |if| statement; e.g., aDesiredSize.width = PR_MAX(aReflowState.mComputedWidth, contentRect.width); aDesiredSize.width += borderPadding.left + borderPadding.right; >@@ -561,7 +578,12 @@ > borderPadding.right; > } else { > nscoord min = borderPadding.left + borderPadding.right + mLegendRect.width; >- aDesiredSize.width = aReflowState.mComputedWidth + borderPadding.left + borderPadding.right; >+ if (aReflowState.mComputedWidth > contentRect.width) { >+ aDesiredSize.width = aReflowState.mComputedWidth + borderPadding.left + borderPadding.right; >+ } >+ else { >+ aDesiredSize.width = contentRect.width + borderPadding.left + borderPadding.right; >+ } > if (aDesiredSize.width < min) > aDesiredSize.width = min; > } Goofy indenting on the arguments here. >@@ -670,5 +692,13 @@ > aOldFrame, > aNewFrame); > } >- > >+NS_IMETHODIMP >+nsFieldSetFrame::GetFrameForPoint(nsIPresContext* aPresContext, >+ const nsPoint& aPoint, >+ nsFramePaintLayer aWhichLayer, >+ nsIFrame** aFrame) >+{ >+ // this should act like a block, so we need to override >+ return GetFrameForPointUsing(aPresContext, aPoint, nsnull, aWhichLayer, (aWhichLayer == NS_FRAME_PAINT_LAYER_BACKGROUND), aFrame); >+} This seems like a kludge, but I guess it's okay, since only nsFieldSetFrame ever calls GetAlign. It seems like it might be more appropriately handled at the call site, but...then I guess we couldn't take into account RTL text that's been force left-aligned? >Index: nsLegendFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/html/forms/src/nsLegendFrame.cpp,v >retrieving revision 3.45 >diff -u -r3.45 nsLegendFrame.cpp >--- nsLegendFrame.cpp 2002/01/09 19:17:46 3.45 >+++ nsLegendFrame.cpp 2002/01/29 19:07:58 >@@ -137,6 +137,13 @@ > PRInt32 nsLegendFrame::GetAlign() > { > PRInt32 intValue = NS_STYLE_TEXT_ALIGN_LEFT; >+ const nsStyleVisibility* vis; >+#ifdef IBMBIDI >+ GetStyleData(eStyleStruct_Visibility, (const nsStyleStruct*&)vis); >+ if (NS_STYLE_DIRECTION_RTL == vis->mDirection) { >+ intValue = NS_STYLE_TEXT_ALIGN_RIGHT; >+ } >+#endif // IBMBIDI > nsIHTMLContent* content = nsnull; > mContent->QueryInterface(kIHTMLContentIID, (void**) &content); > if (nsnull != content) {

patch addressing Waterson concerns

>This seems like a kludge, but I guess it's okay, since only >nsFieldSetFrame ever calls GetAlign. It seems like it might be more >appropriately handled at the call site, but...then I guess we couldn't >take into account RTL text that's been force left-aligned? This is true, in a next step I think we should get rid of this function at all (bug 117788). I think the text align should be mapped to margins. But there is no urgent need to go for this, while the patch here fixes a dupe which blocked html4.0 compliance. Hmmm, may be bryners checkin will make this stuff obsolete anyway....

Comment on attachment 66941 [details] [diff] [review] patch sr=waterson

fix checked in

The testcase works fine and looks like in IE 5.0. Marking verified.