Botond Ballo [:botond] Reporter
	Description • 9 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 1 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 9 years ago

Patch implementing the proposed solution.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 9 years ago

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?

	Markus Stange [:mstange]
	Comment 4 • 9 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 5 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 9 years ago

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

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 7 • 9 years ago

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