If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Scrolling with URL bar visible causes choppiness/shaking

RESOLVED FIXED in Firefox 22

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: cwiiis)

Tracking

unspecified
Firefox 22
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
Created attachment 726101 [details] [diff] [review]
Fix PanZoomController and dynamic toolbar animations fighting

See comment #2 for patch details.
Attachment #726101 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 5

5 years ago
Created attachment 726103 [details] [diff] [review]
Fix scroll jank with dynamic toolbar

See comment #3 for details.
Attachment #726103 - Flags: review?(bugmail.mozilla)
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.
Attachment #726101 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #726103 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 9

5 years ago
Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e581d54aa570
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee24b51a1a9
https://hg.mozilla.org/mozilla-central/rev/e581d54aa570
https://hg.mozilla.org/mozilla-central/rev/bee24b51a1a9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.