Botond Ballo [:botond] Assignee
	Description • 10 years ago

STR:
  1. Load a webpage that can be zoomed out, such as [1], in the B2G browser.
  2. Scroll down a bit, so that the vertical scroll thumb's position
     is not at the top. The more you scroll down the more noticeable
     this bug is.
  3. Zoom out. (Zooming in also exhibits the problem, but it's less
     noticeable.)

Expected results:
  As you're zooming out, at every frame the scroll thumb's position
  reflects the visible (async) scroll offset of the page. When you
  lift your fingers and the page is repainted at the target zoom level,
  the painted scroll thumb position is the same (or very close to)
  that of the last compositor frame.

Actual results:
  As you zoom out, the scroll thumb continues to move *down* in spite
  of the scroll offset getting smaller. (In fact, it can even go off-
  screen!) When the repaint arrives, it jumps up to its correct position. 
  The jump can be quite noticeable.

The symptoms suggest that the adjustment of the scrollbar's transform to account for the (transient) async transform, which happens here [2], is incorrect.

Note that the size of the thumb tracks the zoom correctly, suggesting that only the translation applied at [2] is incorrect; the scale is fine.

[1] https://people.mozilla.org/~bballo/grid.html
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=6ddd4cc9a274#713

	Botond Ballo [:botond] Assignee
	Comment 1 • 10 years ago

The problem here is that the transform we compute is applied relative to the origin of the entire scrollbar, rather than relative to the origin of the scroll thumb.

For example, when we're zooming out and accordingly scale the scroll thumb up, in addition to the scroll thumb itself, the empty space above the scroll thumb is also scaled up, pushing the scroll thumb down.

To correct for this, we need to calculate the amount of the undesired translation, and negate it. I played around with this for a while, with not much luck so far. I'll have a fresh look next week.

	Botond Ballo [:botond] Assignee
	Comment 2 • 10 years ago

This is what I've tried. The correction seems to be in the right direction, but has the wrong magnitude, probably because I'm calculating it in the wrong coordinate system.

	Botond Ballo [:botond] Assignee
	Comment 3 • 10 years ago

After much further experimentation, the correct scale factor to multiply thumbOffsetDelta by is:

  metrics.GetZoom() * <async zoom> * <pres shell resolution>

(Note that metrics.GetZoom() doesn't include the async zoom yet, so that's not being counted twice, but the pres shell resolution is.)

With this change, I still see a tiny amount of jumping, probably due to the calculation here being just an approximation of how Layout actually calculates the scroll thumb's offset, but it's a huge improvement.

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

Comment on attachment 8515384 [details] [diff] [review]
Fix

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

Nice, thanks! I think the second component of the translation (the one that results from the scale origin) could be replaced by an appropriate call to Matrix4x4::ChangeBasis but I'm not sure if that would result in cleaner code. You'd have to create a temp matrix to hold the scale, call ChangeBasis on it, and then combine with the regular scrollbar transform which might be icky. Up to you if you want to try that; either way is fine by me.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +717,5 @@
> +
> +    // The scroll thumb needs to be translated in opposite direction of the
> +    // async scroll. This is because scrolling down, which translates the layer
> +    // content up, should result in moving the scroll thumb down.
> +    // The amount of t he translation should be such that the ratio of the

s/t he/the/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 10 years ago

Ok to land this?

	Botond Ballo [:botond] Assignee
	Comment 7 • 10 years ago

The patch is sitting on top of my bug 1055741 patch locally. I've been hoping bug 1076163 would be reviewed soon-ish, so I can land it, and bug 1055741 (which depends on it), and then things that depend on bug 1055741. However, if we want this bug more urgently, I could rebase it to not depend on bug 1055741.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 10 years ago

No that's ok, it can wait. I just wanted to check you hadn't forgotten about it :)

	Botond Ballo [:botond] Assignee
	Comment 9 • 10 years ago

Now that bug 1055741 has landed, I had another look at this patch, and realized that it also depends on bug 1076241. I'm going to wait for that to land too if no one objects.

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

(In reply to Botond Ballo [:botond] from comment #9)
> Now that bug 1055741 has landed, I had another look at this patch, and
> realized that it also depends on bug 1076241. I'm going to wait for that to
> land too if no one objects.

Now that bug 1076241 is landed, is this good to go as well?

	Botond Ballo [:botond] Assignee
	Comment 11 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Now that bug 1076241 is landed, is this good to go as well?

Yup, I'll land it tomorrow. Just wanted to make sure bug 1076241 is sticking.

	Botond Ballo [:botond] Assignee
	Comment 12 • 10 years ago
landing

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> Nice, thanks! I think the second component of the translation (the one that
> results from the scale origin) could be replaced by an appropriate call to
> Matrix4x4::ChangeBasis but I'm not sure if that would result in cleaner
> code. You'd have to create a temp matrix to hold the scale, call ChangeBasis
> on it, and then combine with the regular scrollbar transform which might be
> icky. Up to you if you want to try that; either way is fine by me.

Decided to leave things as is, but I did add a comment to note that conceptually we're doing a change of basis, in case that helps a reader understand things better.

> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +717,5 @@
> > +
> > +    // The scroll thumb needs to be translated in opposite direction of the
> > +    // async scroll. This is because scrolling down, which translates the layer
> > +    // content up, should result in moving the scroll thumb down.
> > +    // The amount of t he translation should be such that the ratio of the
> 
> s/t he/the/

Fixed.

https://hg.mozilla.org/integration/b2g-inbound/rev/15a1d3459835

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

https://hg.mozilla.org/mozilla-central/rev/15a1d3459835