458296 - padding-bottom is not getting added to scrollheight in FF3

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[rwhite]

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.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Could you attach a testcase using the Add an attachment link?

Comment 2

[rwhite]

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.

Comment 3

[rwhite]

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.

Comment 4

[Ria Klaassen (not reading all bugmail)]

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.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Comment 6

[rwhite]

Have there been any updates for this?

Comment 7

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

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.

Comment 8

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

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.

Comment 9

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

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.

Comment 10

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

And in general computed heights (and min/max heights) propagated in nsHTMLReflowState::CalcQuirkContainingBlockHeight and nsHTMLReflowState::InitConstraints have to be adjusted for scrollbars.

Comment 11

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

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.

Comment 12

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

Attachment: fix

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.

Comment 13

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

That patch fails a few reftests, by the way. The setting of the mVResize flag in ReflowScrolledFrame is suspect.

Comment 14

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

Attachment: alternative fix

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 352075 [details] [diff] [review]
alternative fix

r+sr=dbaron

This fixes another piece of bug 47710, doesn't it?

Comment 16

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

Yes.

Comment 17

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

Pushed abef302b61be

Comment 18

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

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.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352075 [details] [diff] [review]
alternative fix

a191=beltzner

Comment 21

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

Pushed 3194fd024edc to 1.9.1

Comment 23

[Aakash Desai [:aakashd]]

Is this a Windows specific bug?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.

Comment 25

[Aakash Desai [:aakashd]]

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