Closed Bug 907123 Opened 11 years ago Closed 10 years ago

Flicks don't speed up scrolling with apz (need to implement accelerated scrolling)

Categories

(Core :: Panning and Zooming, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: jimm, Assigned: kats)

References

Details

(Whiteboard: [feature] p=0,[ucid:graphics12])

Attachments

(2 files, 1 obsolete file)

STR:

1) open a page that has a lot of room to scroll
2) scroll to the bottom of the page
3) flick up two or three times to get to the top pf the page

result: each time your finger initially touches the screen scrolling will slow. The additional flick motion will continue scrolling at the same rate. 

expected: scroll speed should accelerate with each flick, and there shouldn't be a pause in scrolling when initiating a flick.
Blocks: 915723
No longer blocks: metro-apzc
I'm not really able to reproduce this problem, but it sounds like there are two parts to it. One is that the scroll speed slows when you put your finger down. This sounds like expected behaviour to me, because if a fling is in progress and you put and keep your finger down, the fling should abort and the page should stop scrolling. (i.e. you're effectively holding the page in place with your finger). Similar behaviour can be seen on Fennec and B2G.

The other part of this is that doing multiple successive flings should cause acceleration (i.e. the page should move faster than your finger). This is currently not implemented, so we can leave this bug to track that issue if you like. It is something that I think would be useful in scrolling larger distances across all platforms. We've had an open bug on it for a while in Fennec (bug 708765) with various different options discussed there.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> I'm not really able to reproduce this problem, but it sounds like there are
> two parts to it. One is that the scroll speed slows when you put your finger
> down. This sounds like expected behaviour to me, because if a fling is in
> progress and you put and keep your finger down, the fling should abort and
> the page should stop scrolling. (i.e. you're effectively holding the page in
> place with your finger). Similar behaviour can be seen on Fennec and B2G.

The odd thing is we don't seem to be able to detect another fling vs. a touch so if you fling, then attempt to fling again, the page stops the second time around and then starts another fling.

> The other part of this is that doing multiple successive flings should cause
> acceleration (i.e. the page should move faster than your finger). This is
> currently not implemented, so we can leave this bug to track that issue if
> you like. It is something that I think would be useful in scrolling larger
> distances across all platforms. We've had an open bug on it for a while in
> Fennec (bug 708765) with various different options discussed there.

