937335 - Cannot "catch" scroll bounce

Status: RESOLVED INVALID

(Core :: Widget: Cocoa, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So recently we landed code (at least for OSX) where if you keep trying to scroll up when you are already at the top, the page is sticky with your fingers and slides down a little way, but then slides back up to the top of the screen.

Safari also does this, however if you drag the page down, let your finger up off the trackpad so that the page starts to slide back into place, and then put your fingers quickly back on the trackpad before it reaches the top, you can "catch" the page and drag it down a bit again. With Firefox, you can't "catch" the page.

Being able to do this makes the UX much more natural, like you are dealing with a physical thing. We should make it possible.

Comment 1

[Stephen A Pohl [:spohl]]

I'm unable to 'catch' the page in Safari 7.0. Could you please specify what OS version you're on and what version of Safari you're using? For me, Safari and Firefox behave the same.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I can repro on Safari 7 and OSX 10.9

Comment 3

[Stephen A Pohl [:spohl]]

When you scroll down in Safari, release the trackpad and put your fingers right back on the trackpad without moving them, are you able to catch the page and stop it from going back to the top?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I guess if you just touch you fingers back down, it will continue to slide back. You have to keep trying to scroll past the top. Sorry about the slightly incorrect STR.

Comment 5

[Stephen A Pohl [:spohl]]

Ah, I see now. I guess my fingers aren't fast enough to repro this very well. :-)

There's a comment about repeated vertical scrolls here [1]. We would likely fix this in an |else if| block that follows the |if| block here [2]. Currently, we intentionally restart subsequent scrolls at 0. It may take me a while to get to this myself, but I'd be happy to mentor someone if that would be helpful.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-gestureSupport.js#631
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-gestureSupport.js#634

Comment 6

[Thomas Marlette]

Hi there,

I would like to work on this bug.
How do we setup this mentor business?

Comment 7

[Stephen A Pohl [:spohl]]

Hi Thomas, sorry about the delay. There isn't a very formal mentoring process that I'm aware of. You're basically free to develop a patch that fixes the issue reported here. I've added my thoughts in comment 5 which should get you started. You can attach your patch(es) to this bug and request feedback from me.

I'll assign this bug to you.

There's a good wiki page for instructions on how to build Firefox locally [1]. Another one describes the use of Mercurial [2]. You can also catch me on IRC if you have any questions.

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mercurial

Comment 8

[Thomas Marlette]

Hey sorry for the delayed response, my e-mails were getting redirected to the trash.

I have looked at the code but I actually cannot reproduce the scrolling action in Firefox.  I am using OSX Mountain Lion and I cannot scroll in any fashion?  Is there some trick to doing this?

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to Thomas Marlette from comment #8)
> Is there some trick to doing this?

Right, I forgot to mention this: the feature has been turned off by default while we work out the remaining issues (see bug 860493). You can enable the feature by setting browser.snapshots.limit in about:config to a value greater than 0 (5 will most likely become the default).

Comment 10

[Thomas Marlette]

Hey Stephen,

I changed the value of browser.snapshots.limit and I am still not able to perform a vertical scroll.  I am, however, able to do a horizontal scroll, which I think is a history swipe?

Is this configuration intended to enable the vertical scroll or do I need to do something else?

Comment 11

[Stephen A Pohl [:spohl]]

Yes, you're right again, sorry. browser.snapshots.limit is required, but vertical overscroll was also turned off due to bug 939480. You can apply the backout patch from bug 939480 and vertical overscroll should/will start to work.

Comment 12

[Thomas Marlette]

Hi Stephen,

I can't seem to figure this out.

Based on your comments and the code it seems that updateAnimation() dictates how the box should move by doing translations.  The magnitude of the translations are based on the input to updateAnimation, aVal and some dampening value to lessen the dampening.

I have no idea if the above is right but that's what I have figured so far. 
My main issue is that I am not sure how to go about implementing the catch.

Here is what I think I need to do:  When there is a vertical swipe succeeding a vertical swipe, set updateAnimation as the current position of the box (in hopes to halt movement instead of resetting it).

However, I do not know how to get the current position of the box.  
Is there any advice that you can give me on this?  I have not worked with too many animations and am not sure if my approach is correct in the slightest.

Also can you recommend a good way to debug these types of things?  I have not done much work on browsers or web programming at all and am not sure the best way to go about debugging in this environment.  My current method is change code that I think will work and rebuild.

Thanks,
Thomas

Comment 13

[Stephen A Pohl [:spohl]]

(In reply to Thomas Marlette from comment #12)
> Based on your comments and the code it seems that updateAnimation() dictates
> how the box should move by doing translations.  The magnitude of the
> translations are based on the input to updateAnimation, aVal and some
> dampening value to lessen the dampening.
> 
> I have no idea if the above is right but that's what I have figured so far. 
> My main issue is that I am not sure how to go about implementing the catch.

Your understanding is correct!

> Here is what I think I need to do:  When there is a vertical swipe
> succeeding a vertical swipe, set updateAnimation as the current position of
> the box (in hopes to halt movement instead of resetting it).

This is correct, with one small but important thing to note: calling updateAnimation with a value that results in the animation to be at the current position is equivalent to not calling updateAnimation at all. :-) In other words, I strongly suspect that all you need to do is add an else statement with a simple function return for the if clause here:
http://hg.mozilla.org/mozilla-central/annotate/52c1eacc3f77/browser/base/content/browser-gestureSupport.js#l629

You will want to:
1. Make sure that this actually fixes the bug by comparing with Safari.
2. Update the comment for the if statement.
3. Write proper header for your patch and upload it here.

> Also can you recommend a good way to debug these types of things?  I have
> not done much work on browsers or web programming at all and am not sure the
> best way to go about debugging in this environment.  My current method is
> change code that I think will work and rebuild.

This may be very general, but here we go for the purpose of completeness: In compiled code, you can debug things by either logging or debugging in a debugger. Personally, I like the command line gdb on OS X for debugging. If you need to log things in Objective-C files, you can log via NSLog. Use printf in C/C++.
In front-end JavaScript code it is sometimes enough to just throw up an alert box with the info that you need. If this isn't sufficient you can refer to the JavaScript debugging MDN page for different/better ways of debugging: https://developer.mozilla.org/en-US/docs/Debugging_JavaScript

Comment 14

[Thomas Marlette]

Macbook died, so I was not able to work on this for a long time.

Will look at it over the next few days or so.

Tried fixing with fixes suggest by Stephen, but it did not seem to correct the scrolling issue.  I will make sure and verify, and look into why it is not fixing it

Comment 15

[Thomas Marlette]

Please reassign this bug.  I currently do not have a Mac.

Comment 16

[Shivanker Goel]

Hi,
I would like to work on this bug, and this is my first bug. I already have my dev environment set up.
I followed the steps above to enable vertical overscroll (changing browser.snapshots.limit to 5, and applying the backout patch from bug 939480) but still couldn't perform a vertical overscroll.

If this bug is still relevant, and is a good first bug, could you guide me in reproducing and fixing this?

Comment 17

[Stephen A Pohl [:spohl]]

Thanks, Shivanker! The bounce behavior is transitioning to APZC (Asynchronous Pan/Zoom Controller) and as such, I get the feeling that this bug may not need fixing anymore for the current implementation. Markus, could you confirm?

Comment 18

[Markus Stange [:mstange]]

Correct, it's not worth fixing the current implementation. But in fact the APZ overscroll implementation currently has a similar problem (on Firefox OS, which is the only place we're using it at the moment), and that is getting fixed in bug 1042103.

Comment 19

[Markus Stange [:mstange]]

Taking this off the "good first bug" list since the proper fix needs to wait for more complicated work to happen first.

Comment 20

[Shivanker Goel]

Alright, thanks Stephen, Markus.

Comment 21

[Jim Mathies [:jimm]]

kats, is this something apz will fix generally at some point?

Comment 22

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

Yeah, we'll make sure to do this as part of the APZ implementatation. Right now we don't support overscroll on OS X.

Comment 23

[Stephen A Pohl [:spohl]]

The code in question here has been removed in bug 860493.