-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Add ScrollView.automaticallyAdjustsScrollIndicatorInsets prop (on iOS) #29809
Add ScrollView.automaticallyAdjustsScrollIndicatorInsets prop (on iOS) #29809
Conversation
Hi @justinwh! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
There is an extra concern with this PR: changing the default behavior will require developers who are using ScrollView in this way to re-adjust their code.
Based on the low interest in the initial bug, I don't think this is a lot of people, but it's not zero either. |
Base commit: ad0ccac |
Base commit: 006f5af |
What are the next steps for this PR? We have a local patch for RN in our repo with pretty much the same code. I would like to see that issue fixed so that we don't need to reapply our patch with every upgrade of RN.
Can't you just put this into a new "breaking" version? Whenever we upgrade we check the changes and are expecting something to break. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I'd love that someone in the core team would look at this. Without this, it is impossible to get the scroll indicators right on iOS in some scenarios. It's also a bit frustrating because I think a lot of people would fiddle with it and conclude it's not possible to get it right, or after a long time find this patch (like me 😃) |
Yeah, crazy huh? I'm the original submitter—I don't know what else to do to get this fixed. I documented the issue pretty thoroughly, found the cause, and submitted a fix PR. I'm not authorized to merge it myself. As far as I can tell, nobody from the project who is authorized to merge it is aware it exists. I don't know what to do to get a maintainer's attention. Today I tried asking in the Reactiflux discord linked from the project website, but didn't get any useful answers there either. The original issue (which is now just shy of its one-year anniversary) still displays the "Needs: Triage" label... On the plus side, I can confirm that I've been shipping this fix in my own app for over a year and it has not caused any problems. |
These changes look good to me! cc @PeteTheHeat or @p-sun for any thoughts? |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@justinwh Would it be possible to add your test case (demonstrating the scrollbar positions) as an RNTester case? How were you able to display the scrollbars persistently? We're trying to see how we can prevent this from regressing in the future. |
@lunaleaps Unfortunately, I captured my screenshots manually without persistent scrollbars (i.e. just kept trying until I got the timing right). As far as I know the scrollbars cannot be made to display persistently on iOS. And I think their animation is too quick to capture consistently in a screenshot-based automated test. I'm not aware of any good option for setting up a test case for this. (But I'm open to suggestions.) |
Yeah, I agree. I think then a manual test example case of this might be the best we can do? cc @p-sun who had some thoughts on the changes. |
ce9e405
to
d48b23b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues. Runyarn lint --fix
to automatically fix problems.
|
||
const { | ||
Button, | ||
DeviceInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-unused-vars: 'DeviceInfo' is assigned a value but never used.
Modal, | ||
ScrollView, | ||
StyleSheet, | ||
Switch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-unused-vars: 'Switch' is assigned a value but never used.
ScrollView, | ||
StyleSheet, | ||
Switch, | ||
Text, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-unused-vars: 'Text' is assigned a value but never used.
|}, | ||
> { | ||
state = { | ||
modalVisible: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comma-dangle: Missing trailing comma.
packages/rn-tester/js/examples/ScrollView/ScrollViewIndicatorInsetsExample.ios.js
Outdated
Show resolved
Hide resolved
I added a second commit with an rn-tester screen that demonstrates the behavior (called When the fix is applied, this shows the scrollbars passing outside the device safe area to the screen edge (as intended). If you revert the fix, the scrollbars stay inside the device safe area. |
d48b23b
to
819c91e
Compare
@facebook-github-bot import |
7478a04
to
1f51418
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues. Runyarn lint --fix
to automatically fix problems.
return ( | ||
<View> | ||
<Modal | ||
animationType='slide' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsx-quotes: Unexpected usage of singlequote.
style={styles.switch}/> | ||
<Button | ||
onPress={() => this._setModalVisible(false)} | ||
title='Close'/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsx-quotes: Unexpected usage of singlequote.
</Modal> | ||
<Button | ||
onPress={() => this._setModalVisible(true, 'pageSheet')} | ||
title='Present Sheet Modal with ScrollView'/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsx-quotes: Unexpected usage of singlequote.
title='Present Sheet Modal with ScrollView'/> | ||
<Button | ||
onPress={() => this._setModalVisible(true, 'fullscreen')} | ||
title='Present Fullscreen Modal with ScrollView'/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsx-quotes: Unexpected usage of singlequote.
I updated the PR with the changes discussed in the issue:
I followed the implementation of the react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm Lines 257 to 270 in d1ab032
react-native/ReactCommon/react/renderer/components/scrollview/ScrollViewProps.cpp Lines 166 to 170 in d1ab032
|
1f51418
to
a8875b6
Compare
Re: ScrollViewProps.cpp, I think this is the defaults we're using for the Fabric native view so we should probably set the default of true there as well. Re: RCTScrollViewComponentView.mm I believe so yes, as in updating the prop value. Let me see if someone from with more Fabric experience can confirm. cc @p-sun? |
Yes, @lunaleaps is right. Any new prop should be added to the Fabric component as well. You can add the prop underneath the contentInsetAdjustmentBehavior prop in the two places that Luna linked, add a breakpoint inside RCTScrollViewComponentView.mm, and test it inside the RN Tester app to make sure it works. Thanks for adding a Switch in your example for this prop. 👍 |
Btw, RNTester iOS/Android should already support Fabric, but perhaps with some minor quirks. You can enable Fabric in RNTester iOS manually by setting this env var before
Then at least you can verify whether the updated .mm/C++ files compiled and ran fine. |
…or iOS) Summary: In iOS 13 Apple added a new property to UIScrollView that "automatically" alters the scroll indicator insets in a fashion similar to the way 'contentInsetAdjustmentBehavior' alters content insets. See here for iOS documentation: https://developer.apple.com/documentation/uikit/uiscrollview/3198043-automaticallyadjustsscrollindica?language=objc The OS default value for this property is `true`, which we preserve. When set to `false`, the behavior matches iOS <= 12. Closes facebook#28140
…iew.automaticallyAdjustsScrollIndicatorInsets
a8875b6
to
6b396b8
Compare
I added the Fabric changes, and confirmed they compile and run when The rn-tester page I added requires |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
For the RNTester example, I noticed that there's no difference between true/false for the property in the case of displaying a Modal with @justinwh was there a reason to include Additionally did we discuss adding documentation for this? If not, we should get a PR in react-native-website for this new property! Let me know if you want to work on that. For now, I'll delete the pageSheet Modal example and we can do a follow up PR to add it back in if I'm misunderstanding. See |
@lunaleaps thank you for fixing the typo. The On both devices I tested, enabling it adds a top inset to the In any case, what it does when enabled is outside our control. The important thing is that it leaves the scroll indicator insets alone when disabled, and it looks like that's the case in your screenshots. So I believe this is acceptable. If you want to remove the We didn't discuss documentation. I assumed the docs were generated from the comments, but it sounds like that's not the case. Is there a process for synchronizing the updates to the react-native-website repo with React Native releases that I should be aware of? Here's my proposed documentation for the new prop (let me know if you have feedback on it):
|
@justinwh Sounds good! I'll go ahead and land.. Regarding docs that sounds good! I wonder if we should also link to the UIScrollView docs for the property since. Would you like to make a PR for react-native-website? It should go under the versioned_docs |
…torInsets (Prop added here: facebook/react-native#29809)
I added a link to the |
…torInsets (Prop added here: facebook/react-native#29809)
@lunaleaps merged this pull request in bc1e602. |
hey @justinwh we are integrating this change into our app now and finally removing our RN patch for this, but are confused a bit on what the default actually is. From the code, it looks like it defaults to
What is meant? |
@glenna The docs are correct—the default is Sorry for the confusion. |
@justinwh not a problem! thanks for the clarification! |
Summary
iOS 13 added a new property to
UIScrollView
:automaticallyAdjustsScrollIndicatorInsets
, which isYES
by default. The property changes the meaning of thescrollIndicatorInsets
property. WhenYES
, any such insets are in addition to whatever insets would be applied by the device's safe area. WhenNO
, the iOS <13 behavior is restored, which is for such insets to not account for safe area.In other words, this effects ScrollViews that underlay the device's safe area (i.e. under the notch). When
YES
, the OS "automatically" insets the scroll indicators, whenNO
it does not.There are two problems with the default
YES
setting:scrollIndicatorInsets
to aScrollView
has a different effect on iOS 13 versus iOS 12.scrollIndicatorInsets
. Since negative insets are not supported, if the insets the OS chooses are too large for your app, you cannot fix it.Further explanation & sample code is available in issue #28140 .
This change sets the default for this property to
NO
, making the behavior consistent across iOS versions, and allowing developers full control.Changelog
[iOS] [Changed] - ScrollView scrollIndicatorInsets to not automatically add safe area on iOS13+
Test Plan
Here are screenshots of the demo app (from the original bug) before (with safe area applied to insets) & after (without safe area applied to insets):