Bug 13632 - Overflow scrolling doesn't account for bottom padding
: Overflow scrolling doesn't account for bottom padding
Status: RESOLVED FIXED
: WebKit
CSS
: 523.x (Safari 3)
: Macintosh Mac OS X 10.5
: P2 Major
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2007-05-08 21:52 PST by
Modified: 2009-03-16 14:34 PST (History)


Attachments
Test case. (212 bytes, text/html)
2007-05-08 21:52 PST, Timothy Hatcher
no flags Details
Patch (3.48 KB, patch)
2009-03-12 13:31 PST, Dave Hyatt
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
Patch (4.06 KB, patch)
2009-03-12 13:33 PST, Dave Hyatt
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch (3.91 KB, patch)
2009-03-12 15:49 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2009-03-13 12:10 PST, Dave Hyatt
eric: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2007-05-08 21:52:20 PST
An overflow area that has padding will only scroll to the bottom of the content box, not the bottom of the padding box.

See the attached test-case.
------- Comment #1 From 2007-05-08 21:52:41 PST -------
Created an attachment (id=14422) [details]
Test case.
------- Comment #2 From 2007-05-08 21:53:18 PST -------
This is visible in a Web Inspector change I am working on.
------- Comment #3 From 2007-05-08 22:02:27 PST -------
Yeah, this is a known bug.  I think it's filed already.  We need to clean up lowest/rightmost/leftmost position to be smarter about using the layer tree instead of groveling around the whole render tree.
------- Comment #4 From 2007-05-08 22:02:55 PST -------
Note that the bottom margin of a child element is also potentially ignored.
------- Comment #5 From 2007-06-23 22:39:13 PST -------
*** Bug 14342 has been marked as a duplicate of this bug. ***
------- Comment #6 From 2009-03-12 13:31:14 PST -------
Created an attachment (id=28545) [details]
Patch

Many test results change (for both horizontal and vertical cases).  I verified that all of the tests are progressing.  Especially nice are single line text fields, which now show their right padding (resulting in a more pleasing right edge when you autoscroll one while select-dragging).
------- Comment #7 From 2009-03-12 13:32:45 PST -------
(From update of attachment 28545 [details])
Oops. Forgot to remove some code in rightmostPosition.
------- Comment #8 From 2009-03-12 13:33:10 PST -------
Created an attachment (id=28546) [details]
Patch
------- Comment #9 From 2009-03-12 15:49:14 PST -------
Created an attachment (id=28561) [details]
Patch
------- Comment #10 From 2009-03-12 15:54:02 PST -------
(From update of attachment 28546 [details])
I'd like to see "bottom" and "right" renamed to lowestPosition and rightmostPosition to match the function names.  But I'm not holding my breath (or holding you from committing), given that you're trying to make this patch as small as possible.

All of your lp and rp variables I would have given clearer names:

int lp = lastLineBox()->y() + lastLineBox()->height();
int bottomOfLastLineBox = ...


I'm surprised this isn't already a method on RenderBox:
+            RenderBox* currBox = lastChildBox();
+            while (currBox && currBox->isFloatingOrPositioned())
+                currBox = currBox->previousSiblingBox();

RenderBox* lastNormalFlowChildBox();

the collapsedMarginBottom code is wrong (as discussed on IRC).

The paddingBottom() addition here
bottom = max(bottom, lp + paddingBottom());
, shoudl just move above into this line:
int lp = currBox->y() + currBox->height();
I guess if you're going to rename "int lp" to "int bottomOfLastLinebox" then the padding addition makes sense in the max line instead.

The if here is not needed:
if (firstLineBox()) {
for (InlineRunBox* currBox = firstLineBox(); currBox; currBox = currBox->nextLineBox()) {

the for already has the if implicit in it.

The caret hack seems like it should be abstracted into some other place.  This can't be the only place that wants paddingBoxWidthPlusCaret.  Is the caret always 1px?


Happy to review the next round.
------- Comment #11 From 2009-03-12 15:54:32 PST -------
(From update of attachment 28561 [details])
We got out of sync here. :)  Gonna let you read my other comments first.
------- Comment #12 From 2009-03-13 12:10:30 PST -------
Created an attachment (id=28589) [details]
Patch
------- Comment #13 From 2009-03-13 12:12:13 PST -------
(From update of attachment 28589 [details])
Looks great!  Thank you so much for the cleanup!
------- Comment #14 From 2009-03-16 14:34:46 PST -------
Fixed in r41742.