Closed Bug 1180030 Opened 9 years ago Closed 9 years ago

Scroll snapping conflicts with overscroll animation

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: cwiiis, Assigned: botond)

References

()

Details

Attachments

(3 files)

If you have mandatory scroll-snapping set on a container and you overscroll, the overscroll animation will not play even if the scroll position doesn't change.

For example, if you have scroll-snapping set to repeat on the y-axis at 100px, overscrolling at the top (scroll position 0) does not perform the overscroll bounce animation. The appearance is quite jarring.
Can you attach a test case?
Flags: needinfo?(chrislord.net)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Can you attach a test case?

See the attached URL. If you try to overscroll at the top of the document on FirefoxOS, you will not get an overscroll bounce animation. If you remove the scroll-snap-type property in the web inspector, normal behaviour under this condition is restored.
Flags: needinfo?(chrislord.net)
Thanks, I can reproduce - I agree the lack of "bounce" is a little jarring. (So is the bright red on the test page :p). Not sure offhand why this is happening, will need some investigation.
I looked into this a bit:

  - When you overscroll and your finger leaves the screen,
    a fling animation is started. (If you were moving in
    the direction that increases overscroll, this will
    turn into an overscroll animation after one sample.)

  - When we start a fling animation, we also request a
    fling snap.

  - A fling snap results in a content scrollTo().

  - A content scrollTo() cancels in-progress APZ animations,
    such as the overscroll animation in this case.
I'm also experiencing other misbehaviour on this page: when dragging my finger back and forth, I see a lot of flickering. That's unrelated to overscroll though.
Attached patch bug1180030.patchSplinter Review
This patch restores the overscroll bounce for me.

