1022719 - The overscroll effect gets stuck if you put a second finger on the screen

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Etienne Segonzac (:etienne)]

Tested on today's central, in the homescreen app, in the settings app, in the contacts app.

STR:

* start dragging down at the top of a scrollable div
* put a second finger on the screen
* release both fingers from the screen

Expected:
* we bounce back

Actual
* everything is "stuck" in the position at the time the second finger touched the screen

Turning the screen off then on again (triggering a visibility change) brings you back to a good state.

Comment 1

[Botond Ballo [:botond]]

Confirmed, will investigate.

Comment 2

[Botond Ballo [:botond]]

The problem is that we currently only start a snap-back animation at the end of a fling, but there is no fling after two fingers are down on the screen and released. Should be a straightforward fix.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> The problem is that we currently only start a snap-back animation at the end
> of a fling, but there is no fling after two fingers are down on the screen
> and released.

That's not quite accurate.

We currently ignore touch blocks that start in an overscrolled state. This means that when we get the touch-start for the second finger going down, we set mApzcForInputBlock to nullptr. This means that touch events up to and including the touch-ends are not sent to any APZC, and so any logic that might trigger an animation on touch-end is not run.

Comment 4

[Botond Ballo [:botond]]

The fix is therefore not as straightforward as I originally thought.

I can see a few options:

  1) Instead of ignoring touch-blocks that start in an overscrolled state,
     ignore touch-blocks that start after a snap-back animation has started.
     In particular, the touch-block starting with the second finger going
     down in an overscrolled state would not be ignored.

     This has some implications. If we don't ignore this second touch-block,
     it means we will be pinch-zooming in an overscrolled state, something
     we currently don't allow. We will have to define the semantics of this,
     and add relevant support to the OnScale*() functions.

  2) Still ignore the second touch-block, but have some special logic that
     sends the APZC a manufactured touch-end (or the relevant actual touch-
     end) when we want it to start the snap-back.

  3) Revise our approach to handling touch events while in overscroll
     altogether, i.e. never ignore touch-blocks in an overscrolled state.
     This would require some additional investigation.

Comment 5

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

Could we just bail out at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#544 if mApzcForInputBlock is non-null and in overscroll?

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Could we just bail out at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> APZCTreeManager.cpp#544 if mApzcForInputBlock is non-null and in overscroll?

Interesting suggestion. I think what would happen though is subsequent touch-moves for either finger would be processed by the overscrolled APZC while in the PANNING state, resulting in erratic panning.

Comment 7

[Botond Ballo [:botond]]

I think it would inform this discussion to hear what UX would prefer the behaviour to be when putting a second finger down in an overscrolled state. I will ask Gordon about this.

Comment 8

[Botond Ballo [:botond]]

I talked about this with Gordon. He had three suggestions, in decreasing order of preference UX-wise:

  (1) Keep panning once the second finger goes down, somehow determining how much
      to pan by from the positions of both fingers (e.g. based on the movement
      of the midpoint of the fingers, or based on whichever finger is further in
      the direction of overscroll). When one of the two fingers is lifted, continue
      panning with the remaining finger. When the remaining finger is lifted,
      start the snap-back.

  (2) Keep panning once the second finger goes down, based only on the position of
      the first finger. Basically, the second finger is completely ignored. Once
      the first finger is released, regardless of the status of the second finger,
      start the snap-back.

  (3) Start the snap-back as soon as the second finger goes down and ignore touch
      events from then on.

I'll think about what implementing each of these would involve.

Comment 9

[Botond Ballo [:botond]]

I think (2) seems like the best balance between UX and ease of implementation. I *think* we can implement it relatively easily by detecting a second finger going down when overscrolling, and filtering out touch points with that touch identifier until we receive a touch-end for it. I'll give this a try.

Comment 10

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

