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)
Core
Layout
Tracking
()
VERIFIED
FIXED
People
(Reporter: rwhite, Assigned: roc)
References
Details
(Keywords: regression, testcase, verified1.9.1)
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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.
Comment 4•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 5•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → roc
Assignee | ||
Updated•16 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → wanted1.9.1+
Assignee | ||
Comment 7•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
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.
Assignee | ||
Comment 10•16 years ago
|
||
And in general computed heights (and min/max heights) propagated in nsHTMLReflowState::CalcQuirkContainingBlockHeight and nsHTMLReflowState::InitConstraints have to be adjusted for scrollbars.
Assignee | ||
Comment 11•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
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.
Assignee | ||
Comment 13•16 years ago
|
||
That patch fails a few reftests, by the way. The setting of the mVResize flag in ReflowScrolledFrame is suspect.
Assignee | ||
Comment 14•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Attachment #352075 -
Flags: superreview?(dbaron)
Attachment #352075 -
Flags: superreview?
Attachment #352075 -
Flags: review?(dbaron)
Attachment #352075 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Updated•16 years ago
|
Attachment #352075 -
Flags: superreview?(dbaron)
Attachment #352075 -
Flags: superreview+
Attachment #352075 -
Flags: review?(dbaron)
Attachment #352075 -
Flags: review+
Comment 15•16 years ago
|
||
Comment on attachment 352075 [details] [diff] [review]
alternative fix
r+sr=dbaron
This fixes another piece of bug 47710, doesn't it?
Updated•16 years ago
|
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 16•16 years ago
|
||
Yes.
Assignee | ||
Comment 17•16 years ago
|
||
Pushed abef302b61be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Comment 18•16 years ago
|
||
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 19•16 years ago
|
||
Comment on attachment 352075 [details] [diff] [review]
alternative fix
a191=beltzner
Attachment #352075 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 191 approval] → [needs 191 landing]
Assignee | ||
Comment 21•16 years ago
|
||
Pushed 3194fd024edc to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 23•16 years ago
|
||
Is this a Windows specific bug?
Comment 24•16 years ago
|
||
No.
Comment 25•16 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
OS: Windows XP → All
Hardware: x86 → All
You need to log in
before you can comment on or make changes to this bug.
Description
•