Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 9 years ago

I tracked this down as a regrssion from bug 1152469. STR:

1) On a master B2G build, load http://people.mozilla.com/~kgupta/grid.html
2) Zoom in
3) Pan around

Note that at step 2 once the zoom is done the scrollbar jumps significantly. Also note at step 3 how the scrollbar janks on repaints; the translation applied during async scrolling is too much and so the repaint causes it to move backwards.

With the patch from bug 1152469 backed out, these issues don't happen. (There is still a small jump once the zoom finishes, but it's very minor compared to what happens with bug 1152469 applied).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 9 years ago

Looks like in bug 1152469 the term "compositedHeight / scrollableHeight" was replaced with "aScrollbar->GetScrollbarThumbRatio()". The former term was equivalent to "(metrics.mCompositionBounds / effectiveZoom).height / metrics.GetScrollableRect().height" which includes the resolution and async zoom bits. The ScrollbarThumbRatio however does not. This patch seems to fix the behaviour for me locally.

However, I'm also wondering about the code at [1] for example. It too contains a "compositedWidth/scrollableWidth" term (inverted) - should that have been replaced by the thumb ratio as well?

[1] https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031#l3.43

	Botond Ballo [:botond]
	Comment 2 • 9 years ago

Comment on attachment 8598211 [details] [diff] [review]
Patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +728,5 @@
>  
>      // The scrollbar thumb ratio is in AppUnits.
> +    const LayoutDeviceToParentLayerScale nonLayoutScale = effectiveZoom /
> +        metrics.GetDevPixelsPerCSSPixel();
> +    const float ratio = aScrollbar->GetScrollbarThumbRatio() / nonLayoutScale.scale;

It took a lot of staring at this and fiddling around with numbers to figure out why this is necessary, but I think I finally figured it out:

The units of Layer::GetScrollbarThumbRatio() (and nsSliderFrame::GetThumbRatio(), which is where it comes from) is "CSS pixels of the scroll frame's parent's space per CSS pixels of the scroll frame's space" (or equivalently, app units of ... per app units of ...), in spite of the comment in nsSliderFrame::GetThumbRatio() which claims it returns a "true unitless ratio".

Before this ratio can be treated as a true unitless ratio, therefore, it must be divided by the conversion factor from the parent space into the scroll frame's space, which is precisely what you're calling nonLayoutScale here.

r+ with the following comments added:

  - Comments for Layer::GetScrolbaerThumbRatio() and nsSliderFrame::GetThumbRatio()
    which state the correct units these are in.

  - A comment here, replacing "The scrollbar thumb ratio is in AppUnits", stating
    what we're doing with the ratio here.

Thanks!

	Botond Ballo [:botond]
	Comment 3 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> However, I'm also wondering about the code at [1] for example. It too
> contains a "compositedWidth/scrollableWidth" term (inverted) - should that
> have been replaced by the thumb ratio as well?
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031#l3.43

Oh and yes, good catch! Let's change these as well to use the (adjusted) ratio.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

Updated with comments, carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 9 years ago

Fairly trivial patch and it seems to work as intended.

	Botond Ballo [:botond]
	Comment 6 • 9 years ago

Comment on attachment 8598674 [details] [diff] [review]
Part 2 - Use ratio more widely

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

Thanks!

	Pulsebot
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1ed8e07f86
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27a83ace0ce

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

https://hg.mozilla.org/mozilla-central/rev/bf1ed8e07f86
https://hg.mozilla.org/mozilla-central/rev/f27a83ace0ce