WindowScroll IS_SCROLLING_TIMEOUT in dev-mode

[djeeg]

Hi Brian,

I haved noticed that in development <WindowScroll> lags a fair bit. Digging into a bit further, I found that a single action (eg a scroll arrow click) in both IE and Firefox triggers multiple onScroll events, whereas Chrome only triggers a single event.

Adding some traces to here:

https://github.com/bvaughn/react-virtualized/blob/master/source/WindowScroller/utils/onScroll.js#L39

function onScrollWindow(event) {
  console.log("-------onScrollWindow", new Date().getTime(), event.currentTarget);

These are example timings i see:

chrome
-------onScrollWindow 1499028035417 
  
ie11
-------onScrollWindow 1499028051106 
-------onScrollWindow 1499028051228
-------onScrollWindow 1499028051382

What happens then, for each scroll event, I seem to get 4 re-paints, in this order:

• isScrolling = true
• isScrolling = false
• isScrolling = true
• isScrolling = false

Some suggest throttling the event:
https://stackoverflow.com/questions/22018348/window-scroll-events-triggering-twice/22018607#22018607

Though I have been playing around with something like this

export var IS_SCROLLING_TIMEOUT = (process.env.NODE_ENV !== 'production') ? 500 : 150;

Is there a better way to configure this when rendering is slower in dev-mode?

[bvaughn]

What happens then, for each scroll event, I seem to get 4 re-paints, in this order

This sounds suspicious. If 3 scroll events are fired, I would expect 4 renders: isScrolling:true x3, isScrolling:false x1.

[bvaughn]

Can you please confirm that you're actually seeing the true/false/true/false sequence?

That is not what I would expect, and if I'm right- changing the is-scrolling timeout shouldn't make any difference on how frequently IE11 fires scroll events.

If I'm incorrect, could you provide some sort of Plnkr repro case that I could see?

[djeeg]

What I mean to say, was for every scroll action (aka a user click), I get multiple window.scroll events.
Which is apparently a known feature of some browsers.
Note: each window.scroll event does only seem to trigger a single repaint (isScrolling = true), no issue here

The issue comes when the "unscroll" timer code triggers
It causes a repaint (isScrolling = false), which is normally fine in production mode, or with simple virtualised content.
If however, the render time of each repaint is slower than 150ms, which is is on DEV, then the timer will fire (isScrolling = false), only to have the next window.scroll event fire, causing another round of (isScrolling = true)(isScrolling = false), draining performence.

Its probably only a problem because I am displaying simple content when isScrolling=true and complex content when isScrolling=false, the list is constantly flicking between simple/complex cells

Let me put together a sample when i get home

[bvaughn]

If however, the render time of each repaint is slower than 150ms, which is is on DEV, then the timer will fire (isScrolling = false), only to have the next window.scroll event fire, causing another round of (isScrolling = true)(isScrolling = false), draining performence.

This is the part that still doesn't make sense to me. Why would a re-render cause another scroll event to fire? Why would scroll events be > 150ms apart?

I think I'm going to need to see a repro.

[djeeg]

Apologies Brian, I found the underlying cause, my scroll events were taking too long because I had too many synchronous console.log statements littered across the cell render methods. Once I started buffering the console.log statements, then the scroll handler time dropped from >500ms to ~30ms, well within the 150ms threshold.

This caught my eye when I was debugging in Chrome
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3tViQmRF2l8/discussion

Whats your opinion on including performance related warning messages within your lib? Something like this:

var haswarnedscrollperf = false;
function onScrollWindow(event) {
  var time1 = new Date();
  //...existing logic...
    var time2 = new Date();
    if(process.env.NODE_ENV !== 'production') {
      if(!haswarnedscrollperf) {
          var diff = time2.getTime() - time1.getTime();
          if (diff > IS_SCROLLING_TIMEOUT) {
              console.warn("react-virtualised WindowScroller scroll handler took longer than IS_SCROLLING_TIMEOUT [" + (IS_SCROLLING_TIMEOUT) + "ms]", diff + "ms");
              haswarnedscrollperf = true;
          }
      }
    }
}

[bvaughn]

No problem. I'm glad you figured out what was going on. 😄

Hm. I'm reluctant to add non-RV-specific warnings to RV b'c the library is already pretty hard for a single person to maintain. Specifically the issue you mention above- how would I distinguish between 2 separate scrolls that occur closely together (eg scroll, stop, scroll) vs a scroll with long-running renders like you mention?

I think it's okay to close this issue now, given your most recent findings. We can continue to discuss here though. I'd also happily review any proposals (PRs) you have for improving the dev-messaging regarding this use case.

[djeeg]

Hi Brian, sorry to revive this, I think I have a solution.

I was digging into the Grid code and stumble upon it's debounce logic that is very similar to the logic in WindowScroller, however Grid's debounce has a configurable timeout.

react-virtualized/source/Grid/Grid.js

Line 196 in 67f9dc5

scrollingResetTimeInterval: PropTypes.number,

This is exactly what I need for the WindowScroller

I have been prototyping and found i can set this new prop

<WindowScroller scrollingResetTimeInterval={500}>

And use it like this

function enablePointerEventsAfterDelay() {
  if (disablePointerEventsTimeoutId) {
    clearTimeout(disablePointerEventsTimeoutId);
  }

  var maxtimeout = IS_SCROLLING_TIMEOUT;
    mountedInstances.forEach(function (instance) {
      if(instance.props.scrollingResetTimeInterval) {
          if(instance.props.scrollingResetTimeInterval > maxtimeout) {
              maxtimeout = instance.props.scrollingResetTimeInterval;
          }
      }
    });

  disablePointerEventsTimeoutId = setTimeout(enablePointerEventsAfterDelayCallback, maxtimeout);
}

Let me know if this approach looks okay to you, and I will create a PR.

[bvaughn]

I guess adding such a prop to WindowScroller would be reasonable. 😄