Sounds right.
(In reply to Jim Mathies [:jimm] from comment #2)
> The odd thing is we don't seem to be able to detect another fling vs. a
> touch so if you fling, then attempt to fling again, the page stops the
> second time around and then starts another fling.

Right, because every fling starts with a touch down. If a fling is in progress, and the user puts their finger down, we don't know what they're going to be doing. They could be initiating a second fling, or they might just leave their finger there and want to stop the panning. Without knowing which of the two (or other possible) actions the user is going to take, we can't decide if we need to stop the pan or not. We assume that the user is *not* going to do another fling because that seems like a reasonable assumption, and also involves less state management.

If we implement accelerated scrolling properly, then this should be less noticeable, I think. That is, if you do a quick double-fling it will scroll really fast and you won't have time to really notice that it stopped the first fling when you put your finger down for the second one. On the other hand, if you do a really slow second fling, you will notice that the page stopped scrolling but presumably that's what you want because you did a slow second fling.

I'm going to make this bug about accelerated scrolling and move it to the APZC component. The last paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=708765#c45 has a simple approach that I think should work for implementing this.
Component: Pan and Zoom → Panning and Zooming
Product: Firefox for Metro → Core
Summary: Flicks don't speed up scrolling with apz → Flicks don't speed up scrolling with apz (need to implement accelerated scrolling)
Whiteboard: [release28]
No longer blocks: 915723
Blocks: metro-apzc
Whiteboard: [release28]
Blocks: metrobacklog
No longer blocks: metro-apzc
Whiteboard: [feature]
Whiteboard: [feature] → [feature] p=0
I'll start looking into this as my "waiting for a build" bug.
Assignee: nobody → bugmail.mozilla
Seems to work pretty well on my Hamachi.
Attachment #8411298 - Flags: review?(botond)
Attachment #8411294 - Flags: review?(botond) → review+
Comment on attachment 8411298 [details] [diff] [review]
Part 2 - Accelerate consecutive flings

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +350,5 @@
> +    // actual touch event; the accelerated velocities are then handed off using the
> +    // normal HandOffFling codepath.
> +    if (aApplyAcceleration && (now - mApzc.mLastFlingTime).ToMilliseconds() < gfxPrefs::APZFlingAccelInterval()) {
> +      if (SameDirection(velocity.x, mApzc.mLastFlingVelocity.x)) {
> +        velocity.x += mApzc.mLastFlingVelocity.x;

Should the amount of the acceleration be configurable?

@@ +379,5 @@
>  
>  private:
> +  bool SameDirection(float aDir1, float aDir2)
> +  {
> +    return (aDir1 == 0.0f)

Should we be using FuzzyEquals here?
(In reply to Botond Ballo [:botond] from comment #8)
> > +        velocity.x += mApzc.mLastFlingVelocity.x;
> 
> Should the amount of the acceleration be configurable?

Yeah I was wondering about that. Should I do something like velocity.x = (velocity.x * PrefA) + (mApzc.mLastFlingVelocity.x * prefB)? Where prefA and prefB default to 1.0 to get the effect above?

> @@ +379,5 @@
> > +    return (aDir1 == 0.0f)
> 
> Should we be using FuzzyEquals here?

I was using == on purpose, because the IsNegative check will return true if the two values are on the same side of zero regardless of how small they are. The only case that falls through is when either value is *exactly* zero (or negative zero, which == 0.0f catches as well).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #8)
> > > +        velocity.x += mApzc.mLastFlingVelocity.x;
> > 
> > Should the amount of the acceleration be configurable?
> 
> Yeah I was wondering about that. Should I do something like velocity.x =
> (velocity.x * PrefA) + (mApzc.mLastFlingVelocity.x * prefB)? Where prefA and
> prefB default to 1.0 to get the effect above?

I would.  We can take the prefs out if the equation ends up being simplified after UX is done playing with it.
Updated to make the multiplier values preffed
Attachment #8411298 - Attachment is obsolete: true
Attachment #8411298 - Flags: review?(botond)
Attachment #8412922 - Flags: review?(botond)
Comment on attachment 8412922 [details] [diff] [review]
Part 2 - Accelerate consecutive flings

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +386,5 @@
>    virtual bool Sample(FrameMetrics& aFrameMetrics,
>                        const TimeDuration& aDelta);
>  
>  private:
> +  bool SameDirection(float aDir1, float aDir2)

I find the names 'aDir1' and 'aDir2' a bit misleading, as they are not directions, they are quantities (which lie _in_ a direction based on their sign). Maybe 'aQuantity1' and 'aQuantity2', or 'aVelocity1' and 'aVelocity2' if you prefer?

Also, SameDirection() and Accelerate() can both be made static.
Attachment #8412922 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #12)
> I find the names 'aDir1' and 'aDir2' a bit misleading, as they are not
> directions, they are quantities (which lie _in_ a direction based on their
> sign). Maybe 'aQuantity1' and 'aQuantity2', or 'aVelocity1' and 'aVelocity2'
> if you prefer?

Renamed to aVelocity[12]

> Also, SameDirection() and Accelerate() can both be made static.

Done.

Landed on inbound:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ab970e182f57
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/729fb5ddc1b9
Fixed and relanded; there was a debug-only assertion to ensure we didn't subtract a 0 timestamp, so I added a check for that.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6e068aa1987d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2edf6b019ee2
https://hg.mozilla.org/mozilla-central/rev/6e068aa1987d
https://hg.mozilla.org/mozilla-central/rev/2edf6b019ee2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
feature-b2g: --- → 2.0
Whiteboard: [feature] p=0 → [feature] p=0,[ucid:graphics12]
1. Add user story ID defined in Firefox OS product backlog Google sheet
2. Specify this is Firefox OS 2.0 feature by feature-b2g=2.0
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.