Last Comment Bug 458296 - padding-bottom is not getting added to scrollheight in FF3
: padding-bottom is not getting added to scrollheight in FF3
Status: VERIFIED FIXED
: regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
: 471829 487052 (view as bug list)
Depends on:
Blocks: 375304
  Show dependency treegraph
 
Reported: 2008-10-02 15:41 PDT by rwhite
Modified: 2009-05-06 11:34 PDT (History)
9 users (show)
roc: wanted1.9.1+
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (481 bytes, text/html)
2008-10-04 13:56 PDT, Martijn Wargers [:mwargers]
no flags Details
fix (30.25 KB, patch)
2008-12-08 23:41 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
alternative fix (13.75 KB, patch)
2008-12-09 01:50 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description User image rwhite 2008-10-02 15:41:21 PDT
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 User image Ria Klaassen (not reading all bugmail) 2008-10-03 10:02:07 PDT
Could you attach a testcase using the Add an attachment link?
Comment 2 User image rwhite 2008-10-03 10:12:21 PDT
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 User image rwhite 2008-10-03 14:22:55 PDT
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 User image Ria Klaassen (not reading all bugmail) 2008-10-04 01:01:48 PDT
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 User image Martijn Wargers [:mwargers] 2008-10-04 13:56:56 PDT
Created attachment 341770 [details]
testcase
Comment 6 User image rwhite 2008-10-21 12:40:45 PDT
Have there been any updates for this?
Comment 7 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-07 18:15:09 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-07 18:19:27 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-07 19:13:53 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-07 19:53:03 PST
And in general computed heights (and min/max heights) propagated in nsHTMLReflowState::CalcQuirkContainingBlockHeight and nsHTMLReflowState::InitConstraints have to be adjusted for scrollbars.
Comment 11 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-08 18:28:31 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-08 23:41:02 PST
Created attachment 352065 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-08 23:52:34 PST
That patch fails a few reftests, by the way. The setting of the mVResize flag in ReflowScrolledFrame is suspect.
Comment 14 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-09 01:50:34 PST
Created attachment 352075 [details] [diff] [review]
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 User image David Baron :dbaron: ⌚️UTC-8 2008-12-26 09:01:22 PST
Comment on attachment 352075 [details] [diff] [review]
alternative fix

r+sr=dbaron

This fixes another piece of bug 47710, doesn't it?
Comment 16 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-27 02:41:35 PST
Yes.
Comment 17 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-29 00:23:34 PST
Pushed abef302b61be
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-29 00:24:20 PST
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 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-01-01 11:09:43 PST
Comment on attachment 352075 [details] [diff] [review]
alternative fix

a191=beltzner
Comment 20 User image Brian Polidoro 2009-01-01 20:59:12 PST
*** Bug 471829 has been marked as a duplicate of this bug. ***
Comment 21 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-08 18:18:59 PST
Pushed 3194fd024edc to 1.9.1
Comment 22 User image philippe (part-time) 2009-04-06 16:33:37 PDT
*** Bug 487052 has been marked as a duplicate of this bug. ***
Comment 23 User image Aakash Desai [:aakashd] 2009-05-06 11:19:57 PDT
Is this a Windows specific bug?
Comment 24 User image David Baron :dbaron: ⌚️UTC-8 2009-05-06 11:24:49 PDT
No.
Comment 25 User image Aakash Desai [:aakashd] 2009-05-06 11:34:57 PDT
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

Note You need to log in before you can comment on or make changes to this bug.