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

Improving detection of scroll ended #742

Merged
merged 13 commits into from
Aug 18, 2017
Merged

Improving detection of scroll ended #742

merged 13 commits into from
Aug 18, 2017

Conversation

gnbaron
Copy link
Contributor

@gnbaron gnbaron commented Jul 17, 2017

We are facing some issues like #697 #722 #322 #564 and few others, reporting that there is a problem related to the detection of scroll ended.

Like some people already said in this kind of issue, this is happening because the browser is delaying the setTimeout callback on _debounceScrollEnded in exchange of performance reasons.

This is a proposal to fix this kind of problems using requestAnimationFrame instead of setTimeout and it works fine for me.

This PR includes:

  • Using requestAnimationFrame instead of setTimeout in Grid component and Masonry component.
  • Fixing jest setup to polyfill requestAnimationFrame and cancelAnimationFrame.
  • Change a Masonry test to wait before all "animations" finish before assert the result. (I did it with a setTimeout callback because jest.runOnlyPendingTimers() does not wait a task scheduled with requestAnimationFrame, and this kind of task have priority for setTimeout "tasks", so the assert expression will be executed only after _debounceScrollEnded was called).

…debounce scroll callback will be executed in the expected time.
…mationFrame.

setTimout with zero delay was used in Masonry test to wait tasks of requestAnimationFrame finish before assert the result.
Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks great! I look forward to testing it after work.

