1022956 - [Vertical Homescreen] Trying to stop a scroll starts an application

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Johan Lorenzo [:jlorenzo]]

I am not able to stop flywheel scrolling with today's new base image (v10H-4) and today's gaia+gecko. This has been implemented in bug 1015000.

Build info

Device Flame
Gaia      12af93123c5db55212d51fe235d39f21209a1eaa
Gecko     https://hg.mozilla.org/mozilla-central/rev/9305a8ec77fe
BuildID   20140609040203
Version   32.0a1
ro.build.version.incremental=104
ro.build.date=Fri Jun  6 17:35:09 CST 2014

Step to reproduce
With 3 column layout with more than 30 icons (eng build for instance), flick down twice to start flywheel scrolling
Immediately (before the end of scrolling), tap the screen where an icon is (as to stop the flywheel move).

Actual result
Tapping stop the scrolling but also launch the app.

Comment 1

[Kevin Grandon :kgrandon]

The more I think about this, the more I'm convinced that if we really want this it needs to be fixed at a platform level. If you do the same in any other app, contacts, settings, you will launch sub panels for example.

I'm not really sure that we should fix this in 2.0 because it's a long-lived implementation from APZ.

Comment 2

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

UX should decide if they want tapping during a scroll to stop the scrolling only, or to do a click along with it (current behaviour). Note this has nothing to do with flywheel, it is a general behaviour with scrolling.

Comment 3