Kip, do you foresee any issues with this approach?
Attachment #8632343 - Flags: feedback?(kgilbert)
(In reply to Botond Ballo [:botond] from comment #5)
> I'm also experiencing other misbehaviour on this page: when dragging my
> finger back and forth, I see a lot of flickering. That's unrelated to
> overscroll though.

After some more testing, I think was a combination of:
  - the scroll offset movements brought about my scroll snapping being new/unintuitive to me
  - "Low Precision Transparency" causing unexpected colours (like purple) to appear
I don't think there's actually a bug, except perhaps a missed optimization opportunity in that we're not using ColorLayers for the divs.
(In reply to Botond Ballo [:botond] from comment #7)
> I don't think there's actually a bug, except perhaps a missed optimization
> opportunity in that we're not using ColorLayers for the divs.

Filed bug 1182712 for this.
Comment on attachment 8632343 [details] [diff] [review]
bug1180030.patch

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

I like this; it should work fine.  IIRC, the overflow animation will perform a fling snap as necessary when it completes.
Attachment #8632343 - Flags: feedback?(kgilbert) → feedback+
(In reply to :kip (Kearwood Gilbert) from comment #9)
> I like this; it should work fine.  IIRC, the overflow animation will perform
> a fling snap as necessary when it completes.

What makes you say that? As far as I can tell, APZ only requests a fling snap when a fling animation is started.

That said, I would expect we don't need a fling snap when the overscroll animation completes because the scrollable element will be at one of the extremes (beginning or end) of its scrollable range.
Flags: needinfo?(kgilbert)
(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to :kip (Kearwood Gilbert) from comment #9)
> > I like this; it should work fine.  IIRC, the overflow animation will perform
> > a fling snap as necessary when it completes.
> 
> What makes you say that? As far as I can tell, APZ only requests a fling
> snap when a fling animation is started.
> 
> That said, I would expect we don't need a fling snap when the overscroll
> animation completes because the scrollable element will be at one of the
> extremes (beginning or end) of its scrollable range.
Indeed... I couldn't spot any code to trigger fling snap at the end of an overscroll animation.  It appears that snapping will be triggered if someone interrupts an overscroll animation with a fling, so should not be a problem there.
Flags: needinfo?(kgilbert)
Comment on attachment 8632343 [details] [diff] [review]
bug1180030.patch

Great - thanks, Kip!
Attachment #8632343 - Flags: review?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #10)
> That said, I would expect we don't need a fling snap when the overscroll
> animation completes because the scrollable element will be at one of the
> extremes (beginning or end) of its scrollable range.

Is it always guaranteed that the extremes will be legal scroll positions? What if somebody sets a mandatory snap point that's not at the beginning/end of the scrollable range? If it's not guaranteed then it seems like we should be doing a snap request such that it kicks in after the overscroll stretch animation but before the snap-back animation.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> (In reply to Botond Ballo [:botond] from comment #10)
> > That said, I would expect we don't need a fling snap when the overscroll
> > animation completes because the scrollable element will be at one of the
> > extremes (beginning or end) of its scrollable range.
> 
> Is it always guaranteed that the extremes will be legal scroll positions?
> What if somebody sets a mandatory snap point that's not at the beginning/end
> of the scrollable range? If it's not guaranteed then it seems like we should
> be doing a snap request such that it kicks in after the overscroll stretch
> animation but before the snap-back animation.

I think ideally it'd have the overscroll animation if and only if the extent of the scroll axis is a legal position, but if the next legal position is before the extent of the container, it would just snap back to that point like it does now.

What you suggest would still be better than what we currently do though.
Reading through the spec at http://www.w3.org/TR/css-snappoints-1/ it doesn't seem to say anything about the start and end of the scroll range implicitly being included as snap points but in a test case that I wrote (http://people.mozilla.org/~kgupta/bug/1180030.html) it seems like that's what our implementation does (tested on OS X desktop Aurora build). I'm not sure if we can assume this to always be the case - Kip?
Flags: needinfo?(kgilbert)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Reading through the spec at http://www.w3.org/TR/css-snappoints-1/ it
> doesn't seem to say anything about the start and end of the scroll range
> implicitly being included as snap points but in a test case that I wrote
> (http://people.mozilla.org/~kgupta/bug/1180030.html) it seems like that's
> what our implementation does (tested on OS X desktop Aurora build). I'm not
> sure if we can assume this to always be the case - Kip?
My interpretation of the spec is that there is no implicit snap points at the beginning and end of the scroll range.  Mochitests have been implemented to validate that we do not generate these snap points.

The end user should be able to scroll past the earliest / last snap point; however, releasing their finger from the touchscreen should cause the snapping to trigger and an animation to the valid snap position.  (This pattern should also hold true for other methods of scrolling, such as when dismissing XUL based middle mouse click scrolling, releasing the mouse button when dragging a scroll bar, releasing touches on an apple mMultitouch trackpad, and when lifting your finger from the surface of an Apple magic mouse.)
Flags: needinfo?(kgilbert)
Ok, that sounds like a bug in our implementation then, and it also means that the patch is making an assumption that may not hold.
Comment on attachment 8632343 [details] [diff] [review]
bug1180030.patch

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

Minusing for now based on the above discussion. While this change will be needed, I think by itself it is not complete and we'll need to a RequestFlingSnap at some point during the overscroll animation as well.
Attachment #8632343 - Flags: review?(bugmail.mozilla) → review-
(In reply to Chris Lord [:cwiiis] from comment #14)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> > (In reply to Botond Ballo [:botond] from comment #10)
> > > That said, I would expect we don't need a fling snap when the overscroll
> > > animation completes because the scrollable element will be at one of the
> > > extremes (beginning or end) of its scrollable range.
> > 
> > Is it always guaranteed that the extremes will be legal scroll positions?
> > What if somebody sets a mandatory snap point that's not at the beginning/end
> > of the scrollable range? If it's not guaranteed then it seems like we should
> > be doing a snap request such that it kicks in after the overscroll stretch
> > animation but before the snap-back animation.
> 
> I think ideally it'd have the overscroll animation if and only if the extent
> of the scroll axis is a legal position, but if the next legal position is
> before the extent of the container, it would just snap back to that point
> like it does now.

I think both of these suggestions will require that the compositor know whether the extents are legal positions, which is something it currently doesn't know.
Kats pointed out that he had in mind unconditionally requesting a fling snap, but at a later point than when we currently send it (for example, once the overscroll effect has stretched the page).

This doesn't require the compositor knowing whether the extents are legal positions, but it means we'll still get a scrollTo that may cut off the overscroll bounce before it gets a chance to complete.
Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats
Attachment #8640695 - Flags: review?(bugmail.mozilla)
Bug 1180030 - Request a fling snap when an overscroll animation reaches its oscillation phase. r=kats
Attachment #8640696 - Flags: review?(bugmail.mozilla)
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

(Not asking for review yet.)
Attachment #8640695 - Flags: review?(bugmail.mozilla)
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

I implemented Kats's suggestion of requesting a fling snap when the overscroll animation enters its oscillation phase. The results are what I expected: on a page with scroll snapping, the overscroll animation is cut off some amount of time into the oscillation phase, with the exact amount of time varying from one time to another. Not a great experience, but perhaps better than what we have now. (In particular, the "stretch" phase of the overscroll animation consistently gets to complete.)

Chris, would you be able to apply these patches and see if you think the resulting look/feel is acceptable?
Attachment #8640696 - Flags: review?(bugmail.mozilla) → feedback?(chrislord.net)
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

This doesn't really look or feel any better, ideally we want the bounce effect if the target scroll position doesn't change after the snap. I'm not sure there's going to be another solution that will feel adequate.
Attachment #8640696 - Flags: feedback?(chrislord.net) → feedback-
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats
Attachment #8640696 - Flags: feedback-
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

Bug 1180030 - Request a fling snap when an overscroll animation reaches its oscillation phase. r=kats
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

Here's the alternative approach we discussed on IRC, where we request the fling snap when the overscroll bounce completes. Better?
Attachment #8640696 - Flags: feedback?(chrislord.net)
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

So the first overscroll I did had the effect, but after that it just snaps back like before. I think there might be something not quite right with the implementation?
Attachment #8640696 - Flags: feedback?(chrislord.net)
botond: Not sure if you got my IRC message, but I made sure that both patches were applied, and I still didn't get the overscroll oscillation effect.
Thanks for testing, Chris. I will investigate.
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats
Attachment #8640696 - Attachment description: MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation reaches its oscillation phase. r=kats → MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

Chris and I discussed this on IRC, and it sounded like the issue he was running into was that if the finger leaves the screen while overscrolled and the velocity is zero, the Part 1 patch would compute flingWillOverscroll as false and not suppress the fling snap request, resulting in no overscroll bounce animation. The updated patches fix this. 

Third time's the charm?
Attachment #8640695 - Flags: feedback?(chrislord.net)
Let's get this reviewed and landed. If Chris discovers any additional issues, we can address them in a follow-up.
Attachment #8640695 - Flags: feedback?(chrislord.net) → review?(bugmail.mozilla)
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats
Attachment #8640696 - Flags: review?(bugmail.mozilla)
Comment on attachment 8640696 [details]
MozReview Request: Bug 1180030 - Request a fling snap when an overscroll animation completes. r=kats

https://reviewboard.mozilla.org/r/14361/#review14533

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:664
(Diff revision 3)
> +      mApzc.OverscrollAnimationEnding();

Stick in a "return false" inside this if condition, and a "return true" after it.
Attachment #8640696 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8640695 [details]
MozReview Request: Bug 1180030 - Do not request a fling snap if the fling will overscroll on its first sample. r=kats

https://reviewboard.mozilla.org/r/14359/#review14535
Attachment #8640695 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/78a12bc7e2da
https://hg.mozilla.org/mozilla-central/rev/8b01510ab031
Assignee: nobody → botond
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: