Fix bug in WindowScroller::updatePosition #1642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yamadapc
pushed a commit
to yamadapc/react-virtualized
that referenced
this pull request
Apr 13, 2021
After further testing, I've found that after `handleWindowScrollEvent` is called we also need to force the `isScrolling` state variable to turn back to false (otherwise there might be issues with rendering rows that depend on `isScrolling`).
|
When is this planned for release? |
yamadapc
added a commit
that referenced
this pull request
Apr 19, 2021
After further testing, I've found that after `handleWindowScrollEvent` is called we also need to force the `isScrolling` state variable to turn back to false (otherwise there might be issues with rendering rows that depend on `isScrolling`).
|
I'll release it in the next few days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, I'm opening this PR to fix a bug found in the
WindowScroller::updatePositionmethod.I've pushed a full sandbox reproduction of the bug, with documentation of what is the problem and the fix to https://codesandbox.io/s/7mdt8. I'd be happy to discuss the problem further on a video-call and or send you a video-recording I've of it through e-mail.
In summary, the bug happens when trying to react to "header height" changes with
WindowScroller. The documentation points to callingWindowScroller::updatePosition, however, while that updates the cached position on theWindowScrollerinstance variables, it does not update thescrollTopstate variable. This causes virtual-lists linked to it to not properly render immediately after a header height change; only after a subsequent scroll event.I've added E2E tests which reproduce the error and a prop that fixes it by forcing a scroll event handler call.
There're tests for the prop being true/false so we can see that when it's false the problem exists but when true the problem is fixed.
The fix is essentially flagged by this prop and would be opt-in. I'm not sure if that's the best way.