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

Auto Reinitialising - Using event based updates to remove polling overhead #341

Closed
wants to merge 3 commits into from
Closed

Conversation

ashley-hunter
Copy link
Contributor

Currently autoreinitialise uses an interval (defaulting to every 500ms) to check to see the content has changed size and if so update the scrollbars. This is very inefficient, especially if we require the scrollbars to update quickly when size changes, and it is well documented that we should avoid where possible using low autoreinitialise delays.

Instead, we can reinitialise once we detect the content changing size using events, instead of polling. So now, the component will sit idle until the browser fires an event, and only then will we reinitialise, making it much more efficient and will avoid running size checks that can cause reflow when they are not needed.

@vitch
Copy link
Owner

vitch commented Sep 8, 2016

Thanks for this PR - it would certainly be great to not have to rely on an interval to monitor resizing.

Looking through the code, I see it adds listeners for "scroll" events but I don't see anything for "resize". Does it respond correctly when the browser window changes size? Looking at resize-sensor which your code references it says that it detects when "anything inside of element caused my size to change" - what about when something else on the page causes the size of the element to change?

Maybe you could push an example of this is action somewhere so I could view it?

@ashley-hunter
Copy link
Contributor Author

I have made a further change to account for when the containing element is resized, so even though we are not watching for a resize event specifically, if the container element is resized it will trigger a scroll event causing a reinitialise.

I will try to put together an example sometime soon.

@SerdarSanri
Copy link

I also found out that, reinitialiseInterval part at line 1348 is no longer needed as well.

// clear reinitialize timer if active if (reinitialiseInterval) { clearInterval(reinitialiseInterval); }

@illuusio
Copy link
Collaborator

Do you want to rework this to changed upstream

@gitbreaker222
Copy link

gitbreaker222 commented Apr 5, 2018

@ashley-hunter I stumbled upon an old project, that has to be upgraded for mobile devices and really could benefit from your improvement. Is it possible for you to resolve the conflicts?

(sidenote on performance: the constant reinitialising adds in my case 200 DOM-Nodes and 100 JS-event-listeners per second, causing a constant style-recalcs-overhead with almost 20% CPU usage)

@illuusio
Copy link
Collaborator

illuusio commented Apr 6, 2018

Good side note. I have to look at this how much work this takes to 'Next' level

@ashley-hunter
Copy link
Contributor Author

We have implemented this in our project and been using it for quite some time with good success. I have made some changes since this PR was created. We added an additional settings option called resizeSensor which if enabled would supersede autoReinitialise - bringing significant performance improvements.

The resize sensor will work for almost all use cases, however if there are absolutely positioned elements within the scroll pane then you may still need to use autoReinitialise.

I don't have time right at the moment to rework these changes and update the documentation etc.. but you can find the changes we have made here if it is of any help to anyone (note there are additional modifications in this file not related to resize sensor):

https://github.com/UXAspects/UXAspects/blob/develop/src/ng1/plugins/customscroll/jquery.jscrollpane.js#L317

@illuusio
Copy link
Collaborator

illuusio commented Apr 7, 2018

Do someone have time to review those changes how much work do they need to get in? @gitbreaker222?

@gitbreaker222
Copy link

two things:

  1. I have built my own little workaround and went from
  $( function() {
  	$( '#itemlist' ).jScrollPane({
  		showArrows: true,
  		verticalArrowPositions: 'split',
  		horizontalArrowPositions: 'split',
  		verticalDragMaxHeight: 100,
  		autoReinitialise: true
  	});
  });

to

 var initJScrollPane = function() {
     $( '#itemlist' ).jScrollPane({
         showArrows: true,
         verticalArrowPositions: 'split',
         horizontalArrowPositions: 'split',
         verticalDragMaxHeight: 100,
         autoReinitialise: false
     });
 }
 $(initJScrollPane);
 window.addEventListener('resize', initJScrollPane)
  1. looking at the diff of scrollpane.js ln.:336 it seems to me, that this could be improved by using resize instead
resizeGrowElement.addEventListener('scroll', onGrow.bind(this));
resizeShrinkElement.addEventListener('scroll', onShrink.bind(this));

Maybe the whole block there could be greatly simplified, if the helper-size-divs were obsolete then. But I do have noticed, that resize should be combined with e.g. getAnimationFrame which in turn would limit IE compatibility to IE10 … if this is a thing 🤷‍♂️

maybe I should try to set up a demo on codepen or so…

@ashley-hunter
Copy link
Contributor Author

ashley-hunter commented Apr 17, 2018

@gitbreaker222 window resize listener may suffice for your use case, in which case it is certainly the simpler option at the moment, it'll definitely be a big improvement over autoreinitialise.

The additional case is when the scrollpane is resized but the window isn't, for example if I have a panel that opens/closes, eg:

So the window resize event doesn't quite do the same as the resize helper divs, instead they are more like the Resize Observer but browser support is currently very poor. The helper divs are essentially a polyfill for the ResizeObserver that doesn't use timeouts/requestAnimationFrame etc..

@illuusio
Copy link
Collaborator

I'm currently under heavy burden but next week I try to find time to properly check this out. This is good conversation.

@ashley-hunter
Copy link
Contributor Author

I have updated the code and create a new PR: #361

Closing this PR in favor of the new one

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

Successfully merging this pull request may close these issues.

5 participants