If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jlorenzo, Assigned: kats)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 verified, b2g-v2.1 verified)

Details

(Whiteboard: [systemsfe])

Attachments

(4 attachments, 1 obsolete attachment)

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.
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.
Component: Gaia::Homescreen → Panning and Zooming
Product: Firefox OS → Core
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.
Flags: needinfo?(gbrander)
Summary: [Vertical Homescreen] Trying to stop a flywheel scrolling starts an application (regression) → [Vertical Homescreen] Trying to stop a scroll starts an application (regression)
QA Whiteboard: [VH-FC-blocking+]
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
We can fix the Cities list to be consistent with the homescreen behaviour. The UX team needs to decide which one is preferred.
See Also: → bug 889717
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(gbrander)
Sounds good, thanks!
Duplicate of this bug: 1009660
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?
Flags: needinfo?(tshakespeare)
Flags: needinfo?(rmacdonald)
Flags: needinfo?(jsmith)
Flags: needinfo?(bugmail.mozilla)
blocking-b2g: --- → 2.0?
Flags: needinfo?(bugmail.mozilla)
NI'ing Jacqueline who worked on the feature.
Flags: needinfo?(tshakespeare) → needinfo?(jsavory)
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.
QA Whiteboard: [VH-FC-blocking+] → [VH-FC-blocking-]
Flags: needinfo?(jsmith)
(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.
Flags: needinfo?(rmacdonald)
Flags: needinfo?(jsavory)

Updated

3 years ago
blocking-b2g: 2.0? → 2.0+
QA Whiteboard: [VH-FC-blocking-] → [VH-FC-blocking+]
Assignee: nobody → bugmail.mozilla
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Target Milestone: --- → 2.0 S4 (20june)

Updated

3 years ago
QA Whiteboard: [VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking+]
I agree we should fix this, but just to make sure the flags are consistent - is this actually a regression?
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.
Keywords: regression
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.
Created attachment 8442950 [details]
accidentalTapsWhenScrolling.MOV
Created attachment 8442952 [details]
scrollAndSlidersIssue.MOV
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)

Updated

3 years ago
Whiteboard: [systemsfe]
Summary: [Vertical Homescreen] Trying to stop a scroll starts an application (regression) → [Vertical Homescreen] Trying to stop a scroll starts an application
Created attachment 8445277 [details] [diff] [review]
Patch

The pref might need adjusting based on feedback.
Attachment #8445277 - Flags: review?(drs+bugzilla)
Whiteboard: [systemsfe]
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
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."
Attachment #8445277 - Flags: review?(drs+bugzilla) → review-
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.
Attachment #8445277 - Flags: review?(bas)
Created attachment 8446593 [details] [diff] [review]
Patch

Updated to address other review comments and add some gtests.
Attachment #8445277 - Attachment is obsolete: true
Attachment #8445277 - Flags: review?(bas)
Attachment #8446593 - Flags: review?(drs+bugzilla)
Attachment #8446593 - Flags: review?(bas)
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.
Attachment #8446593 - Flags: review?(drs+bugzilla) → review-
(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 on attachment 8446593 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8446593 - Flags: review- → review+
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.
Attachment #8446593 - Flags: review?(bas) → review+
Landed using hypot and other comments addressed:

https://hg.mozilla.org/integration/b2g-inbound/rev/581f045e7f5c
https://hg.mozilla.org/mozilla-central/rev/581f045e7f5c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fb4f7972218
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
Created attachment 8530753 [details]
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified
You need to log in before you can comment on or make changes to this bug.