Closed Bug 1027309 Opened 6 years ago Closed 6 years ago

[Homescreen] Still getting stuck on the homescreen when panning down with two fingers

Categories

(Core :: Panning and Zooming, defect)

33 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: marcia, Assigned: kats)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached image 2014-06-18-13-00-45.png
Flame, while running:

Gaia   431aed0a7c7560c6eacd35ea69aa0a7a4ebe72c7
SourceStamp 37f08ddaea48
BuildID 20140618040513
Version 33.0a1
Base: 121

STR:
1. Use two fingers and pan down the screen.
2. Receive the attached screenshot.


Bug 1022719 fixed this, but it still seems to be happening in the build I am running today. It happens with the Music app as well (see Bug 1026793)
Can confirm, it happens every time two fingers or more are used to scroll.
I could reproduce it in the settings app as well.
To clarify(In reply to Marcia Knous [:marcia - use needinfo] from comment #0)
> Bug 1022719 fixed this, but it still seems to be happening in the build I am
> running today.

To clarify, this bug concerns a different gesture than the one in bug 1022719's STR. That involved panning with one finger and then putting a second finger on the screen, and that has been fixed. This involves panning with two fingers.

I'm not sure off the top of my head what state changes happen in GestureEventListener/APZC when you pan with two fingers. It seems to be distinct from a pinch, because you cannot go into overscroll while you're pinching, but you can while panning with two fingers.
We should find out if this affects 2.0 as well. This should probably be a blocker.
blocking-b2g: --- → 2.1?
status-b2g-v2.0: --- → ?
Keywords: qawanted
blocking-b2g: 2.0? → 2.0+
Assignee: nobody → bugmail.mozilla
I investigated, and found the following:

- When two fingers go down, the mApzcForInputBlock is set to the root APZC for the process, because that's the zoomable APZC.
- Therefore, all touch events are delivered to the root APZC.
- While scrolling all scroll attempts go through the DispatchScroll/AttemptScroll code path, which always delivers the scroll attempt to the APZCs in scroll-handoff-chain order.
- Since the scroll-handoff-chain starts with the sub-APZC rather than the root APZC, it is the sub-APZC that actually does the scrolling and goes into overscroll.
- When doing a touch-end, it is the root APZC that enters the fling state, determines that there is no overscroll (because there isn't, on the root APZC), and terminates.
- This leaves the sub-APZC in overscroll.

I think the problem here is that even though mApzcForInputBlock is the root APZC and that's where the events are going, the first APZC in the scroll handoff chain is the sub-APZC. That seems like a violation of various assumptions baked into the code. I think the scroll handoff chain needs to be rebuilt using the correct mApzcForInputBlock; I will try implementing that.
This fixes the problem, but also completely prevents two-finger scrolling on the homescreen (and probably in other places). Not sure if that's acceptable or not. I'll have to look back at previous versions and see if this is always what we did.
Based on the behaviour with APZ disabled I think disabling scrolling with two fingers should be ok. However the above patches still leave an APZC in a bad state when this happens, which manifests as the scrollbar not fading out. Need to fix that.
Btw, I just filed bug 1030181 to refactor all this duplicated code. I would have done it as a part 0 on this bug but I don't want to uplift that change to 2.0, so I decided to put it a different bug.
I can get overscroll similarly stuck in the Line app by scrolling down with a single finger when the last message is one that I sent.
Opened bug 1030221 for comment 11 per Kats's request.
Comment on attachment 8445383 [details] [diff] [review]
Part 1 - More logging

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1041,5 @@
>    // but do not follow the expected layer tree structure. If there are no
>    // scroll parent links we just walk up the tree to find the scroll parent.
>    AsyncPanZoomController* apzc = aInitialTarget;
>    while (apzc != nullptr) {
> +    APZCTM_LOG("mOverscrollHandoffChain[%d] = %p\n", mOverscrollHandoffChain.length(), apzc);

Since this can fail, maybe it's best to log this after the append?

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +468,5 @@
>      bool shouldContinueFlingX = mApzc.mX.FlingApplyFrictionOrCancel(aDelta, friction, threshold),
>           shouldContinueFlingY = mApzc.mY.FlingApplyFrictionOrCancel(aDelta, friction, threshold);
>      // If we shouldn't continue the fling, let's just stop and repaint.
>      if (!shouldContinueFlingX && !shouldContinueFlingY) {
> +      APZC_LOG("%p ending fling animation. Overscrolled = %d\n", &mApzc, mApzc.IsOverscrolled());

Super-nit: in the only other place we APZC_LOG a bool, it's written as "aFirstPaint=%d" (I don't care whether or not you change this)
Attachment #8445383 - Flags: review?(drs+bugzilla) → review+
Comment on attachment 8445915 [details] [diff] [review]
Part 2 - Build the handoff chain from the input-receiving APZC

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

I'm probably not the best person to review this, but it looks fine, and without Botond I'm not sure that we have a choice. I'd like this to have a test but I don't think it matters that much.
Attachment #8445915 - Flags: review?(drs+bugzilla) → review+
Comment on attachment 8445916 [details] [diff] [review]
Part 3 - Reset the APZ that is no longer the input-receiving APZ

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +609,5 @@
> +        MultiTouchInput cancel(MultiTouchInput::MULTITOUCH_CANCEL, 0, TimeStamp::Now(), 0);
> +        mApzcForInputBlock->ReceiveInputEvent(cancel);
> +      }
> +      mApzcForInputBlock = apzc;
> +    }

This should be refactored into a function and called from both places.
Attachment #8445916 - Flags: review?(drs+bugzilla) → review-
(In reply to Doug Sherk (:drs) from comment #15)
> This should be refactored into a function and called from both places.

I collapse the two things into one as part of bug 1030181. Is that acceptable, or do you want me to do it here as well?
Comment on attachment 8445916 [details] [diff] [review]
Part 3 - Reset the APZ that is no longer the input-receiving APZ

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> I collapse the two things into one as part of bug 1030181. Is that
> acceptable, or do you want me to do it here as well?

Yeah, I just noticed that as I was reviewing bug 1030181. It's fine.
Attachment #8445916 - Flags: review- → review+
(In reply to Doug Sherk (:drs) from comment #13)
> Since this can fail, maybe it's best to log this after the append?

Done.

> Super-nit: in the only other place we APZC_LOG a bool, it's written as
> "aFirstPaint=%d" (I don't care whether or not you change this)

Done.

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/466e508cbb86
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/5518a4d5db5b
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ba0403f3ef5d
This issue has been verified successfully on Flame2.1/Flame2.0
Verify video: "verify_1027309.MP4"

Flame2.0 build:
Gaia-Rev        1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/faa64077b0c2
Build-ID        20141119000207
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141119.035230
FW-Date         Wed Nov 19 03:52:41 EST 2014
Bootloader      L1TC00011880

Flame2.1 build:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev      
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.040617
FW-Date         Tue Nov 25 04:06:28 EST 2014
Bootloader      L1TC00011880
You need to log in before you can comment on or make changes to this bug.