[Johan Lorenzo [:jlorenzo]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> UX should decide if they want tapping during a scroll to stop the scrolling
> only, or to do a click along with it (current behaviour). Note this has
> nothing to do with flywheel, it is a general behaviour with scrolling.

I think the UX needs to be consistent. When you browse a list (like Cities when choosing your timezone), tapping the screen only stops the scroll and does not select the item.

Comment 4

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

We can fix the Cities list to be consistent with the homescreen behaviour. The UX team needs to decide which one is preferred.

Comment 5

[Gordon Brander :gordonb]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> UX should decide if they want tapping during a scroll to stop the scrolling
> only, or to do a click along with it (current behaviour). Note this has
> nothing to do with flywheel, it is a general behaviour with scrolling.

UX says: no drive-by tappings.

Tap should only stop a scroll, and not simultaneously activate a click, as long as the page is skating faster than some threshold speed.

Rational: I can not think of any scenario where you want to click while scrolling, however I can foresee there might be a problem with negligible velocities blocking the user from tapping. For this reason, we should probably allow clicks below a small threshold velocity, rather than waiting for 0.

Shout out if you can think of any edge-cases not anticipated here.

Comment 6

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

Sounds good, thanks!

Comment 8

[Diego Marcos [:dmarcos]]

A bug I filed (bug 1009660) has been duped against this one. Scrolling has been always extremely frustrating resulting in a very poor experience. I attached several videos where I change volume settings by accident while trying to scroll. e.g:

https://bug1009660.bugzilla.mozilla.org/attachment.cgi?id=8437896

Scrolling is the bread and butter of a touchscreen device. We must nail those fundamental affordances hence this bug should be top priority. I'm summoning other people on the UX side. Blocking 2.0?

Comment 9

[Tiffanie Shakespeare [:tif]]

NI'ing Jacqueline who worked on the feature.

Comment 10

[Jason Smith [:jsmith]]

The video here indicates this is present across multiple apps, which makes me think is probably a long-standing issue as Kevin indicates. This seems like something we could look into fixing, but I'm not sure I'd block on this.

Comment 11

[Rob MacDonald [:robmac]]

(In reply to Gordon Brander :gordonb from comment #5) 
> UX says: no drive-by tappings.
 
Agreed. This would resolve the issue in Diego's video as well, which I was able to easily replicate.

Comment 12

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

I agree we should fix this, but just to make sure the flags are consistent - is this actually a regression?

Comment 13

[Kevin Grandon :kgrandon]

I don't think this is a regression from the platform side at all, and now that this is in the platform component we should remove it. We had previously attempted to workaround this with touchstart and setTimeouts but that was a mess, and didn't really work. Perhaps that's where the regression keyword came from.

Comment 14

[Diego Marcos [:dmarcos]]

This is not a regression but makes scrolling extremely frustrating. We must nail these extremely basic interactions. What do you think people are going to think if they go to a store and try a phone that behaves like in the videos I'm attaching? I previously attached them to bug 1009660 that was marked as duplicated of this one.

Comment 15

[Diego Marcos [:dmarcos]]

Attachment: accidentalTapsWhenScrolling.MOV

Comment 16

[Diego Marcos [:dmarcos]]

Attachment: scrollAndSlidersIssue.MOV

Comment 17

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

I don't think anybody is arguing the importance of fixing this. After all we tagged it as 2.0 blocker and have somebody assigned to it. Calling it a regression just confuses the issue.

Comment 18

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

Attachment: Patch

The pref might need adjusting based on feedback.

Comment 19

[Doug Sherk (:drs) (inactive)]

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

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

It sucks that we have to do this, but it's alright. I wish there was a way to handle this from GEL by just calling APZC::GetVelocityLength() when it gets a touch start, but that would just add more goop, wouldn't be able to access the pref, and would probably be uglier.

r- because I think this should have a couple of tests.

::: gfx/2d/BasePoint.h
@@ +67,5 @@
>    }
>  
> +  T Length() const {
> +    return sqrt(x*x + y*y);
> +  }

This will have to be reviewed by Bas.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +190,5 @@
>   *
> + * "apz.fling_stop_on_tap_threshold"
> + * When flinging, if the velocity is above this number, then a tap on the
> + * screen will stop the fling without dispatching a click to content. If the
> + * velocity is below this threshold a click will also be dispatched.

s/click/single tap/

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +114,5 @@
> +    CancelMaxTapTimeoutTask();
> +    SetState(GESTURE_NONE);
> +    break;
> +  default:
> +    NS_WARNING("IgnoreLastTouchStart() called while in unexpected state");

Missing a |SetState(GESTURE_NONE)| as well as a |break| here.

::: gfx/layers/apz/src/GestureEventListener.h
@@ +54,5 @@
>    nsEventStatus HandleInputEvent(const MultiTouchInput& aEvent);
>  
>    /**
> +   * This function can be called to cancel any tap-related timeouts and state
> +   * that were triggered because we recently processed a touch-start.

"Cancels any tap-related timeouts and clears any state that was set because we recently processed a touch-start."

Comment 20

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

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

I'm working through the rest of the review comments and adding tests, but in the meantime flagging Bas for the change to BasePoint.h. Note that I just lifted that function from BasePoint3D.h (and removed the z component) so I really just need a rubberstamp review here.

Comment 21

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

Attachment: Patch

Updated to address other review comments and add some gtests.

Comment 22

[Doug Sherk (:drs) (inactive)]

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

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +94,5 @@
> +  // Run all the tasks in the queue, returning the number of tasks
> +  // run. Note that if a task queues another task while running, that
> +  // new task will not be run. Therefore, there may be still be tasks
> +  // in the queue after this function is called. Only when the return
> +  // value is 0 is the queue guaranteed to be empty.

It seems like this could cause race conditions and corruption, possibly even intermittent failures (although it's unlikely since you're just storing pointers). I checked nsTArray and it seems that it isn't thread-safe. Maybe we should implement some kind of locking here. It looks to me like it would be fairly trivial to do that.

@@ +821,5 @@
>  }
>  
> +// Start a fling, and then tap while the fling is ongoing. When
> +// aSlow is false, the tap will happen while the fling is at a
> +// high velocity, and we check that the tap doesn't trigger a tap

s/trigger/trigger sending/

@@ +848,5 @@
> +  EXPECT_EQ(2, mcc->RunThroughDelayedTasks());
> +
> +  // If we want to tap while the fling is fast, let the fling advance for 10ms only. If we want
> +  // the fling to slow down more, advance to 2000ms. These numbers may need adjusting if our
> +  // friction and threshold values change, but they should be deterministic at least.

Maybe we should note this with the pref declaration in APZC.cpp. I don't think most people would expect that editing a pref can make tests fail. But I don't think there's anything reasonable we can do here.

@@ +853,5 @@
> +  int timeDelta = aSlow ? 2000 : 10;
> +  int tapCallsExpected = aSlow ? 1 : 0;
> +  int delayedTasksExpected = aSlow ? 3 : 2;
> +
> +  // Advance the fling animation 10 milliseconds.

Stale comment. s/10 milliseconds/by timeDelta/, maybe.

Comment 23

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

(In reply to Doug Sherk (:drs) from comment #22)
> It seems like this could cause race conditions and corruption, possibly even
> intermittent failures (although it's unlikely since you're just storing
> pointers). I checked nsTArray and it seems that it isn't thread-safe. Maybe
> we should implement some kind of locking here. It looks to me like it would
> be fairly trivial to do that.

I don't see how that's possible. Everything in the gtest environment is single threaded. At worst PostDelayedTask() would get called while we're in the mTaskQueue[0]->Run() part of RunDelayedTask(), and that has well-defined, deterministic behaviour.

> Maybe we should note this with the pref declaration in APZC.cpp. I don't
> think most people would expect that editing a pref can make tests fail. But
> I don't think there's anything reasonable we can do here.

Good point, I'll add a note there. Will fix the other comments.

Comment 24

[Doug Sherk (:drs) (inactive)]

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> I don't see how that's possible. Everything in the gtest environment is
> single threaded. At worst PostDelayedTask() would get called while we're in
> the mTaskQueue[0]->Run() part of RunDelayedTask(), and that has
> well-defined, deterministic behaviour.

Oh, ok, I forgot about that.

Comment 25

[Bas Schouten (:bas.schouten)]

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

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

::: gfx/2d/BasePoint.h
@@ +66,5 @@
>      return Sub(-x, -y);
>    }
>  
> +  T Length() const {
> +    return sqrt(x*x + y*y);

nit: Let's just use hypot here directly.

Comment 26

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

Landed using hypot and other comments addressed:

https://hg.mozilla.org/integration/b2g-inbound/rev/581f045e7f5c

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581f045e7f5c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fb4f7972218

Comment 29

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0& 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/10

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0

Flame v2.1 version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0