1152469 - Scroll thumb overlaps scrollbar arrow during async scroll

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Enable APZ on a platform that has non-overlay scrollbars,
     like Linux or Windows.
  2. Load a page that scrolls vertically.
  3. Do a quick wheel scroll ending at the top or bottom of
     the page.

Expected results:
  The scroll thumb is correctly animated during the async scroll.

Actual results:
  The scroll thumb can overlap the arrow at the end of the scrollbar
  during the async scroll.

Comment 1

[Botond Ballo [:botond]]

Issue was diagnosed and a fix suggested on IRC. Posting relevant subset of conversation for reference:

[14:11] <botond> dvander: yeah, i do see the issue where after scrolling fast, the scroll thumb overlaps the arrow
[14:12] <botond> dvander: and, i'm pretty sure i know why, too
[14:14] <botond> dvander: AsyncCompositionManager::ApplyAsyncTransformToScrollbarForContent does a back-of-the-envelope calculation to approximate how Layout would position the scroll thumb in response to a scroll: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#746
[14:15] <botond> dvander: that calculation was written with overlay scrollbars on b2g in mind, and thus doesn't take into account the space taken up by the arrows in non-overlay scrollbars
[14:16] <botond> dvander: accordingly, the position it calculates for the thumb for, say, a scroll offset of 0, is "at the top of the scrollbar", which on desktop means overlapping the arrow
[14:21] <botond> dvander: presumably fixing this will involve layout telling the compositor about scrollbar arrow sizes somehow
[14:48] <mstange> all we'd need is one float, that expresses the ratio between scroll position changes and thumb position changes
[14:52] <botond> mstange: yep, that'd work
[14:53] <mstange> botond: yay
[14:53] <mstange> botond: I think there's an mRatio field in nsSliderFrame that expresses exactly that

Comment 2

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

Attachment: fix

Patch implementing the proposed solution.

Comment 3

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

Comment on attachment 8590512 [details] [diff] [review]
fix

@botond, The ratio from layout is in AppUnits, which this patch converts to CSSPixels. But I think I actually have to convert this to ParentLayerCoords too?

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8590512 [details] [diff] [review]
fix

Looks good, but I'd do the conversion from app units to CSS pixels in nsSliderFrame::GetThumbRatio(). (I don't like saying that a ratio is "in app units", when it's really in "app units per css pixel", or more specifically in "thumb offset app units per scrolled css pixels". Doing the conversion earlier makes it a true unitless value ("css pixels per css pixels") and gets around that problem.)

At this point it might almost make sense to group the three scrollbar-related layer properties into a ScrollbarProperties struct. But let's not do that in thi bug, if at all.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8590512 [details] [diff] [review]
fix

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

(In reply to David Anderson [:dvander] from comment #3)
> @botond, The ratio from layout is in AppUnits, which this patch converts to
> CSSPixels. But I think I actually have to convert this to ParentLayerCoords
> too?

As Markus pointed out, the ratio is not "in app units", it's in "app units per CSS pixel", which you then convert to "CSS pixels per CSS pixel", which is unitless, so there's no need for any additional conversion between CSS and ParentLayer units.

(I agree that it would be nicer to store the ratio in "CSS pixels per CSS pixel" to begin with.)

::: gfx/layers/Layers.h
@@ +1717,5 @@
>    };
>    nsAutoPtr<StickyPositionData> mStickyPositionData;
>    FrameMetrics::ViewID mScrollbarTargetId;
>    ScrollDirection mScrollbarDirection;
> +  float mScrollbarThumbRatio; // Ratio of the thumb position to the scroll

Please initialize this to something (probably zero) in the constructor.

Comment 6

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2cb069b031

w/ comments addressed. Thanks for clarifying the mRatio units.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031