1085404 - Flywheel scrolling on Facebook (and other sites) causes regular pauses

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

If you engage fly-wheel scrolling on Facebook (and other sites, but it's especially noticeable on facebook), at the start of the fling gesture, the scrolling pauses rather than continuing or following the finger. Will attach a video to demonstrate.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Video demonstrating the issue

Comment 2

[Chris Lord [:cwiiis]]

The issue is especially noticeable if you watch the scroll indicator (it feels more noticeable than it looks in this short video).

Comment 3

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

Might be slow touch listeners. We sit on the events until content lets us know if they're doing a preventdefault or not.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Might be slow touch listeners. We sit on the events until content lets us
> know if they're doing a preventdefault or not.

We shouldn't stop scrolling entirely during this time though surely? Either the previous fling should continue or scrolling should follow the finger - as it is, it stops dead which feels very odd...

Comment 5

[Chris Lord [:cwiiis]]

To explain my reasoning for this;

We currently let content have first-dibs on what to do for a touch for X time (300ms?). During this time, we delay any processing until we hear back from content, then we do whatever we were going to do, or nothing if content handles the event.

I assume we want to stop flings when content handles an event, or it'd feel pretty weird, but I think we should do this *after* content responds - my reasoning is that at the moment, we delay all our native reactions to events *except* stopping flings, which we do immediately. This definitely feels odd, but I think logically it doesn't make as much sense either.

It's especially noticeable on flywheel scrolling I suppose because content is busy trying to keep up with rendering, so touch handlers take a lot longer to respond.

Comment 6

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #5)
> I assume we want to stop flings when content handles an event, or it'd feel
> pretty weird, but I think we should do this *after* content responds

Reading the relevant code, canceling previous animations *before* content responds appears to have been an intentional choice:

    // We want to cancel animations here as soon as possible (i.e. without waiting for
    // content responses) because a finger has gone down and we don't want to keep moving
    // the content under the finger. 

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#1024

Comment 7

[Botond Ballo [:botond]]

In fact, this was the explicit subject of bug 1039979.

Comment 8

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

It goes even further back; this behaviour was first added in Fennec in bug 749384.

Comment 9

[Chris Lord [:cwiiis]]

That's interesting - Why do we only apply this logic to stopping though? That makes little sense to me. If you can't scroll the page during that time, I don't see why it should stop... And it certainly makes flywheel scrolling feel weird/broken...

I wonder if we can selectively do this, or do it in a less jarring way? Perhaps rather than stopping the fling, it can make it decelerate at a faster rate? Or perhaps it can only stop it if the users' finger is actually held in position, rather than on just any arbitrary touch motion?

Comment 10

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

(In reply to Chris Lord [:cwiiis] from comment #9)
> I wonder if we can selectively do this, or do it in a less jarring way?
> Perhaps rather than stopping the fling, it can make it decelerate at a
> faster rate? Or perhaps it can only stop it if the users' finger is actually
> held in position, rather than on just any arbitrary touch motion?

This might be something to explore but it's not going to be very high on my priority list. There's a fundamental conflict here where making one action feel better will degrade the experience in the other. Tuning to find a sweet spot will take time.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: WIP: Apply friction instead of cancelling animation

What do you think of this as a method of doing this?

You're right in that it needs tuning - overscroll friction is too high in this case, it needs a separate pref. I just wanted to get something done quickly though, and this is already feels better than the current behaviour to me (of course, this is subjective and needs more testing and UX feedback).

Comment 12

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

Comment on attachment 8508004 [details] [diff] [review]
WIP: Apply friction instead of cancelling animation

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

As far as the overall approach goes I think it's a reasonable way to do it. I'm a little concerned that there might be edge cases where we set an APZC in high-friction mode and then don't unset it, but I can't think of any concrete scenario in which that might happen. And there's a bunch of naming and nits and so on that would need to be fixed, but since this is f? and not r? I'll skip those. If we can get UX to sign off on doing something like this I have no objections.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's do this as a part of the "scrolling physics" work (bug 1066688), including UX in the conversations, otherwise we're just bouncing around with partial solutions.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Apply friction instead of cancelling animation

I assume this won't pass review given your previous comments, but flagging it anyway.

I don't intend to check this in without ux approval, for the record.

Comment 15

[Chris Lord [:cwiiis]]

(In reply to Milan Sreckovic [:milan] from comment #13)
> Let's do this as a part of the "scrolling physics" work (bug 1066688),
> including UX in the conversations, otherwise we're just bouncing around with
> partial solutions.

I assume you meant bug 1066888 - this is quite a separate issue from overscroll, I'm not sure what lumping it in with that would help achieve? I won't flag this for UX yet as I don't want to add to anyone's work-load unnecessarily, but I think this can and should be handled separately from overscroll (as it's an unrelated effect).

To be clear, currently, as soon as you put a finger to the screen, we stop any fling. This is fine in the case that there are no touch handlers, as we immediately respond to the input. In the case that there are touch handlers, it means that any touch during a fling causes a jarring halt to the fling before any more touch events/flings are handled (unless content responds instantly, which is rare enough anyway, but practically impossible during flywheel scrolling).

This patch changes are initial touch response to adding friction to any ongoing fling, rather than stopping it altogether. This has the effect of immediate response to a touch without the jarring effect of a complete stop. I assume that this also improves rendering behaviour as the displayport won't change so erratically during flywheel scrolling.

Does this change your opinion on handling this as part of bug 1066888?

I find the value I've picked in the current patch works quite well on a Flame.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

You're right, the wrong bug number.  We were going back and forth whether we promote bug 1066888 to take care of all the physics, or make it block another bug.  We eventually settled on the later.  So, I really wanted to talk about bug 1042017 instead, and figure out what the dependency or overlap, if any, between this and that bug is.

Comment 17

[Mason Chang [Inactive] [:mchang]]

From talking with Gordon during the UX / GFX meetings, Gordon is concerned about the fling friction and overscroll. I too thought they were separate issues. Gordon's concern is that if we fling really really fast and get to the edge of the content, and reduce the friction, the overscroll effect will be too pronounced. He wanted us to tune both the overscroll effect along with the fling friction as one "scrolling" overhaul. Overall though, I agree that having separate friction for separate apps / not stopping in the middle of a fling are the way to go and that our current fling behavior needs some tuning.

Comment 18

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

Comment on attachment 8510302 [details] [diff] [review]
Apply friction instead of cancelling animation

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

This seems fine, just a couple of comments. Unsetting review flag because I don't want to r+ it without ux review, and because of the discussion in the above comments. I've added this to the list of things to discuss with gordon at the ux/gfx meeting tomorrow.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +308,5 @@
> +
> +  /**
> +   * Sets normal friction on any currently running animation.
> +   */
> +  void SetIsNotInHighFriction();

Collapse these two into a SetInHighFriction(bool) like you did in Axis

::: gfx/layers/apz/src/Axis.h
@@ +172,5 @@
> +   * Sets whether the extra friction should be applied to the movement of this
> +   * axis.
> +   */
> +  void SetInHighFriction(bool aInHighFriction) {
> +    mInHighFriction = aInHighFriction;

Move this into the .cpp

Comment 19

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

So, we discussed this with Gordon today and he had a counter-proposal which I think would work even better. Basically once we are flinging, and the user does a second fling (i.e. flywheel), we should just eat those touch events in the APZ and not dispatch it to content at all. That way we don't have to wait for content listeners to run and so we can kick off the flywheel much sooner (as soon as we determine the user is doing fling, as opposed to when content listeners are done running). Conceptually we would be treating the flywheel scrolling as an APZ gesture which is kind of the intent behind it to begin with.

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> So, we discussed this with Gordon today and he had a counter-proposal which
> I think would work even better. Basically once we are flinging, and the user
> does a second fling (i.e. flywheel), we should just eat those touch events
> in the APZ and not dispatch it to content at all.

Would this not violate the spec by depriving content of the chance to prevent-default or otherwise handle that particular touch-start event?

Comment 21

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

Technically yes but we already do this in the case of overscroll, for example. And conceptually a flywheel scroll is not very different from the user just doing a super duper hardcore fling just once. With this change the events sent to content in both of these cases would be the same (with the possible exception of the timestamps on the touch-move events). So I don't think fundamentally we're altering what content can or cannot do in this case.

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Don't let pages swallow events while flinging

I assume this is what was meant?

This is the sort of behaviour I'd expect in terms of how it ends up as a UX; I also think it's reasonable from a web-developer viewpoint as well, though others may well disagree.

Comment 23

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

Comment on attachment 8512712 [details] [diff] [review]
Don't let pages swallow events while flinging

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

Not quite what I had in mind. Some problems with this approach:
- IsMoving() is a very "lax" check and will often be true even when we're not flywheel-ing. That check would need to be more restrictive. In fact I would just use the existing FastMoving check
- Touch events will still get delivered to content for the second fling, which we don't want to happen if we're not going to respect the prevent-default on it. That's just going to be confusing

What I had in mind was more along the lines of using the nsEventStatus_eConsumeNoDefault return value from APZCTreeManager::ReceiveInputEvent to implement this, so that the input block get eaten entirely if we're in a fast fling. That way content won't see the input events at all. We should probably merge this with the DisallowSingleTap code so that basically whatever input block we get while fast-moving, we eat it - whether it's a tap to stop the fling or another fling to kick into flywheel.

Some of my upcoming changes to this code (which I'll probably put in a new bug hanging off bug 918288) should make this easier to do.

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Not quite what I had in mind. Some problems with this approach:
> - IsMoving() is a very "lax" check and will often be true even when we're
> not flywheel-ing. That check would need to be more restrictive. In fact I
> would just use the existing FastMoving check

Right, but we don't only want this to trigger during flywheeling, we want it to trigger in any situation you could initiate flywheeling. There probably should be a threshold, but it should be slower than the FastMoving threshold I think.

> - Touch events will still get delivered to content for the second fling,
> which we don't want to happen if we're not going to respect the
> prevent-default on it. That's just going to be confusing

Whoops, you're right...

> What I had in mind was more along the lines of using the
> nsEventStatus_eConsumeNoDefault return value from
> APZCTreeManager::ReceiveInputEvent to implement this, so that the input
> block get eaten entirely if we're in a fast fling. That way content won't
> see the input events at all. We should probably merge this with the
> DisallowSingleTap code so that basically whatever input block we get while
> fast-moving, we eat it - whether it's a tap to stop the fling or another
> fling to kick into flywheel.

I don't object to anything said, but it does feel like it's getting quite complicated to counter a <=300ms pause - would it not make sense, at least in the interim, that we just don't call CancelAnimations until we get the return from the handler?

> Some of my upcoming changes to this code (which I'll probably put in a new
> bug hanging off bug 918288) should make this easier to do.

If you could cc me, I'd appreciate it - also, if you ever fancy taking this bug, go ahead ;) (but I don't mind dealing with it)

Comment 25

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

(In reply to Chris Lord [:cwiiis] from comment #24)
> I don't object to anything said, but it does feel like it's getting quite
> complicated to counter a <=300ms pause - would it not make sense, at least
> in the interim, that we just don't call CancelAnimations until we get the
> return from the handler?

Yes, it is complicated, but that's because the problem is inherently complicated. There's a delicate balance between what the user wants and what web content wants and we have to be careful how we implement this. I don't agree with removing the CancelAnimations call at this point because (a) that's not going to be something we can ship on its own either on 2.1 or 2.2 and (b) we can implement the more correct complicated solution for the 2.2 timeframe anyway. If we're going to leave stuff in a broken state it might as well be the status quo broken state and not a different broken state which doesn't move us forward.

> 
> If you could cc me, I'd appreciate it - also, if you ever fancy taking this
> bug, go ahead ;) (but I don't mind dealing with it)

Will do, I'll probably file it later today or tomorrow with my patches. I don't mind taking this bug but probably not until at late this week since I don't anticipate getting blocked on my hit testing work until then. I'd consider it a fair trade if you finished off bug 1084321 and I took this one :)

Comment 26

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> (In reply to Chris Lord [:cwiiis] from comment #24)
> Will do, I'll probably file it later today or tomorrow with my patches. I
> don't mind taking this bug but probably not until at late this week since I
> don't anticipate getting blocked on my hit testing work until then. I'd
> consider it a fair trade if you finished off bug 1084321 and I took this one
> :)

Oh man, I don't even know what's happening with that one... Huge horizontal scrollbar, I don't even... I'll have a play and see if I can work around it somehow (I wonder if the test even needs to be scrollable?)

Comment 27

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

Attachment: WIP Patch

This is what I had in mind. This patch applies on top of the patches in bug 1090398 (although it should be pretty easy to rebase onto current master). It seems to do what I expect although I haven't tested it extensively. I'll come back to this once bug 1090398 is landed.

Comment 29

[Chris Lord [:cwiiis]]

Over to you :)

Comment 30

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

Attachment: Patch

The WIP seems to hold up well. I just renamed some stuff and tested it some more.

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8529152 [details] [diff] [review]
Patch

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

::: gfx/layers/apz/src/InputQueue.cpp
@@ +109,5 @@
>    // XXX calling ArePointerEventsConsumable on |target| may be wrong here if
>    // the target isn't confirmed and the real target turns out to be something
>    // else. For now assume this is rare enough that it's not an issue.
> +  if (block->IsDuringFastMotion()) {
> +    result = nsEventStatus_eConsumeNoDefault;

Please update the comment for APZCTreeManager::ReceiveInputEvent(), which currently says for eConsumeNoDefault: "Currently this is only returned if the APZ determines that something is in overscroll and the event should be ignored entirely."

Comment 32

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

Updated and landed: https://hg.mozilla.org/integration/b2g-inbound/rev/6780420c1572

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6780420c1572