scrollingResetTimeInterval
)
const delay = () => {
if (Date.now() - this._scrollDebounceStart >= this.props.scrollingResetTimeInterval) {
Copy link
Owner

Choose a reason for hiding this comment

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

We could assign scrollingResetTimeInterval to a local variable and avoid the this.props. lookup chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done.

@bvaughn
Copy link
Owner

bvaughn commented Jul 17, 2017

Hm, looks like there are some conflicts. Try rebasing?

…debounce scroll callback will be executed in the expected time.
…mationFrame.

setTimout with zero delay was used in Masonry test to wait tasks of requestAnimationFrame finish before assert the result.
Copy link
Contributor Author

@gnbaron gnbaron left a comment

Choose a reason for hiding this comment

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

Sorry, It's rebased.

scrollingResetTimeInterval
)
const delay = () => {
if (Date.now() - this._scrollDebounceStart >= this.props.scrollingResetTimeInterval) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done.

@bvaughn
Copy link
Owner

bvaughn commented Jul 18, 2017

Excellent! Thank you! 😄

I'm going to hold off on merging this until #743 has been merged since that PR is pretty complicated and I don't want to cause any conflicts. Stay tuned!

@bvaughn bvaughn mentioned this pull request Jul 19, 2017
this._debounceScrollEndedCallback,
scrollingResetTimeInterval
);
const delay = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Allocating function on every scroll event can slow down performance.

* Check if the difference between current time and the last scroll ended event is greater.
* than the scrollingResetTimeInterval prop, else schedule this function to execute again.
*/
_delayScrollEnded() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Conflict can be solved If you will use class property here instead of bind

_delayScrollEnded = () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm merging right now.
I was just getting a coffee 😄

@gnbaron
Copy link
Contributor Author

gnbaron commented Jul 20, 2017

I did the merge with these last changes on Grid component, but after the merge of this commit, the build is broken:

...
source/Grid/Grid.jest.js -> dist/commonjs/Grid/Grid.jest.js
TypeError: source/Grid/Grid.js: Property value expected type of string but got null
    at Object.validate (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-types/lib/definitions/index.js:161:13)
    at validate (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-types/lib/index.js:505:9)
    at Object.builder (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-types/lib/index.js:466:7)
    at PluginPass.ObjectExpression (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-plugin-transform-es2015-duplicate-keys/lib/index.js:65:26)
    at newFn (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/visitors.js:276:21)
    at NodePath._call (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/path/context.js:76:18)
    at NodePath.call (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/path/context.js:48:17)
    at NodePath.visit (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/path/context.js:105:12)
    at TraversalContext.visitQueue (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitSingle (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/context.js:108:19)
    at TraversalContext.visit (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/context.js:192:19)
    at Function.traverse.node (/home/guilherme/dev/js/test/react-virtualized/node_modules/babel-traverse/lib/index.js:114:17)

It is weird because no one warning or error was reported from flow or eslint.
I tried to checkout the master branch without my changes, but the problem already happens.

What do you think?

@TrySound
Copy link
Contributor

TrySound commented Jul 20, 2017

@TrySound
Copy link
Contributor

brigand/babel-plugin-flow-react-proptypes#118

@bvaughn
Copy link
Owner

bvaughn commented Jul 21, 2017

Although tests pass, this branch prints a bunch of warnings not present in master:

Warning: Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op.

Please check the code for the Grid component.

_delayScrollEnded = () => {
const { scrollingResetTimeInterval } = this.props;
if (Date.now() - this._scrollDebounceStart >= scrollingResetTimeInterval) {
this._debounceScrollEndedCallback();
Copy link
Owner

Choose a reason for hiding this comment

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

minor nit: I realize we weren't doing this before, and it's no big deal, but we might as well clear this._disablePointerEventsTimeoutId here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that the pointer must be cleared, but this hasn't been doing on _debounceScrollEndedCallback function?

Copy link
Owner

Choose a reason for hiding this comment

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

You're right 😄 Sorry, I was just skimming the diff and that callback wasn't visible. Sorry~ 👍

const { scrollingResetTimeInterval } = this.props;
if (Date.now() - this._scrollDebounceStart >= scrollingResetTimeInterval) {
this._debounceScrollEndedCallback();
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto as for Grid

@jaredLunde
Copy link
Contributor

@bvaughn @guilhermefloriani

Warning: Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op.

Please check the code for the Grid component.

I'd guess that error is occurring because the following two lines should be window.cancelAnimationFrame rather than window.clearTimeout

@gnbaron
Copy link
Contributor Author

gnbaron commented Aug 9, 2017

Nice @jaredLunde, now seems to be ok, the warnings not show up.
Sorry everyone, I didn't had time to work on this.

@jaredLunde
Copy link
Contributor

One last place requestAnimationFrame needs to replace setTimeout is in the WindowLoader -> onScroll util. I have done so in a local branch and also replaced this kind of pattern which gets used multiple times with requestAnimationTimeout and cancelAnimationTimeout functions:

    const delay = () => {
      if (Date.now() - this._scrollDebounceStart >= this.props.scrollingResetTimeInterval) {
        this._debounceScrollEndedCallback()
      } else {
        this._disablePointerEventsTimeoutId = window.requestAnimationFrame(delay)
      }
    }

    this._scrollDebounceStart = Date.now()
    this._disablePointerEventsTimeoutId = window.requestAnimationFrame(delay)

So this code snippet is reduced to:

   this._disablePointerEventsTimeoutId = requestAnimationTimeout(
      this._debounceScrollEndedCallback, 
      this.props.scrollingResetTimeInterval
    )

And window.cancelAnimationFrame(...) becomes cancelAnimationTimeout(...). This also removes the need for a _scrollDebounceStart property.

What's going to be the easiest way to review those changes? Does it make more sense to submit a separate PR altogether with this master branch or to submit a PR to @guilhermefloriani's branch?

@bvaughn
Copy link
Owner

bvaughn commented Aug 9, 2017

Up to you 2 to work out. @guilhermefloriani could give you push access to his fork of RV and you could make your changes directly to it.

@gnbaron
Copy link
Contributor Author

gnbaron commented Aug 9, 2017

I sent an invite to him.

… requestAnimationFrame

This update also replaces setTimeout/clearTimeout in the WindowScroller component with the requestAnimationFrame-based timeout function.
@@ -1,3 +1,9 @@
import requestAnimationTimeout, {
cancelAnimationTimeout
} from '../../utils/requestAnimationTimeout'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks a bit weird. Maybe better to use only named exports? And then filename can be changed to animationTimeout.js

} from '../../utils/requestAnimationTimeout'



Copy link
Contributor

Choose a reason for hiding this comment

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

To much line breaks

@bvaughn bvaughn merged commit c2e1d93 into bvaughn:master Aug 18, 2017
@bvaughn
Copy link
Owner

bvaughn commented Aug 18, 2017

This looks good! Collection still uses setTimeout but I don't think that's a very commonly used component so it's probably okay to update it later.

I think the only thing remaining would be to fallback to setTimeout for browsers that don't support requestAnimationFrame (eg IE 9). I've done this via f3beee0.

Thanks for your work on this issue!

@bvaughn
Copy link
Owner

bvaughn commented Sep 18, 2017

This feature has been released in version 9.10.0. Thank you for contributing!

@peisenmann
Copy link

Oh no, Collection is the one I'm using and experiencing this XD. Any odds there's a fix in the works?

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.

5 participants