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

Provide option to disable throttling/set custom timing #507

Closed
b-strauss opened this issue Aug 31, 2020 · 10 comments
Closed

Provide option to disable throttling/set custom timing #507

b-strauss opened this issue Aug 31, 2020 · 10 comments

Comments

@b-strauss
Copy link

b-strauss commented Aug 31, 2020

simplebar has a throttling on the recalculate function which leads to flickering in situations where the simplebar has a max-height and there are animations inside the container that cause a resize.

tested on chrome 85.0.4183.83

resize

As you can see, as soon as the max-height is reached no more flickering is happening.

I assume this comes from the fact that there is a hardcoded throttling with 64 ms after the resize observer. Could you maybe add an option to disable the throttling, or even better allow people to use their own throttling mechanism?

This way people could control how to throttle.

Example with requestAnimationFrame:

// options
{
  createThrottle: (func, delay) => {
    let isRunning = false;
    return function (...args) {
      if (isRunning) {
        return;
      }
      const context = this;
      isRunning = true;
      window.requestAnimationFrame(() => {
        func.apply(context, args);
        isRunning = false;
      });
    };
  },
}
@Grsmto
Copy link
Owner

Grsmto commented Sep 2, 2020

This seems more like a bug. I don't see why the horizontal scrollbar would show if you're changing only the height.
Is your scrollable area scrolling on both horizontal and vertical? Or only vertical?

I might be wrong but I don't think this has anything to do with the throttling here.

@b-strauss
Copy link
Author

b-strauss commented Sep 2, 2020

This has nothing to do with the horizontal scrollbar. That was a bug in the layout (that is already fixed), where the corners of a rotating element was sometimes outside of the bounds of the scroll container.

Simplebar uses a ResizeObserver to watch for the height change.

https://github.com/Grsmto/simplebar/blob/master/packages/simplebar/src/simplebar.js#L305-L316

But the recalculate function is throttled to only trigger every 64 milliseconds.

https://github.com/Grsmto/simplebar/blob/master/packages/simplebar/src/simplebar.js#L55

Which means that the recalculate function is called at a lower rate than the repainting of the browser.

For a 60Hz monitor (16,6ms), this means there are 4 missed frames.
For a 144Hz monitor (6,9ms), this means there are 9 missed frames.

I suggest to use a throttling that is based on requestAnimationFrame.

@Grsmto
Copy link
Owner

Grsmto commented Sep 3, 2020

I don't understand what you gif is showing then lol.

I get it that some frames are missed but the recalculate function is only there to define if the scrollbar is required or not, and its size. These stuff don't really need to be perfectly synced with the UI. Or why do you think they should be?

I think I just don't understand your problem. If you could provide a replicable example that would be really helpful! Thanks

@b-strauss
Copy link
Author

b-strauss commented Sep 3, 2020

Here's a reproduction.

This example has a SimpleBar container that has a min-height and a max-height. The content element of the SimpleBar container has an animated height. There are 4 buttons to toggle the content height. xsmall and small makes the content smaller than the SimpleBar container, the animation is smooth. Animating between small and medium clearly shows that there is stuttering. When the size is set to large the max-height is reached and SimpleBar starts scrolling the content.

https://jsfiddle.net/p0v8Ltk1/6/

In the second example i copied the SimpleBar code and removed the throttling of the recalculate function.
Everything is smooth now:

https://jsfiddle.net/p0v8Ltk1/8/

Our customers are using iPad Pros that have a refresh rate of 120Hz. On screens like this the stuttering is even more visible than on 60Hz monitors. But its visible on both.

Our solution was to fork the code and include a throttling that is based on requestAnimationFrame.

Like i said. I would suggest to change it to a requestAnimationFrame based solution, or to provide a way to include a costum throttling mechanism.

Thanks 😃

Edit (Animated versions):
stutter: https://jsfiddle.net/qky4r3ev/
no stutter: https://jsfiddle.net/qky4r3ev/2/

@Grsmto
Copy link
Owner

Grsmto commented Sep 4, 2020

Thank you very much for the reproductible examples. This makes a lot of sense now!

I think I would accept a PR with your changes using RAQ if you want to do it. Otherwise i'll just do it when I can.

Thanks again, great stuff. Can't quite believe this was never reported before. 😅

@b-strauss
Copy link
Author

Great thanks.

Are you going to release this with version 5?

@Grsmto
Copy link
Owner

Grsmto commented Sep 11, 2020

Yes this should be released in the latest beta simplebar@6.0.0-beta.4

@b-strauss
Copy link
Author

Would it be possible to do a bugfix release for version 5?

When are you planning to release version 6?

@Grsmto
Copy link
Owner

Grsmto commented Oct 2, 2020

Sorry I just realised I didn't read your last question properly.

I wish I had more time to work on this and release it as a patch and on the new version as well but with the number of issue in the repo I don't :(
To partially solve this issue I added an tool that can publish a new version from a PR, so you could be able to send your PR and it would publish it automatically. I didn't test so I'm not sure it works yet tho.

For v6 tbh I don't know, I don't have a specific date for it but I have one last issue I want to solve before releasing it. Hopefully next week 🤞

@Grsmto
Copy link
Owner

Grsmto commented Jan 19, 2023

This should be released now. Thanks!

@Grsmto Grsmto closed this as completed Jan 19, 2023
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

No branches or pull requests

2 participants