I think (1) would be implementable by doing what I suggested in comment 5, combined with having GetFirstTouchScreenPoint return the average of all the points rather than just the first point. I didn't think about it too hard though, I might be missing something.

Comment 12

[Botond Ballo [:botond]]

Attachment: bug1022719.patch

This implements approach (2).

Kats, what do you think about doing this for now, and perhaps attempting an implementation of (1) as a follow-up?

Comment 13

[Botond Ballo [:botond]]

Attachment: Patch that implements approach (2)

Added a missing 'break'.

Comment 14

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

Comment on attachment 8438644 [details] [diff] [review]
Patch that implements approach (2)

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

What if you put down three fingers?

Comment 15

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

What if you lick the screen?! (Sorry, couldn't resist :)

Comment 16

[Botond Ballo [:botond]]

Attachment: Patch that implements approach (1)

This is an attempt at implementing approach (1). It is not complete in that currently it starts the snap-back as soon as one of two fingers are lifted, rather than waiting until both are, but it does implement panning based on the midpoint of the two fingers.

Unfortunately, while testing this I realized that (1) has the issue that the scroll position jumps when switching from panning based on a single touch point, to panning based on the midpoint of two touch points. This jump is pretty jarring, and after discussing this with Gordon we agree that as a result, (2) is the better approach.

As a result, I will go back to working on approach (2). My current patch basically works, but it doesn't do things properly for 3 or more touch points, so I'll revise it to do so. 

I'm posting this patch just so it's there if we want to go back to approach (1) for some reason.

Comment 17

[Botond Ballo [:botond]]

Attachment: Patch that implements approach (2)

Updated patch for approach (2) to handle 3 or more touch points properly.

Comment 19

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

Comment on attachment 8438746 [details] [diff] [review]
Patch that implements approach (2)

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

Comments below apply to both versions of the code (InputData and WidgetInputEvent). Minusing because I think this needs to handle CANCEL better.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +601,5 @@
> +  // touch points included in this event.
> +  for (size_t i = 0; i < mIgnoredTouchIdentifiers.Length(); ++i) {
> +    for (size_t j = 0; j < aEvent.touches.Length(); ++j) {
> +      if (aEvent.touches[j]->Identifier() == mIgnoredTouchIdentifiers[i]) {
> +        if (aEvent.message == NS_TOUCH_CANCEL ||

For CANCEL events I think we should just clear mIgnoredTouchIdentifiers entirely and pass the event through. I don't know if CANCEL events usually have touch points or not, or if the data in them is reliable at all. Also if we get a cancel event we should definitely not be returning eConsumeNoDefault because that will prevent the event from getting to other code that might need to clear state.

@@ +612,5 @@
> +        break;
> +      }
> +    }
> +    if (aEvent.touches.IsEmpty()) {
> +      return nsEventStatus_eConsumeNoDefault;

I don't think we should ever hit this case, right? I'd like a release-mode warning if we do hit this.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +377,5 @@
>     * was inside an overscrolled APZC.
>     */
>    bool mInOverscrolledApzc;
> +  /* Identifiers for touch points that we are currently ignoring.
> +   * This only happens when more than oen finger is down while in overscroll.

s/oen/one/

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> For CANCEL events I think we should just clear mIgnoredTouchIdentifiers
> entirely and pass the event through. I don't know if CANCEL events usually
> have touch points or not, or if the data in them is reliable at all. Also if
> we get a cancel event we should definitely not be returning
> eConsumeNoDefault because that will prevent the event from getting to other
> code that might need to clear state.

Seems reasonable. I'll update the patch.

(Does this line [1] not make an assumption about the number of touch points in a CANCEL event?)

> @@ +612,5 @@
> > +        break;
> > +      }
> > +    }
> > +    if (aEvent.touches.IsEmpty()) {
> > +      return nsEventStatus_eConsumeNoDefault;
> 
> I don't think we should ever hit this case, right? I'd like a release-mode
> warning if we do hit this.

I would expect to hit it in the following case:

  - First finger pans into overscroll.

  - Second finger touches down. 
    Second finger's id is added to mIgnoredTouchIdentifiers.
    Panning continues.

  - First finger is released. 
    Touch-end triggers snap-back animation. 
    Second finger's id is still in mIgnoredTouchIdentifiers.

  - Second finger is released, triggering a TOUCH_END with a single touch point.
    This touch point gets filtered out, resulting in an empty event which we discard.
    (We don't want to send this touch-end to APZC, since we didn't send the
    corresponding touch-start).

Is this scenario problematic in some way?

(By the way, I realized I am doing something silly going in the InputData version of this code. There I exit early before I get a chance to remove the touch identifier from mIgnoredTouchIdentifiers. I'll fix that.)

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

Comment 21

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

(In reply to Botond Ballo [:botond] from comment #20)
> (Does this line [1] not make an assumption about the number of touch points
> in a CANCEL event?)

Yeah it does. We should probably not do that.

> I would expect to hit it in the following case:
> 
>   - First finger pans into overscroll.
> 
>   - Second finger touches down. 
>     Second finger's id is added to mIgnoredTouchIdentifiers.
>     Panning continues.
> 
>   - First finger is released. 
>     Touch-end triggers snap-back animation. 
>     Second finger's id is still in mIgnoredTouchIdentifiers.
> 
>   - Second finger is released, triggering a TOUCH_END with a single touch
> point.
>     This touch point gets filtered out, resulting in an empty event which we
> discard.
>     (We don't want to send this touch-end to APZC, since we didn't send the
>     corresponding touch-start).
> 
> Is this scenario problematic in some way?

Ah I see. In that case disregard my comment about adding the warning. I can't think of any problems that might result from this, although I still feel uneasy about this whole approach..

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> > I would expect to hit it in the following case:
> > 
> >   - First finger pans into overscroll.
> > 
> >   - Second finger touches down. 
> >     Second finger's id is added to mIgnoredTouchIdentifiers.
> >     Panning continues.
> > 
> >   - First finger is released. 
> >     Touch-end triggers snap-back animation. 
> >     Second finger's id is still in mIgnoredTouchIdentifiers.
> > 
> >   - Second finger is released, triggering a TOUCH_END with a single touch
> > point.
> >     This touch point gets filtered out, resulting in an empty event which we
> > discard.
> >     (We don't want to send this touch-end to APZC, since we didn't send the
> >     corresponding touch-start).
> > 
> > Is this scenario problematic in some way?
> 
> Ah I see. In that case disregard my comment about adding the warning. I
> can't think of any problems that might result from this, although I still
> feel uneasy about this whole approach..

Do you feel uneasy about what we are doing in terms of functionality/UX (i.e. (2) instead of (1) from comment 8), or just how we are implementing it? If the latter, do you have a different implementation approach in mind that would make you feel more at ease?

Comment 23

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

(In reply to Botond Ballo [:botond] from comment #22)
> Do you feel uneasy about what we are doing in terms of functionality/UX
> (i.e. (2) instead of (1) from comment 8), or just how we are implementing
> it? If the latter, do you have a different implementation approach in mind
> that would make you feel more at ease?

I think the UX is fine, I'm just uneasy about the implementation. Pulling touch points of the events seems a little hacky to me. Something that I would find conceptually cleaner is to just ignore the right things in the right places, but I admit that might be more brittle and complicated code-wise. I don't really have any better suggestions.

Comment 24

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> I'm just uneasy about the implementation. Pulling
> touch points of the events seems a little hacky to me. Something that I
> would find conceptually cleaner is to just ignore the right things in the
> right places, but I admit that might be more brittle and complicated
> code-wise.

My conceptual justification for doing it this way is the following: 

When you put down one finger during a snap-back, that touch is ignored, by not processing the entire event, including not sending it to content.

Here, we are doing something analogous: ignoring a touch (the second finger) that occurs during an overscrolled state that we do not want to have any effect. It's just that here there's a pre-existing touch (the first finger) that we don't want to ignore, so we can't ignore the entire event - instead, we just filter out the ignored touch from it, including when sending it to content.

Note that "just ignoring the right things in the right places" can't extend to content the way that this approach does.

Comment 25

[Botond Ballo [:botond]]

Attachment: Patch that implements approach (2)

Addressed review comments.

Note that in ReceiveInputEvent(InputData), I moved the copy of 'multiTouchInput' to 'inputForApzc' outside of the 'if (mApzcForInputBlock)' block. The reason I had to do this is somewhat complicated:

  - The filtering out of ignored touch points has to be done on
    the copy, since the original is 'const'. (This will change in
    the future.)

  - Removing identifiers which are getting a touch-end from
    mIgnoredTouchIdentifiers basically has to be done in the same
    loop as filtering out the ignored touch points.

      - If we remove the touch-end identifiers first, these
        touch points will not be filtered out of the touch-end
        that gets propagated to the APZC and content.

      - If we filter out the touch points first, we might exit
        early before we get a chance to remove the identifiers
        (since the number of touch points might go down to zero).

  - Removing identifiers which are getting a touch-end needs to
    be done even if we don't have an mApzcForInputBlock.

We can get around this with some code repetition, but moving the copy outside the 'if' is simpler. Let me know if this is OK.

Comment 26

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

Comment on attachment 8439413 [details] [diff] [review]
Patch that implements approach (2)

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

This looks ok. I realized while reading this it might be simpler to store the touch identifier of the touch we do care about, rather than storing the identifiers for all the touches we want to ignore. Feel free to make that change if you think it would be simpler/clearer.

Comment 27

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> This looks ok. I realized while reading this it might be simpler to store
> the touch identifier of the touch we do care about, rather than storing the
> identifiers for all the touches we want to ignore. Feel free to make that
> change if you think it would be simpler/clearer.

Summarizing some IRC discussion:

  - I agree that this would be simpler.

  - In addition, I realized that my patch is assuming that for a TOUCH_START,
    the new touch is the last one in the mTouches array. This is not guaranteed
    to be the case. Therefore, we need to get the information of which touch
    identifier to ignore/retain from elsewhere.

I will post new patches that make these changes.

Comment 28

[Botond Ballo [:botond]]

Attachment: [Approach (2)] Part 1 - Const-correctness improvements

Comment 29

[Botond Ballo [:botond]]

Attachment: [Approach (2)] Part 2 - The actual fix

Comment 30

[Botond Ballo [:botond]]

The Try push at https://tbpl.mozilla.org/?tree=Try&rev=a7baab88a6f0 includes these patches as well.

Comment 32

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

Comment on attachment 8439545 [details] [diff] [review]
[Approach (2)] Part 2 - The actual fix

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

r=me with the thing below fixed. Would be nice to have a simple gtest for this as well, but I'm not going to block the landing on that.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +454,5 @@
> +              inputForApzc.mTouches.RemoveElementAt(j);
> +              if (inputForApzc.mTouches.IsEmpty()) {
> +                return nsEventStatus_eConsumeNoDefault;
> +              }
> +              break;

s/break/j--/. Ditto for the other version of the code. You can also move the if condition out of the loop here.

Comment 33

[Botond Ballo [:botond]]

Addressed review comment and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d592384ef8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5b1689e3a0

Comment 34

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> Would be nice to have a simple gtest for
> this as well, but I'm not going to block the landing on that.

Filed a follow-up for this: bug 1025436.

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2d592384ef8c
https://hg.mozilla.org/mozilla-central/rev/4b5b1689e3a0

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55a22e2c66ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/76bc5db75c1f

Comment 37

[Botond Ballo [:botond]]

31 is unaffected as the overscroll effect was introduced in bug 998025, which landed in 32.