120958 - Table is thinner than it should be

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Eugene Savitsky]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Over to tables....

Comment 3

[Bernd]

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

Comment 4

[Bernd]

Attachment: patch

Comment 5

[Bernd]

taking

Comment 6

[karnaze (gone)]

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;

Comment 7

[Bernd]

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

Comment 8

[Bernd]

Attachment: patch

patch including Chris comments

Comment 9

[karnaze (gone)]

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

r=karnaze

Comment 10

[Chris Waterson]

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) {

Comment 11

[Bernd]

Attachment: patch

patch addressing Waterson concerns

Comment 12

[Bernd]

>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 13

[Chris Waterson]

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

sr=waterson

Comment 14

[Bernd]

fix checked in

Comment 15

[Amarendra Hanumanula]

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