Closed
Bug 1190469
Opened 7 years ago
Closed 7 years ago
Interrupting a pan will break scroll snapping with apz
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
People
(Reporter: cwiiis, Assigned: kats)
References
()
Details
Attachments
(3 files)
Assume a page with mandatory scroll snapping setup. - If you put a second finger on the screen during a pan gesture, when you release your fingers scroll snapping will not activate. - If you lock your phone while scrolling, then remove your finger, upon unlocking, the view will not have snapped. There are some situations where interrupting the scroll not only doesn't snap, but then disables scrolling until you do something more drastic to recover it, but I've not managed to make a reduced test-case for that. On the given URL, it should be impossible to end up in a stable scroll position with red showing, but following either of the two steps listed above allows this.
Assignee | ||
Comment 1•7 years ago
|
||
CC'ing Markus because we were recently discussing how there might be more places we need to do a RequestFlingSnap call.
Reporter | ||
Comment 3•7 years ago
|
||
[Blocking Requested - why for this release]: This causes some pretty bad looking/feeling behaviour in top-level components in Gaia. Namely, the home screen can get 'stuck' between the apps and pinned pages panel, and the task manager can get stuck between windows. It would be nice to have this fixed so that authors can rely on this feature without timeout work-arounds.
blocking-b2g: --- → 2.5?
Reporter | ||
Comment 7•7 years ago
|
||
The amount of duplicates we're getting of this is starting to get a little silly :) Markus, do you have time to look at this, or can you delegate to someone that does? Also cc'ing Botond as it's an apz issue.
Flags: needinfo?(mstange)
Assignee | ||
Comment 8•7 years ago
|
||
I can take a look at this. I'm looking at other APZ+snapping issues anyway.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(mstange)
Assignee | ||
Comment 9•7 years ago
|
||
Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=
Attachment #8679431 -
Flags: review?(botond)
Assignee | ||
Comment 10•7 years ago
|
||
Bug 1190469 - Request scroll snapping under roughly the same conditions that we clear overscroll. r=
Attachment #8679432 -
Flags: review?(botond)
Assignee | ||
Comment 11•7 years ago
|
||
Try push with above patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d69a35e360e Chris, if you get a chance, please apply these patches and let me know if you still see issues with them. We don't have a lot of automated test coverage of this code so extra eyes on it would be great.
Updated•7 years ago
|
Attachment #8679431 -
Flags: review?(botond) → review+
Comment 13•7 years ago
|
||
Comment on attachment 8679431 [details] MozReview Request: Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r= https://reviewboard.mozilla.org/r/23413/#review21039
Updated•7 years ago
|
Attachment #8679432 -
Flags: review?(botond)
Comment 14•7 years ago
|
||
Comment on attachment 8679432 [details] MozReview Request: Bug 1190469 - Remove unused function. r= https://reviewboard.mozilla.org/r/23415/#review21045 ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2508 (Diff revision 1) > + RequestSnap(); I looked through the call sites of CancelAnimation() where we do not pass DoNotSnap: - CancelAnimationAndGestureState(), called on touch-cancel and similar scenarios. I agree we want to snap there. - Destroy() It doesn't seem like we want to snap there. - OnScrollWheel() Here the CancelAnimation() call happens before the scroll. It doesn't seem like we want to snap there. (We probably want to snap *after* the scroll, but I guess that's handled by a separate mechanism in bug 1141884). - OnPan*() Not sure about these. I don't have a good mental model of pan gesture events. On a different note, as these snap requests are sent for every APZC, we're generating some extra IPC traffic. Might it make sense to have a flag in FrameMetrics that indicates whether the scroll frame has snap points, and only send the requests if it does?
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #14) > Comment on attachment 8679432 [details] > MozReview Request: Bug 1190469 - Request scroll snapping under roughly the > same conditions that we clear overscroll. r= > > https://reviewboard.mozilla.org/r/23415/#review21045 > > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2508 > (Diff revision 1) > > + RequestSnap(); > > I looked through the call sites of CancelAnimation() where we do not pass > DoNotSnap: > > - CancelAnimationAndGestureState(), called > on touch-cancel and similar scenarios. > I agree we want to snap there. > > - Destroy() > It doesn't seem like we want to snap there. > > - OnScrollWheel() > Here the CancelAnimation() call happens before > the scroll. It doesn't seem like we want to > snap there. (We probably want to snap *after* > the scroll, but I guess that's handled by a > separate mechanism in bug 1141884). > > - OnPan*() > Not sure about these. I don't have a good > mental model of pan gesture events. Yeah it probably makes most sense to not snap for the CancelAnimations calls in both OnScrollWheel and OnPan* because in all of those cases a user action is about to start scrolling, and we don't want to trigger a snap to the pre-scroll position. Doing that means that there's only a couple of calls which don't pass DoNotSnap, so I'm thinking of inverting the condition and calling it RequestSnap. > On a different note, as these snap requests are sent for every APZC, we're > generating some extra IPC traffic. Might it make sense to have a flag in > FrameMetrics that indicates whether the scroll frame has snap points, and > only send the requests if it does? Yeah that's a good idea, I can file a follow-up for that. We should also clean up the flags in FrameMetrics as there are a billion of them floating around and probably could be packed better.
Comment 16•7 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15) > Doing that means that there's only a couple of calls which don't pass > DoNotSnap, so I'm thinking of inverting the condition and calling it > RequestSnap. I was just going to suggest the same thing :) > > On a different note, as these snap requests are sent for every APZC, we're > > generating some extra IPC traffic. Might it make sense to have a flag in > > FrameMetrics that indicates whether the scroll frame has snap points, and > > only send the requests if it does? > > Yeah that's a good idea, I can file a follow-up for that. We could alternatively just punt on this until we fix bug 1219296; I'll leave that up to you. > We should also > clean up the flags in FrameMetrics as there are a billion of them floating > around and probably could be packed better. Good idea!
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #16) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15) > > Doing that means that there's only a couple of calls which don't pass > > DoNotSnap, so I'm thinking of inverting the condition and calling it > > RequestSnap. > > I was just going to suggest the same thing :) I did this, but ended up having to use CancelAnimationFlags::RequestSnap at the call sites because of the naming conflict with the function RequestSnap(). Also, I discovered that in the Destroy() function we do actually want to do a snap; that's what fixes the "lock the phone with finger down" scenario described in comment 0. > > > On a different note, as these snap requests are sent for every APZC, we're > > > generating some extra IPC traffic. Might it make sense to have a flag in > > > FrameMetrics that indicates whether the scroll frame has snap points, and > > > only send the requests if it does? > > > > Yeah that's a good idea, I can file a follow-up for that. > > We could alternatively just punt on this until we fix bug 1219296; I'll > leave that up to you. With the updated patch it's more obvious where the extra snap calls are, and I think the extra overhead won't be too high. So yeah I'll punt on this for now. > > We should also > > clean up the flags in FrameMetrics as there are a billion of them floating > > around and probably could be packed better. > > Good idea! I'll file a bug for this anyway.
Assignee | ||
Comment 18•7 years ago
|
||
(Also, I inserted a new patch to remove an unused function, MozReview probably won't match up the patches properly. Sorry...)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8679431 [details] MozReview Request: Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r= Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8679432 [details] MozReview Request: Bug 1190469 - Remove unused function. r= Bug 1190469 - Remove unused function. r=
Attachment #8679432 -
Attachment description: MozReview Request: Bug 1190469 - Request scroll snapping under roughly the same conditions that we clear overscroll. r= → MozReview Request: Bug 1190469 - Remove unused function. r=
Attachment #8679432 -
Flags: review?(botond)
Assignee | ||
Comment 21•7 years ago
|
||
Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r=
Attachment #8680123 -
Flags: review?(botond)
Comment 22•7 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17) > Also, I discovered that in the Destroy() function we do actually want to do > a snap; that's what fixes the "lock the phone with finger down" scenario > described in comment 0. Ah, good catch! I was thinking we wouldn't need to because the APZC is being destroyed anyways, but RequestFlingSnap also sets content-side state and we want that.
Comment 23•7 years ago
|
||
Comment on attachment 8679432 [details] MozReview Request: Bug 1190469 - Remove unused function. r= https://reviewboard.mozilla.org/r/23415/#review21055 ::: gfx/layers/apz/src/APZCTreeManager.h (Diff revision 2) > - void CancelAnimation(const ScrollableLayerGuid &aGuid); This function call has a call site: https://dxr.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d/widget/android/AndroidJNI.cpp#336
Attachment #8679432 -
Flags: review?(botond)
Updated•7 years ago
|
Attachment #8680123 -
Flags: review?(botond) → review+
Comment 24•7 years ago
|
||
Comment on attachment 8680123 [details] MozReview Request: Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r= https://reviewboard.mozilla.org/r/23561/#review21057
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #23) > This function call has a call site: > https://dxr.mozilla.org/mozilla-central/rev/ > 2b333a1d94e805a59c619ee41a6dec7fdcce505d/widget/android/AndroidJNI.cpp#336 Oh, good catch! dxr failed to find it, presumably because android. I'll drop that patch.
Comment 26•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f858616f8a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec5fb135e42
Comment 27•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f858616f8a2 https://hg.mozilla.org/mozilla-central/rev/6ec5fb135e42
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•