Bug 13632 – Overflow scrolling doesn't account for bottom padding

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 14422 [details]
Test case.

[Timothy Hatcher]

This is visible in a Web Inspector change I am working on.

[Dave Hyatt]

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.

[Dave Hyatt]

Note that the bottom margin of a child element is also potentially ignored.

[Gérard Talbot (no longer involved)]

*** Bug 14342 has been marked as a duplicate of this bug. ***

[Dave Hyatt]

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).

[Dave Hyatt]

Comment on attachment 28545 [details]
Patch

Oops. Forgot to remove some code in rightmostPosition.

[Dave Hyatt]

Created attachment 28546 [details]
Patch

[Dave Hyatt]

Created attachment 28561 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 28561 [details]
Patch

We got out of sync here. :)  Gonna let you read my other comments first.

[Dave Hyatt]

Created attachment 28589 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 28589 [details]
Patch

Looks great!  Thank you so much for the cleanup!

[Dave Hyatt]

Fixed in r41742.