Bug 1066888 follow-up: the overscroll effect seems at the wrong place at the end of a scroll

VERIFIED FIXED in mozilla36

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: botond, NeedInfo)

Tracking

unspecified
mozilla36
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
There is one thing that does not feel right: the overscroll effect at one edge is different whether a scroll is happening because of inertia or because the user tries to scroll with the finger.

I think the right bouncing behavior is when the user scrolls with the finger. The effect when a scroll is ending with inertia should be the same.

Please tell me if you'd like a video to better explain myself :)
Blocks: 1066888
No longer depends on: 1066888
Gordon should triage here and make a call.
blocking-b2g: --- → 2.2+
Duplicate of this bug: 1097196
Duplicate of this bug: 1097144
Assignee: nobody → botond
Assignee

Comment 4

5 years ago
Copying the STR from bug 1097144, as they are more detailed:

Repro Steps:
1) Update a Flame device to BuildID: 20141111040202.
2) Open Calendar app, and select "Week" or "Day".
3) Scroll to the top or bottom of the screen.
  
Actual:
Black area is seen during overscoll animation.
  
Expected: 
No black area is seen during overscroll animation.
Assignee

Comment 5

5 years ago
I do see the black area even while overscrolling during panning, but very briefly; it seems to disappear on the first repaint. The reason it doesn't disappear when overscrolling during a fling is probably that we don't repaint in that case.

I'm surprised that the previous (pre - bug 1066888) stretch effect didn't have this problem; I wonder what's different about it.
Assignee

Comment 6

5 years ago
I tried adding some repaint requests before and during the overscroll animation, but that didn't help.
I don't think this is because of repaint requests. The bounce happens on the wrong end of the screen.
Assignee

Comment 8

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> I don't think this is because of repaint requests. The bounce happens on the
> wrong end of the screen.

Huh! You're right, I didn't notice that.
Assignee

Comment 9

5 years ago
Here's what's happening:

  - APZC::GetOverscrollTransform() figured out at which end of the composition bounds 
    we're overscrolled by looking at the combination of mOverscroll and mInUnderscroll.

  - mInUnderscroll is set in Axis::SampleOverscrollAnimation() by detecting sign changes
    to mOverscroll.

  - If the velocity coming in to the overscroll animation is sufficiently low that on
    the first sample, the spring is already making the content go in the opposite
    direction, mOverscroll never gets a chance to take on a value in the direction of
    overscroll ==> a sign change is not detected ==> mInUnderscroll is set
    incorrectly ==> we interpret the overscroll as being at the wrong end.

Patch coming up.
Awesome. Per Milan's comment, I do think this is a bug (in fact, accidentally filed dupe 1066888, which Kats found and folded into this bug).
Assignee

Comment 11

5 years ago
Posted patch Fix (obsolete) — Splinter Review
with a side of hack
Attachment #8523231 - Flags: review?(bugmail.mozilla)
Comment on attachment 8523231 [details] [diff] [review]
Fix

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

::: gfx/layers/apz/src/Axis.cpp
@@ +198,5 @@
> +  // changes in mOverscroll. If mOverscroll starts off as zero, and what would
> +  // have been a sign change already occurs on the first sample, we fail to
> +  // detect it, and set mInUnderscroll incorrectly; to avoid this, we set
> +  // mOverscroll to a small amount in the direction of the initial velocity.
> +  if (mOverscroll == 0) {

It's not clear to me when mOverscroll would be non-zero here. If we're starting an overscroll animation shouldn't the overscroll be zero at this point?

@@ +202,5 @@
> +  if (mOverscroll == 0) {
> +    if (aVelocity > 0) {
> +      mOverscroll = FLT_EPSILON;
> +    } else {
> +      mOverscroll = -FLT_EPSILON;

This seems like a roundabout way of controlling the initial value of mInUnderscroll. Instead can we just set mInUnderscroll directly to the desired value here?
Assignee

Comment 13

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> ::: gfx/layers/apz/src/Axis.cpp
> @@ +198,5 @@
> > +  // changes in mOverscroll. If mOverscroll starts off as zero, and what would
> > +  // have been a sign change already occurs on the first sample, we fail to
> > +  // detect it, and set mInUnderscroll incorrectly; to avoid this, we set
> > +  // mOverscroll to a small amount in the direction of the initial velocity.
> > +  if (mOverscroll == 0) {
> 
> It's not clear to me when mOverscroll would be non-zero here. If we're
> starting an overscroll animation shouldn't the overscroll be zero at this
> point?

No, an overscroll animation can be started after we've already panned into overscroll when we release the finger.

> @@ +202,5 @@
> > +  if (mOverscroll == 0) {
> > +    if (aVelocity > 0) {
> > +      mOverscroll = FLT_EPSILON;
> > +    } else {
> > +      mOverscroll = -FLT_EPSILON;
> 
> This seems like a roundabout way of controlling the initial value of
> mInUnderscroll. Instead can we just set mInUnderscroll directly to the
> desired value here?

The desired value of mInUnderscroll here is |false|. It should already be at that value - I'll add an assert to that effect.
Assignee

Comment 14

5 years ago
After talking to Kats on IRC, I had a more detailed look at what was going on, and realized that my assessment in comment 9 was incorrect.

I *thought* that we could have an undetected sign change on the first sample of an overscroll animation, but that actually can't happen. Since the spring force is proportional to the amount of the overscroll coming into the animation, if that amount is zero (which is the only situation where we wouldn't detect a sign change), the spring force is zero so there wouldn't be a sign change.

The actual problem was that OverscrollAnimation::Sample() was being called with a negative time duration, so even though there was no sign change in the velocity, there was a sign change in the overscroll amount.

I noticed FlingAnimation::Sample() and SmoothScrollAnimation::Sample() already handle the case where the time duration is negative; rather than adding it to OverscrollAnimation::Sample() as well, and risking running into this again when we add a new animation, I just moved up the check to AsyncPanZoomAnimation::Sample().
Attachment #8523231 - Attachment is obsolete: true
Attachment #8523231 - Flags: review?(bugmail.mozilla)
Attachment #8523982 - Flags: review?(bugmail.mozilla)
Comment on attachment 8523982 [details] [diff] [review]
Ignore calls to AsyncPanZoomAnimation::Sample() with a negative time duration

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

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +1127,5 @@
> +    // In some situations, particularly when handoff is involved, it's possible
> +    // for |aDelta| to be negative on the first call to sample. Ignore such a
> +    // sample here, to avoid each derived class having to deal with this case.
> +    if (aDelta.ToMilliseconds() <= 0) {
> +      return true;

I feel like it would be cleaner to just put this in APZC::UpdateAnimation since that's the only thing that calls AsyncPanZoomAnimation::Sample. This is fine too, I don't have strong objections.
Attachment #8523982 - Flags: review?(bugmail.mozilla) → review+
Assignee

Comment 16

5 years ago
landing
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> I feel like it would be cleaner to just put this in APZC::UpdateAnimation
> since that's the only thing that calls AsyncPanZoomAnimation::Sample. This
> is fine too, I don't have strong objections.

I prefer this way as it covers potential future changes where we add other call sites of Sample().

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc96a763c3f
https://hg.mozilla.org/mozilla-central/rev/5cc96a763c3f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
This issue has been resolved fixed on the Flame 2.2 KK Shallow Flash (319mb)

Result: Black area during scrolling no longer occurs when scrolling through the Week or Day sections of calendar.

Flame 2.2
Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.