No horizontal scrollbar with element inside fieldset and padding-top

VERIFIED FIXED in mozilla1.9beta2

Status

()

defect
P3
normal
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: martijn.martijn, Assigned: roc)

Tracking

({regression, testcase})

Trunk
mozilla1.9beta2
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCo])

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

13 years ago
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.
Reporter

Comment 1

13 years ago
Posted file testcase
Reporter

Updated

12 years ago
Flags: blocking1.9?
Assignee: nobody → roc
Flags: blocking1.9? → blocking1.9+
Posted file another testcase
This testcase does not rely on padding in the fieldset
Posted patch fix (obsolete) — Splinter Review
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.
Attachment #285991 - Flags: superreview?(bzbarsky)
Attachment #285991 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
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.
Attachment #285991 - Flags: superreview?(bzbarsky)
Attachment #285991 - Flags: superreview-
Attachment #285991 - Flags: review?(bzbarsky)
Attachment #285991 - Flags: review-
Posted patch good callSplinter Review
Sure wish we had saturating nscoords...
Attachment #285991 - Attachment is obsolete: true
Attachment #286058 - Flags: superreview?(bzbarsky)
Attachment #286058 - Flags: review?(bzbarsky)
Comment on attachment 286058 [details] [diff] [review]
good call

r+sr=bzbarsky
Attachment #286058 - Flags: superreview?(bzbarsky)
Attachment #286058 - Flags: superreview+
Attachment #286058 - Flags: review?(bzbarsky)
Attachment #286058 - Flags: review+
Whiteboard: [needs review] → [needs landing]
Whiteboard: [needs landing] → [needs landing][dbaron-1.9:RwCo]
Reporter

Comment 7

12 years ago
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
Checking in layout/forms/nsFieldSetFrame.cpp;
/cvsroot/mozilla/layout/forms/nsFieldSetFrame.cpp,v  <--  nsFieldSetFrame.cpp
new revision: 3.164; previous revision: 3.163
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing][dbaron-1.9:RwCo] → [dbaron-1.9:RwCo]
Target Milestone: --- → mozilla1.9 M10
Reporter

Comment 9

12 years ago
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.
Status: RESOLVED → VERIFIED
checked in test.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.