Bug 13632 - Overflow scrolling doesn't account for bottom padding
Summary: Overflow scrolling doesn't account for bottom padding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.5
: P2 Major
Assignee: Dave Hyatt
URL:
Keywords:
: 14342 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-05-08 21:52 PDT by Timothy Hatcher
Modified: 2009-03-16 14:34 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2007-05-08 21:52:20 PDT
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 Timothy Hatcher 2007-05-08 21:52:41 PDT
Created attachment 14422 [details]
Test case.
Comment 2 Timothy Hatcher 2007-05-08 21:53:18 PDT
This is visible in a Web Inspector change I am working on.
Comment 3 Dave Hyatt 2007-05-08 22:02:27 PDT
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 Dave Hyatt 2007-05-08 22:02:55 PDT
Note that the bottom margin of a child element is also potentially ignored.
Comment 5 Gérard Talbot 2007-06-23 22:39:13 PDT
*** Bug 14342 has been marked as a duplicate of this bug. ***
Comment 6 Dave Hyatt 2009-03-12 13:31:14 PDT
Created attachment 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 Dave Hyatt 2009-03-12 13:32:45 PDT
Comment on attachment 28545 [details]
Patch

Oops. Forgot to remove some code in rightmostPosition.
Comment 8 Dave Hyatt 2009-03-12 13:33:10 PDT
Created attachment 28546 [details]
Patch
Comment 9 Dave Hyatt 2009-03-12 15:49:14 PDT
Created attachment 28561 [details]
Patch
Comment 10 Eric Seidel (no email) 2009-03-12 15:54:02 PDT
Comment on attachment 28546 [details]
Patch

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 Eric Seidel (no email) 2009-03-12 15:54:32 PDT
Comment on attachment 28561 [details]
Patch

We got out of sync here. :)  Gonna let you read my other comments first.
Comment 12 Dave Hyatt 2009-03-13 12:10:30 PDT
Created attachment 28589 [details]
Patch
Comment 13 Eric Seidel (no email) 2009-03-13 12:12:13 PDT
Comment on attachment 28589 [details]
Patch

Looks great!  Thank you so much for the cleanup!
Comment 14 Dave Hyatt 2009-03-16 14:34:46 PDT
Fixed in r41742.