Skip to content

[iOS] Fixes scrollIndicatorInsets not work in old arch #46944

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.

Sign up for GitHub

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

Closed
zhongwuzw wants to merge 3 commits into from
Closed

[iOS] Fixes scrollIndicatorInsets not work in old arch #46944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f986a0
[iOS] Fixes scrollIndicatorInsets not work in old arch
zhongwuzw Oct 10, 2024
ce2173f
Remove contentOffset
zhongwuzw Oct 10, 2024
abaa37d
Restore verticalScrollIndicatorInsets
zhongwuzw Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/react-native/React/Views/ScrollView/RCTScrollView.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,23 @@ -(type)getter \
RCT_SET_AND_PRESERVE_OFFSET(setShowsVerticalScrollIndicator, showsVerticalScrollIndicator, BOOL)
RCT_SET_AND_PRESERVE_OFFSET(setZoomScale, zoomScale, CGFloat);

- (void)setScrollIndicatorInsets:(UIEdgeInsets)value
{
[_scrollView setScrollIndicatorInsets:value];
}

- (UIEdgeInsets)scrollIndicatorInsets
{
UIEdgeInsets verticalScrollIndicatorInsets = [_scrollView verticalScrollIndicatorInsets];
UIEdgeInsets horizontalScrollIndicatorInsets = [_scrollView horizontalScrollIndicatorInsets];

return UIEdgeInsetsMake(
verticalScrollIndicatorInsets.top,
horizontalScrollIndicatorInsets.left,
verticalScrollIndicatorInsets.bottom,
horizontalScrollIndicatorInsets.right);
}

- (void)setAutomaticallyAdjustsScrollIndicatorInsets:(BOOL)automaticallyAdjusts API_AVAILABLE(ios(13.0))
{
// `automaticallyAdjustsScrollIndicatorInsets` is available since iOS 13.
Expand Down
1 change: 1 addition & 0 deletions packages/react-native/React/Views/ScrollView/RCTScrollViewManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ - (UIView *)view
RCT_EXPORT_VIEW_PROPERTY(scrollEventThrottle, NSTimeInterval)
RCT_EXPORT_VIEW_PROPERTY(zoomScale, CGFloat)
RCT_EXPORT_VIEW_PROPERTY(contentInset, UIEdgeInsets)
RCT_EXPORT_VIEW_PROPERTY(scrollIndicatorInsets, UIEdgeInsets)
RCT_EXPORT_VIEW_PROPERTY(verticalScrollIndicatorInsets, UIEdgeInsets)
Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi Oct 10, 2024

Choose a reason for hiding this comment

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

This is a breaking change. JS would not see the vertical scroll indicator anymore and JS components that use it will break. Can we:

  1. deprecate the verticalScrollIndicatorInsets prop
  2. add the scrollIndicatorInsetsprops
  3. make sure that the verticalScrollIndicatorInsets prop uses the scrollIndicatorInsets under the hood
  4. Add a deprecation warning in JS, somehow so users that are using that prop can change it?

Copy link
Copy Markdown
Contributor Author

@zhongwuzw zhongwuzw Oct 10, 2024

Choose a reason for hiding this comment

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

@cipolleschi I don't see ScrollView props have verticalScrollIndicatorInsets, actually it's changed from scrollIndicatorInsetsprops to verticalScrollIndicatorInsets in #44820 , but I don't know why..

Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi Oct 10, 2024

Choose a reason for hiding this comment

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

Yeah, we need to keep the vertical version around, because the RCT_EXPORT_VIEW_PROPERTY macro exposes the verticalScrollIndicatorInsets to JS and there might be product code that uses that prop.
If we change it to scrollIndicatorInsets, the product code will break.

Copy link
Copy Markdown
Contributor Author

@zhongwuzw zhongwuzw Oct 10, 2024

Choose a reason for hiding this comment

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

@cipolleschi I restore the verticalScrollIndicatorInsets, but actually, if product use it, it would crash because RCTScrollView not implements verticalScrollIndicatorInsets methods.

Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi Oct 10, 2024

Choose a reason for hiding this comment

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

RCTScrollView not implements verticalScrollIndicatorInsets methods.

How did it work before? 😱

Copy link
Copy Markdown
Contributor Author

@zhongwuzw zhongwuzw Oct 10, 2024

Choose a reason for hiding this comment

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

I couldn't find any relevant context in the pull request at #44820. It may just be an oversight. :)

RCT_EXPORT_VIEW_PROPERTY(scrollToOverflowEnabled, BOOL)
RCT_EXPORT_VIEW_PROPERTY(snapToInterval, int)
Expand Down