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

ScrollView - only send the didScroll event if the document origin has changed #1838

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

shwanton
Copy link

@shwanton shwanton commented Jun 1, 2023

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

  • In 0.71, we had a race condition where the ScrollView scrollViewDocumentViewBoundsDidChange notification would sometimes trigger the onScroll event with a contentSize of zero.
  • This happened when the scrollview was loading data & showing a spinner. With and empty contentSize, the spinner would be stuck and the content would never render.
  • With this fix, we track the latest document view origin & we only emit the onScroll if the origin has changed which means scrolling has occurred.

Changelog

[MACOs] [FIXED] - ScrollView - only send the didScroll event if the document origin has changed

Test Plan

  • This race condition was intermittent and I could not repro with RNTester using setTimeout to imitate the behavior of loading data.

Before (spinner gets stuck & content doesn't load)

CleanShot.2023-05-31.at.17.27.44.mp4

After

CleanShot.2023-05-31.at.17.34.10.mp4

@shwanton shwanton force-pushed the scrollview-onscroll-fix branch from 6edb1da to e7b7c3c Compare June 1, 2023 00:43
@shwanton shwanton marked this pull request as ready for review June 1, 2023 00:44
@shwanton shwanton requested a review from a team as a code owner June 1, 2023 00:44
@shwanton shwanton changed the title ScrollView - only send the onScroll event if the document view has changed [0.71] ScrollView - only send the onScroll event if the document view has changed Jun 1, 2023
@shwanton shwanton changed the title [0.71] ScrollView - only send the onScroll event if the document view has changed [0.71] ScrollView - only send the didScroll event if the document origin has changed Jun 1, 2023
@Saadnajmi
Copy link
Collaborator

Will this also come to main?

@shwanton shwanton changed the title [0.71] ScrollView - only send the didScroll event if the document origin has changed ScrollView - only send the didScroll event if the document origin has changed Jun 1, 2023
@shwanton
Copy link
Author

shwanton commented Jun 1, 2023

Will this also come to main?

I originally had this only on main. Just changed the title so it's not referencing 0.71
Do you want me to pick this to 0.71 as well?

@Saadnajmi
Copy link
Collaborator

Will this also come to main?

I originally had this only on main. Just changed the title so it's not referencing 0.71
Do you want me to pick this to 0.71 as well?

Up to you if you want to pick to 0.71. I think we won't personally do it unless we run into the same bug.

@shwanton shwanton force-pushed the scrollview-onscroll-fix branch from e7b7c3c to c502094 Compare June 1, 2023 15:24
@Saadnajmi Saadnajmi merged commit b45a40a into microsoft:main Jun 1, 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

Successfully merging this pull request may close these issues.

2 participants