336153 - No horizontal scrollbar with element inside fieldset and padding-top

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

See upcoming testcase.

You should see a horizontal scrollbar also with the testcase.
And the bottom scrollbarbutton should work normally.
This is not the case with current trunk builds.
This regressed between 2006-01-25 and 2006-01-26, making this a likely regression from bug 317375.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: another testcase

This testcase does not rely on padding in the fieldset

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This wasn't really a display lists regression ... they just exposed some really bogus reflow code in nsFieldSetFrame (surprise!), namely:

     if (aReflowState.ComputedHeight() != NS_INTRINSICSIZE &&
         borderPadding.top + mLegendSpace+kidDesiredSize.height > aReflowState.ComputedHeight()) {
       kidDesiredSize.height = aReflowState.ComputedHeight()-(borderPadding.top + mLegendSpace);
     }

Problem #1 is subtracting the fieldset's border+padding from its content height and assuming that means something sensible. It doesn't.

Problem #2 is bashing the fieldset's anonymous block frame size without adjusting anything else, like its overflow area or the layout of its children. That's what causes weird problems in the testcases in this bug.

My solution is to change the computed height of the child *before* we reflow it to subtract out the "content space" taken up by the legend, if any --- which I think is what the bogus code is trying to do. This works very well.

This patch also removes lame attempts to handle margins on the fieldset's anonymous block, which are pointless because it can't have margins. Really handling margins would complicate the fix I'm implementing here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285991 [details] [diff] [review]
fix

>Index: layout/forms/nsFieldSetFrame.cpp
>+    // Our child is "height:100%" but we actually want its height to be reduced
>+    // by the amount of content-height the legend is eating up.
>+    kidReflowState.SetComputedHeight(PR_MAX(0, aReflowState.ComputedHeight() - mLegendSpace));

If we're auto-height, the kid will be too, no?  That is, aReflowState.ComputedHeight() might be unconstrained.  You probably don't want to run this line in that case.

The rest of this looks good, but we need to address the unconstrained issue.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: good call

Sure wish we had saturating nscoords...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 286058 [details] [diff] [review]
good call

r+sr=bzbarsky

Comment 7

[Martijn Wargers (dead)]

Note to self, need to look whether this would fix the click problem I have with the "Zoek" button sometimes at http://www.mensenlinq.nl/mensenlinq/overlijdennl/site/home

Comment 8

[Reed Loden [:reed]]

Checking in layout/forms/nsFieldSetFrame.cpp;
/cvsroot/mozilla/layout/forms/nsFieldSetFrame.cpp,v  <--  nsFieldSetFrame.cpp
new revision: 3.164; previous revision: 3.163
done

Comment 9

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre

Also the problem I had at http://www.mensenlinq.nl/mensenlinq/overlijdennl/site/home seems fixed.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: reftest

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in test.