1103276 - Overscrolling does not factor both scrollbar dimensions

Status: RESOLVED INVALID

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: overscroll.PNG

When using APZ with classic scrollbars, if both horizontal and vertical scrollbars are drawn, overscrolling is off by 17 pixels (the height/width of a vertical/horizontal scrollbar). Screenshot attached. It looks like DisplacementWillOverscrollAmount() doesn't get the right scrollable rect.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1103276.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8528615 [details] [diff] [review]
bug1103276.patch

Review of attachment 8528615 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +189,5 @@
> +    // If we're using non-overlay scrollbars, we may have computed more
> +    // accurate composition bounds. Update the scroll position clamp now to
> +    // reflect that.
> +    CSSSize scrollPort = aMetrics.CalculateCompositedSizeInCssPixels();
> +    utils->SetScrollPositionClampingScrollPortSize(scrollPort.width, scrollPort.height);

Wrap this code in a |if (!LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars))| condition for clarity and to avoid unexpected side-effects on other platforms.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Turned out to be a regression from bug 1076192.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Will this patch still be needed for iframes?

Comment 5

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Will this patch still be needed for iframes?

I don't think so - I think the scroll-position-clamping-scroll-port-size is only used for the root-content-document.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

15:50:57 kats: botond: why do you say the SPCSPS is only used for the root content document? any presshell can have one
15:51:17 botond: kats: well, it's only _set_ for the RCD
15:51:33 kats: botond: right now, yes. that patch was intended to change that behaviour
15:52:16 kats: although now that i think about it more it makes sense to not have to do it for subframes
15:53:12 kats: botond: oh i see what you're saying. it's only set incorrectly for the RCD in tabchild
15:53:22 kats: the rest of them will have a correct-by-default value already
15:53:28 kats: s/value/scroll range/
15:53:44 kats: that makes sense
15:54:51 botond: kats: i thought i saw a comment somewhere that indicated it was specific to the viewport frame, but i can't find it at the moment
15:55:01 botond: kats: but, your reasoning makes sense as well