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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid onScroll events breaking FlatList #1873

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

lenaic
Copy link

@lenaic lenaic commented Jul 6, 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

RCTScrollView was submitting onScroll events after an onContentSizeChanged event. This was caused by the setFrame call leading to a DocumentViewBoundsChanged notification that would get triggered a second time after the horizontal/vertical scrollers would be enabled/disabled.

This double notification would trigger a scrollViewDidScroll call with an onScroll event being sent because the scroll position would toggle temporarily due to the scrollers being shown/hidden.

This PR updates the scroll view to compare the scroll position with the last sent position after the scrollers were set. This fixes the wrong onScroll event during the setFrame because the scroll position is stable and valid by the time we do the comparison.

Changelog

[macOS] [FIXED] - Update ScrollView to emit only valid onScroll events

Test Plan

Tested by running RNTester on macOS with paper and using the FlatList examples.

Screenshot 2023-07-06 at 19 04 50

@lenaic lenaic requested a review from a team as a code owner July 6, 2023 17:08
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Not obvious from the description, but how did this break Flatlist? This is separate from the general "Flatlist is slow on macOS" issue, right?

@lenaic
Copy link
Author

lenaic commented Jul 6, 2023

Not obvious from the description, but how did this break Flatlist?

We encountered issues in product code where dynamic content loading was implemented using a FlatList and tracking the scroll position. Because the scroll view was emitting scroll events while the view frame was still being set, the FlatList would receive an incorrect scroll event with a 0 height which was breaking the virtual rendering.

This is separate from the general "Flatlist is slow on macOS" issue, right?

It is a separate issue. Here it was happening at mount and resize time.

@Saadnajmi Saadnajmi closed this Jul 6, 2023
@Saadnajmi Saadnajmi reopened this Jul 6, 2023
@Saadnajmi
Copy link
Collaborator

Not obvious from the description, but how did this break Flatlist?

We encountered issues in product code where dynamic content loading was implemented using a FlatList and tracking the scroll position. Because the scroll view was emitting scroll events while the view frame was still being set, the FlatList would receive an incorrect scroll event with a 0 height which was breaking the virtual rendering.

This is separate from the general "Flatlist is slow on macOS" issue, right?

It is a separate issue. Here it was happening at mount and resize time.

My bad for close/open.. finger slipped.
Are you also planning to bring this to the 0.71 branch? That would be nice :)

@lenaic
Copy link
Author

lenaic commented Jul 6, 2023

Are you also planning to bring this to the 0.71 branch? That would be nice :)

Absolutely! I'll submit another bug fix PR and then bring both changes over to 0.71.

@Saadnajmi Saadnajmi merged commit 910e8f8 into microsoft:main Jul 13, 2023
18 of 19 checks passed
@lenaic lenaic deleted the flatlist-onscroll-fix branch July 14, 2023 12:12
lenaic pushed a commit to lenaic/react-native-macos that referenced this pull request Jul 14, 2023
Saadnajmi pushed a commit that referenced this pull request Jul 14, 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.

None yet

2 participants