Closed Bug 458296 Opened 16 years ago Closed 16 years ago

padding-bottom is not getting added to scrollheight in FF3

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: rwhite, Assigned: roc)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(3 files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17 I have a manually setup scrollbar that is using the scrollHeight on an element to determine the scroll range. It works in FF2 but not in FF3 because the padding-bottom on the area is not getting added to the scrollHeight. It gets added to the clientHeight and the offsetHeight, but not the scrollHeight. Reproducible: Always Steps to Reproduce: 1. (in Firebug) Look at the offsetHeight and scrollHeight in the DOM on an element 2. Add padding-bottom to it in the style tab and re-check the DOM tab. 3. In FF2, both values are updated. In FF3, only the offsetHeight is updated. It seems to be working on smaller divs but for the one I am using, I have overflow: hidden, height: 80%, padding-bottom: 200px. If FF2, the scrollHeight is 5974 and in FF3 its 5774.
Could you attach a testcase using the Add an attachment link?
I'm sorry but I can't give out the url because it is a client site for BankOfAmerica so I can't give out a username and password for it. I'll try and setup a test system and see if it has the same issues.
I have a test site setup. The url is acmerealty.livemanager.com, user: test1 password: test1. After you log in, click on the Dealers tab in the left nav. The scroll bar is setup for the list on the left. It updates the scrollTop manually, but I can't adjust the scrollTop to go higher than the scrollHeight - offsetHeight. So in FF3, it doesn't scroll all the way to the bottom of the list. Let me know if you need any more details. Thanks for the help.
Thanks! This regressed on 4 Dec 2007. Works: 2007120419 Fails: 2007120419 Bonsai link: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2007-12-04+18%3A00&maxdate=2007-12-04+20%3A00 Probably caused by Bug 375304.
Blocks: 375304
Component: General → Layout
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
Flags: blocking1.9.1?
Attached file testcase (deleted) —
Keywords: testcase
Assignee: nobody → roc
Have there been any updates for this?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9.1? → wanted1.9.1+
Bug 375304 definitely caused this. The fix for that bug made some things work better by propagating the computed height (and min/max height) of a scrollable element to its anonymous scrolled block frame. But that broke this because the padding of the scrollable element is basically placed at the bottom edge of the scrollable element instead of being "wrapped around" the inner DIV. Interestingly, Webkit has the same bug as us here.
Arguably our current rendering is correct if you consider the bottom-padding of a scrollable element to not move with the scrolling --- i.e., permanently attached to the bottom edge of the element. But that's certainly unintuitive.
So it looks like the cleanest approach would be to partially revert bug 375304 and let the scrolled frame not have its computed height set to the scrollframe's computed height. Then its height can follow the height of its children and this bug goes away. But then we have to fix bug 375304 some other way, probably by applying min/max constraints in CalculateContainingBlockSizeForAbsolutes.
And in general computed heights (and min/max heights) propagated in nsHTMLReflowState::CalcQuirkContainingBlockHeight and nsHTMLReflowState::InitConstraints have to be adjusted for scrollbars.
Changing that back is quite a big change, since it requires adjusting percentage-height calculations in a few places to account for a possible horizontal scrollbar, but so far it still seems like the right thing to do. Forcing the scrolled frame computed height to the scrollframe's height puts the padding in the wrong place; we could work around it, but fundamentally it's ugly and wrong.
Attached patch fix (deleted) — Splinter Review
Okay, here's a patch that partially reverts bug 375304, so that scrolled frames no longer get a computed height. Instead they're allowed to choose their intrinsic height, fixing this bug. Most of this patch is changes to avoid regressing a bunch of stuff. CalculateContainingBlockSizeForAbsolutes needs to change so that the adjustments for the scrollable parent frame's size occur even when the block has unconstrained computed height. We also need to fix the cbHeightChanged check to be more conservative when CalculateContainingBlockSizeForAbsolutes is monkeying with things (this is probably a bug on trunk, actually). I also removed the part that was handling a root block element as an abs-pos container for the viewport, which doesn't happen anymore (since CanvasFrame became an abs-pos container). nsHTMLScrollFrame::ReflowScrolledFrame no longer needs to set the computed height (and min/max heights) of the scrolled frame. But we set the child's mVResize flag if our height and/or horizontal scrollbar are changing in a way that means our "computed height minus scrollbar" value would change, so that descendants dependent on that height will be reflowed. CanvasFrame can no longer use its own height as the abs-pos containing block height. It has to compute that height from its parent. nsHTMLReflowState needs to re-add some ugly code to handle scrolled frames correctly when we're looking for an ancestor frame to use for percentage-height calculations. We're adding some utility methods here to adjust for the presence of horizontal scrollbar. Scrolled nsTableRowGroupFrames have to explicitly ask for the computed height that's based on the scrolled frame possibly minus the horizontal scrollbar height. I'm not convinced yet that this patch is the right way to go overall.
That patch fails a few reftests, by the way. The setting of the mVResize flag in ReflowScrolledFrame is suspect.
Attached patch alternative fix (deleted) — Splinter Review
I think this is better overall. Leave the computed height for scrolled frames. Just ensure that the bottom margin-edge of block children is included in the block's overflow area, and for scrolled blocks, the bottom-padding is added too. The bottom margin-edges are included not only for margin-root blocks, but also for blocks with computed height, since we know the outgoing margins are not going to be used anywhere so if they're going to affect the overflow height we need to include them here. This is very simple and low-risk. Ultimately it probably fixes more behaviour than the other patch too, since it adds to the overflow area of any computed-height block, not just scrolled blocks. In particular it happens to fix 413027-1.html.
Attachment #352075 - Flags: superreview?
Attachment #352075 - Flags: review?
Attachment #352075 - Flags: superreview?(dbaron)
Attachment #352075 - Flags: superreview?
Attachment #352075 - Flags: review?(dbaron)
Attachment #352075 - Flags: review?
Whiteboard: [needs review]
Attachment #352075 - Flags: superreview?(dbaron)
Attachment #352075 - Flags: superreview+
Attachment #352075 - Flags: review?(dbaron)
Attachment #352075 - Flags: review+
Comment on attachment 352075 [details] [diff] [review] alternative fix r+sr=dbaron This fixes another piece of bug 47710, doesn't it?
Whiteboard: [needs review] → [needs landing]
Pushed abef302b61be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Comment on attachment 352075 [details] [diff] [review] alternative fix After this has baked a bit, I think we should consider taking it for 1.9.1, since it fixes a regression from FF2->FF3 that apparently affects sites.
Attachment #352075 - Flags: approval1.9.1?
Comment on attachment 352075 [details] [diff] [review] alternative fix a191=beltzner
Attachment #352075 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Pushed 3194fd024edc to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Is this a Windows specific bug?
No.
Ok, changing the platform to All/All .... and verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre ID:20090505030940 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre ID:20090505030932
Status: RESOLVED → VERIFIED
OS: Windows XP → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: