Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scroll bounce #315

Merged
merged 19 commits into from Feb 13, 2019
Merged

Scroll bounce #315

merged 19 commits into from Feb 13, 2019

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented Feb 8, 2019

Attempt to implement scroll bounce in ScrollView. Mostly based on https://github.com/atomiks/elastic-scroll-polyfill.

Part of #288.

The PR includes some steps towards a functional API for animations, with functions like tweet and chain, based on ideas from Pure. Totally experimental. 😄

It also includes a Bezier implementation that is targeting to emulate the CSS cubic-bezier with 4 params. But it's not working as it should yet, it jumps a little bit too much 😅

@bryphe One of the things that this PR shows is the need to differentiate on macOS between "user-generated wheel events" and "momentum-generated wheel events". As shown in the gif below, right now there's no way to know if an event is happening because of the user or not, which leads to the bouncing animations being triggered twice. It can't be solved with timeouts because the duration of the system momentum varies depending on the speed of the original event. So it seems we'll have to upgrade reason-glfw to include the momentumPhase you mentioned in the original issue. Maybe I could try to add that, as it seems a blocker for this PR.

scroll-bounce

TODO:

  • Fix cubic bezier jumpy behavior
  • Polish the Animation API code
  • Figure out how to run AnimationTest (is there a script for that?) and add more tests
  • Add a prop bounce to ScrollView (open to API suggestions)
  • Stop bouncing if a user-generated event is detected
    - [ ] Explore the "stretching" / rubber band effect (maybe for another PR) Edit: deferred until Expose momentum property for scroll events glfw/glfw#1429 is available

@wokalski
Copy link
Collaborator

wokalski commented Feb 8, 2019

@jchavarri I just remembered that Ole Begemann once attempted to reimplement ScrollView and people took his concept further by adding inertia, bouncing, and rubber-banding. More information here:
https://oleb.net/blog/2014/05/scrollview-inertia-bouncing-rubberbanding/

EDIT: It's about iOS but should give you a good idea how to do it. If you have any problems with reading the syntax, just write me on discord 😃

playback;
};

module Chain = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, we really needed the ability to chain animations! Very cool 👍

src/UI/Animation.re Outdated Show resolved Hide resolved
| Transitioning => ()
| Idle when isAtTop || isAtBottom =>
open Animated;
setBouncingState(Transitioning);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed with the current behavior is that while the scroll is in 'Transitioning' (ie, its bouncing), it doesn't respond to any additional scrolls. Perhaps that is by design?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryphe Good catch, this was bugging me.

The code I was using as reference relies on the browser also updating the scrollTop property when the user interrupts the animation. It checks the current value againts the value stored in the previous iteration, and if they are different, it supposes the event must come from the user scrolling (in that "stateful DOM" way so common in the old jQuery days 😄 ).

Unfortunately that solution is not available for us because we own the whole scrolling process, but after reading your comment, it made me think that we could store a direction inside the Transitioning state value, like:

type direction = Top | Bottom; // | Left | Right could be added later
type bouncingState = Idle | Transitioning(direction)

That way, we can read the deltaY and if it's going in a different direction that the bouncing was originally started, we go back to Idle. I'll play around this idea to see what it gives.

This is another case where the information about the nature of the event (user or momentum) would be really helpful :) Hopefully we can figure that out.

Another thought is that momentum doesn't exist in the browser 🤔 so maybe a solution that doesn't rely on momentum is actually desirable...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryphe I have implemented the idea above in ed68875. It seems to work ok, but it's still mostly a hack 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchavarri - nice! I just tried it out and it does feel better (it felt 'sticky' before when it was bouncing) - the hack sounds reasonable to me 👍

Another thought is that momentum doesn't exist in the browser 🤔 so maybe a solution that doesn't rely on momentum is actually desirable...

Yes, interesting, I just looked around too, to see if this is doable... The closest thing I found was this library lethargy which seemed to implement some pretty intense logic to reverse-engineer if the event coming in is due to momentum. Certainly not straightforward and seems like a hack really (ie, misinterprets some cases as inertial: d4nyll/lethargy#14)

@bryphe
Copy link
Member

bryphe commented Feb 9, 2019

This is awesome @jchavarri , thank you! 😄 So cool seeing it work.

@bryphe One of the things that this PR shows is the need to differentiate on macOS between "user-generated wheel events" and "momentum-generated wheel events"

Ah, interesting, I was wondering if we would hit this. Yes, it seems we might have to augment the _glfwInputScroll to add a momentum property... I logged an issue on GLFW to track and see if this has been thought about: glfw/glfw#1429

  • Figure out how to run AnimationTest (is there a script for that?) and add more tests

Ah sorry this wasn't more clearly documented. You can run the tests right now with esy b dune runtest - it looks like a few might need to be updated with the new API.

  • Add a prop bounce to ScrollView (open to API suggestions)

Perhaps we could have a bounceOptions that is configurable (time for bounce away/back?) - and then have some default options (perhaps per-platform - we could make Windows/Linux shorter time than OSX)?

Alternatively - we could just have bounce = false|true 🤔 (and again - we could set per-plat defaults - Windows/Linux = false and OSX = true).

Explore the "stretching" / rubber band effect (maybe for another PR)

Neat, this would be fun to try!

@bryphe bryphe mentioned this pull request Feb 9, 2019
@jchavarri
Copy link
Member Author

Hey @bryphe I added a few things to the PR:

  • Ended up pulling the cubic bezier logic out on its own repo, as I was having some trouble with it and wanted to properly test it etc. there are a lot of complex functions 😅 I think the algorithm is working fine now, I put it in https://github.com/jchavarri/rebez, but I'm fine with moving it to revery-ui org if you prefer :) It's the very barebones cubic bezier implementation, like CSS cubic-bezier. It might help other folks out there I guess.
  • Fixed the animation blocking the scroll, by using the "direction" hack mentioned above
  • Fixed the tests and added some more to cover the animation chaining

Perhaps we could have a bounceOptions that is configurable (time for bounce away/back?) - and then have some default options (perhaps per-platform - we could make Windows/Linux shorter time than OSX)?
Alternatively - we could just have bounce = false|true 🤔 (and again - we could set per-plat defaults - Windows/Linux = false and OSX = true).

I feel inclined to have just bounce as a boolean for now. If somehow the bouncing animation is not right, I think it should be fixed for every user of Revery 🤔 What do you think?

Regarding setting its value depending on the OS, I'm not sure where the getOs function should live. I see there is one in discover.ml. I wonder if it should go in Platform? or in some other module?

Once that's fixed, I think that would be it for this PR, if you test it and don't see any other issues with the animations / bouncing state logic. Unfortunately I don't think we can implement the "stretching" without that extra momentum information, see atomiks/elastic-scroll-polyfill#3.

There's a sad part of the story: even if we get this to work natively, I don't think it'll be possible to add it in JavaScript because in that case the wheel events don't carry this information. But at least the bouncing can be there 😄

@bryphe
Copy link
Member

bryphe commented Feb 12, 2019

  • Ended up pulling the cubic bezier logic out on its own repo, as I was having some trouble with it and wanted to properly test it etc. there are a lot of complex functions 😅

Cool! That makes sense 😅 I think the cubic bezier logic is something that would be generally useful beyond Revery.

Awesome, starred! I'm fine either way - I think you have a permission to create repositories in revery-ui as you wish - but it's nice to have your name associated with it too, since you did the entirety of work for it. Either way, we could also call it out as one of the libraries we depend on at the bottom of our README.

I feel inclined to have just bounce as a boolean for now. If somehow the bouncing animation is not right, I think it should be fixed for every user of Revery 🤔 What do you think?

This sounds reasonable to me, it's simpler for sure.

Regarding setting its value depending on the OS, I'm not sure where the getOs function should live. I see there is one in discover.ml. I wonder if it should go in Platform? or in some other module?

Platform seems OK, since it deals with native integration - the only other alternative I can think of is Environment. Likely we'll need to reconcile these and bring them together at some point - so either place would be fine with me.

Once that's fixed, I think that would be it for this PR, if you test it and don't see any other issues with the animations / bouncing state logic. Unfortunately I don't think we can implement the "stretching" without that extra momentum information, see atomiks/elastic-scroll-polyfill#3.

Sounds great! Once we have native momentum information, we can explore that effect 👍

There's a sad part of the story: even if we get this to work natively, I don't think it'll be possible to add it in JavaScript because in that case the wheel events don't carry this information. But at least the bouncing can be there 😄

Bummer, ya, it does not seem like we really get that information! Might be a concession we have to make on the Web side... but that's OK. The only option I can think of is perhaps we could disable the momentum scroll to false - like with -webkit-overflow-scrolling and treat the events like there is no momentum (and apply our own). Not sure if this would help us though, and we can track separately.

Thanks for all the work on this! Very cool to see it come together!

@bryphe bryphe mentioned this pull request Feb 12, 2019
@jchavarri
Copy link
Member Author

@bryphe updated the PR with the last remaining parts:

  • a function Environment.os that gets the current OS
  • a prop bounce with default value true only in MacOS

I added a checkbox to the ScrollView example with the idea that the examples evolve over time to a kind of storybook 😄 but we can remove it if you prefer to add all props in a single PR in the future.

scroll

@jchavarri
Copy link
Member Author

Oh, and thanks for the esy version update! Tests seem to be passing now 🎉

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @jchavarri ! I like the 'Bounce' option you added in the Example 👍

@bryphe
Copy link
Member

bryphe commented Feb 13, 2019

Awesome @jchavarri , thank you!

  • a function Environment.os that gets the current OS

Perfect! I tried this out though and got a failure on Windows (uname isn't an available command). I figured this would be easy for me to fix + test since I'm on Windows, so I hope you don't mind - I went ahead and added this tweak in 9bd1473

  • a prop bounce with default value true only in MacOS

Nice!

I added a checkbox to the ScrollView example with the idea that the examples evolve over time to a kind of storybook 😄 but we can remove it if you prefer to add all props in a single PR in the future.

This is great! We can always revisit for now. But I like that it makes it explicit that we have this functionality, and we can demo it on any platform.

@bryphe
Copy link
Member

bryphe commented Feb 13, 2019

I also fixed a merge issue in the lockfile. I'll bring this in as soon as the next round of builds are green; I'm planning on merging #331 shortly and don't want you to have to deal with merge conflicts on this PR.

Thanks for all the work on this, @jchavarri !

@bryphe bryphe merged commit 70b772a into revery-ui:master Feb 13, 2019
@jchavarri jchavarri deleted the bounce-scroll branch February 13, 2019 10:38
akinsho pushed a commit to akinsho/revery that referenced this pull request Feb 18, 2019
* Adding `tween` to Animation, start changing types towards functional API

* Adding Chain module and first buggy bouncing back animation

* cleanup

* add cubicbezier

* remove logs

* Making chains a list. Allowing to interrupt bouncing animations if scrolling on the opposite direction.

* Using rebex and fixing timing for chained animations. improving the general feel.

* Fix tests

* Point rebez to github

* remove link-dev

* esy updates

* Adding bounce prop and Environment.os

* refmt

* Fix platform detection on Windows

* Formatting

* Fix line ordering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants