850889 - Scrolling with URL bar visible causes choppiness/shaking

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Go to any page that scrolls
2) Scroll to the bottom of the page
3) Scroll up slightly to make the URL bar appear
4) Scroll down with medium/medium-low finger acceleration

This causes the screen to jitter several times.

Comment 1

[Chris Lord [:cwiiis]]

Taking this. Will fix - this is caused by the clamp-on-margin-change code, it should be pretty easy to fix this.

I'm also going to conflate the issue that it forces a gecko redraw on every margin change and fix it so that it only forces it for the last margin change in an animation - this causes what could be considered to be jitter, especially on older devices.

Comment 2

[Chris Lord [:cwiiis]]

Bug 852072 is stopping me from attaching a patch, so let's do a lo-fi review...

---

Patch fix-dynamic-toolbar-clamp-on-set: http://www.pastebin.mozilla.org/2225249

So the major part of this bug was caused by setViewportMetrics overriding the fixed layer margins and JavaPanZoomController pre-calculating the animate-to viewport, causing conflict between setFixedLayerMargins and animated bounce-back/zoom.

The second part was caused by the 'clamp-on-margin-set' property - This would clamp the viewport whenever the fixed layer margins were changed, but we really only want to clamp to the changed margin, and only when it decreases.

r?kats

Comment 3

[Chris Lord [:cwiiis]]

Patch fix-dynamic-toolbar-jank: http://www.pastebin.mozilla.org/2225270

This reduces the amount of messaging we do on fixed layer margin changes and also stops forcing the draw on every change (which will be the major cause of jank).

r?kats

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Fix PanZoomController and dynamic toolbar animations fighting

See comment #2 for patch details.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Fix scroll jank with dynamic toolbar

See comment #3 for details.

Comment 6

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

Comment on attachment 726101 [details] [diff] [review]
Fix PanZoomController and dynamic toolbar animations fighting

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

I'm not entirely happy with how the new code references oldMetrics.something in pretty much every line; it seems like it should be a helper method on ViewportMetrics instead. But I can't think of a way to organize the code so that it's actually better/cleaner/more readable.

Comment 7

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

Comment on attachment 726103 [details] [diff] [review]
Fix scroll jank with dynamic toolbar

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

::: mobile/android/base/BrowserApp.java
@@ +624,5 @@
> +            // bar aren't animating.
> +            PointF velocityVector = mLayerView.getPanZoomController().getVelocityVector();
> +            if ((aVisibleHeight == 0 || aVisibleHeight == aHeight) &&
> +                velocityVector.x == 0 && velocityVector.y == 0) {
> +                mLayerView.getLayerClient().forceRedraw();

I think the velocityVector checks here should be fuzzy, (a la FloatUtils.fuzzyEquals). Also how often does this code get hit? The previous version had an early-exit condition in GeckoLayerClient.setFixedLayerMargins if the margins hadn't changed, and that doesn't get hit any more here. So if this code runs with the same margins over and over we'll still do force redraws.

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Comment on attachment 726103 [details] [diff] [review]
> Fix scroll jank with dynamic toolbar
> 
> Review of attachment 726103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +624,5 @@
> > +            // bar aren't animating.
> > +            PointF velocityVector = mLayerView.getPanZoomController().getVelocityVector();
> > +            if ((aVisibleHeight == 0 || aVisibleHeight == aHeight) &&
> > +                velocityVector.x == 0 && velocityVector.y == 0) {
> > +                mLayerView.getLayerClient().forceRedraw();
> 
> I think the velocityVector checks here should be fuzzy, (a la
> FloatUtils.fuzzyEquals). Also how often does this code get hit? The previous
> version had an early-exit condition in GeckoLayerClient.setFixedLayerMargins
> if the margins hadn't changed, and that doesn't get hit any more here. So if
> this code runs with the same margins over and over we'll still do force
> redraws.

Will make the fuzzyEquals change. We don't actually call this function continuously with the same values, so it's overkill to bother checking - rather than complicate this code, I'll remove the check in GeckoLayerClient. Confirmed IRL.

Comment 9

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e581d54aa570
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee24b51a1a9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e581d54aa570
https://hg.mozilla.org/mozilla-central/rev/bee24b51a1a9