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.scrollIndicatorInsets behavior changed in iOS 13 #28140

Closed
justinwh opened this issue Feb 20, 2020 · 24 comments
Closed

ScrollView.scrollIndicatorInsets behavior changed in iOS 13 #28140

justinwh opened this issue Feb 20, 2020 · 24 comments
Labels
Needs: Triage 🔍 Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@justinwh
Copy link
Contributor

justinwh commented Feb 20, 2020

When running on iOS 13, ScrollViews set their scrollIndicatorInsets automatically to be inside the device's "safe area" (i.e. inside the "notch"). On older iOS versions this did not happen—users needed to take steps to accommodate the notch (either setting the scrollIndicatorInsets directly or nesting ScrollView inside SafeAreaView).

The difference is clear using a basic test app with a fullscreen ScrollView. On an iOS 13 device (this is easiest to see on a phone with a notch, e.g. an iPhone X), when you scroll the view and reveal the indicators, they start/stop inside the notch.

ios13

On an iOS 12 device, the indicators start/stop at the edge of the view.

ios12

(Both of these screenshots are taken just as the scrolling reaches the top of the view.)

Here's the App.js I used in a brand new, latest version (0.61.5) RN project to generate the above screenshots:

import React from 'react';
import * as RN from 'react-native';

const a20 = new Array(20).fill({}, 0, 20);

export default function App (props) {
  return (
    <RN.ScrollView automaticallyAdjustContentInsets={false}
      contentInsetAdjustmentBehavior='never'
      style={{flex: 1}}>
      {a20.map((o, i) => (
        <RN.View key={i}
          style={{backgroundColor: ((i % 2 === 0) ? 'red' : 'blue'), height: 75}}/>
      ))}
    </RN.ScrollView>
  );
}

You cannot set negative scrollIndicatorInsets to replicate the iOS 12 behavior on iOS 13. And any scrollIndicatorInsets you do assign are added to the ones automatically chosen by the OS, so if you set them directly to a value that worked correctly on iOS 12, you'll get broken-looking results on iOS 13 (extra large insets).

This behavior is not affected by the ScrollView.automaticallyAdjustContentInsets or ScrollView.contentInsetAdjustmentBehavior properties (as far as I can tell).

React Native version:

System:
    OS: macOS 10.15.3
    CPU: (4) x64 Intel(R) Core(TM) i5-4258U CPU @ 2.40GHz
    Memory: 194.42 MB / 16.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 10.18.1 - /usr/local/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 6.12.1 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 13.2, DriverKit 19.0, macOS 10.15, tvOS 13.2, watchOS 6.1
    Android SDK:
      API Levels: 28, 29
      Build Tools: 28.0.3, 29.0.2
      System Images: android-29 | Google APIs Intel x86 Atom
      Android NDK: 20.0.5594570
  IDEs:
    Android Studio: 3.5 AI-191.8026.42.35.5977832
    Xcode: 11.3.1/11C504 - /usr/bin/xcodebuild
  npmPackages:
    react: 16.9.0 => 16.9.0 
    react-native: 0.61.5 => 0.61.5
@justinwh
Copy link
Contributor Author

I've found it's possible to set the iOS 13 behavior to match iOS <=12 behavior by setting automaticallyAdjustsScrollIndicatorInsets to false in ScrollView's init code. This is a new property in iOS 13, and it's true by default.

Apple Documentation:
https://developer.apple.com/documentation/uikit/uiscrollview/3198043-automaticallyadjustsscrollindica?language=objc

@react-native-bot
Copy link
Collaborator

Thanks for submitting your issue. Please run react-native info in your terminal and copy the results into your issue description after "React Native version:". If you have already done this, please disregard this message.

👉 Click here if you want to take another look at the Bug Report issue template.

@react-native-bot react-native-bot added the Needs: Environment Info Please run `react-native info` and edit your issue with that command's output. label Feb 20, 2020
@justinwh
Copy link
Contributor Author

It's possible this is related to these other iOS 13 scroll indicator inset-related issues, though those are referring to horizontal insets, not the vertical (notch) insets. The fix might be the same.

#26610

#28085

@justinwh
Copy link
Contributor Author

My exploratory hack/fix was to add this snippet to RCTScrollView.m inside initWithEventDispatcher():

if (@available(iOS 13.0, *)) {
    _scrollView.automaticallyAdjustsScrollIndicatorInsets = NO;
}

@react-native-bot react-native-bot removed the Needs: Environment Info Please run `react-native info` and edit your issue with that command's output. label Feb 20, 2020
@outaTiME
Copy link

outaTiME commented Apr 2, 2020

Hi guys,
in into the same issue with ios 13 im using the following configuration (with FlatList too):

<NativeScrollView
  {...extra}
  {...(safeArea && {
    contentInset: { bottom: Layout.IPHONE_X_PADDING },
  })}
  automaticallyAdjustContentInsets={false}
  contentInsetAdjustmentBehavior="never"
>
  {children}
</NativeScrollView>

Screen Shot 2020-04-02 at 00 03 13

@justinwh this is a core issue, do yo found something else about that?

@justinwh
Copy link
Contributor Author

justinwh commented Apr 2, 2020

I've confirmed this issue still exists in RN 0.62.0, and my proposed fix still works.

Can someone from the project weigh in on this? Does disabling the automatic adjustment sound like an appropriate fix? Should I submit a PR?

@adbl
Copy link
Contributor

adbl commented May 28, 2020

Also seeing this, not able to get the scrollIndicators look correct on a plain react-navigation (latest to date) StackView with floating/transparent header, even if I figure out some workaround in my screen, it will probably have to be separate for iOS 13, seems like a pretty big issue.

@justinwh I will try out your suggestion. Since you haven't gotten any attention from maintainers, maybe just submit a PR and hope to get attention?

@adbl
Copy link
Contributor

adbl commented May 28, 2020

@justinwh maybe add the react-native info to issue description which the bot suggested in order for the issue to get some human treatment?

@adbl
Copy link
Contributor

adbl commented May 28, 2020

Fix proposed by @justinwh seems to work well. Here is a branch that can be used as a PR if you want: https://github.com/adbl/react-native/tree/v0.62.2-fix-scroll-indicators

@stale
Copy link

stale bot commented Aug 29, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 29, 2020
@justinwh
Copy link
Contributor Author

I've confirmed (again) that this issue still exists in RN 0.63.2, and my proposed fix still works.

I've been shipping my own app with a patched RN since February to "fix" this issue. My patch is pretty dumb—it just makes iOS 13 devices match the pre-iOS 13 behavior. That approach eliminates the need for OS version-specific workarounds, which is "good enough" for a one-off hack.

I don't think it's necessarily the best global fix for RN, which is why I didn't submit it as a PR. It feels to me like this requires some consideration.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 29, 2020
@justinwh
Copy link
Contributor Author

Okay, I've submitted a PR for this

#29809

@stale
Copy link

stale bot commented Dec 25, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 25, 2020
@jfrolich
Copy link

This is not stale.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 11, 2021
@p-sun
Copy link
Contributor

p-sun commented May 8, 2021

Thanks for including an example! Rather than breaking many people's ScrollViews on iOS 13 and 14 who relied on Apple's automatic inset adjustments, a better solution is to pass automaticallyAdjustsScrollIndicatorInsets like automaticallyAdjustContentInsets as a prop on ScrollView to solve this for your use case.

Having automaticallyAdjustsScrollIndicatorInsets set to true is Apple's intended behavior. With this set to false, the user will not be able to tell whether or not there is more to scroll on devices with rounded corners, or with ScrollViews underneath native navigation bars or tab bars. See screenshots.

We should aim to give a better user experience by default rather than to force the indicators to line up with the top of the ScrollView. Usually people wouldn't want a ScrollView that overlaps with the battery indicators, like the example in this PR.

Screen Shot 2021-05-07 at 8 27 42 PM

Screen Shot 2021-05-07 at 10 55 16 PM

@justinwh
Copy link
Contributor Author

justinwh commented May 8, 2021

@p-sun Thank you for your feedback! I agree with your sentiment about providing the best developer experience by default. But I'm concerned that choosing a default of true for automaticallyAdjustsScrollIndicatorInsets may not lead to the best experience.

The reason is: the "automatic" insets it chooses are frequently wrong, including in the most common case where ScrollViews are not underlaying the safe area.

You can see this in the rn-tester app itself. Without my patch on iOS 14.5, the SectionList on the app's main screen has its scroll indicators inset from the top edge.

wrong-auto-insets

The SectionList in the screenshot is fully scrolled to the top, and the scroll indicator is still inset. The rn-tester code has not added that inset (see the code below)—it's created by iOS's automaticallyAdjustsScrollIndicatorInsets=YES setting.

<SectionList
sections={filteredSections}
extraData={filteredSections}
renderItem={renderListItem}
ItemSeparatorComponent={ItemSeparator}
keyboardShouldPersistTaps="handled"
automaticallyAdjustContentInsets={false}
keyboardDismissMode="on-drag"
renderSectionHeader={renderSectionHeader}
ListFooterComponent={() => <View style={{height: 80}} />}
/>

I don't know why iOS decides to add this inset. The SectionList in rn-tester doesn't underlay the device safe area. And the size of the inset isn't the height of the status bar either. Apple's docs don't explain how it works. And at least in React Native apps, it frequently does the wrong thing.

Here's the same SectionList in the same position with my patch applied (which sets automaticallyAdjustsScrollIndicatorInsets=NO.)

correct-insets

It's also worth reviewing how the related/similar props in ScrollView currently behave, since I found these pretty hard to learn (and use) as a React Native user.

  • automaticallyAdjustContentInsets has a default value of true. However, when I tested this prop in the rn-tester screen I added (which underlays the status bar), it had no effect—leaving it unset, setting it true and setting it false all result in no contentInset adjustment. (I'm not sure whether I've misunderstood the docs, this prop is deprecated, or what. If you know how this works, I'm happy to be corrected.)

  • contentInsetAdjustmentBehavior has a default value of 'never'. This is different from its default value in iOS, which would be 'automatic' (see contentInsetAdjustmentBehavior). Unlike the above prop, this one does change ScrollView's behavior (in my testing). However, in my own apps I've found it suffers from the same unpredictability as automaticallyAdjustsScrollIndicatorInsets: it often makes unexpected adjustments when set to any value other than 'never'. A comment in RCTScrollView.m says so at the spot where it assigns the default to 'never'.

#if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && __IPHONE_OS_VERSION_MAX_ALLOWED >= 110000 /* __IPHONE_11_0 */
// `contentInsetAdjustmentBehavior` is only available since iOS 11.
// We set the default behavior to "never" so that iOS
// doesn't do weird things to UIScrollView insets automatically
// and keeps it as an opt-in behavior.
if ([_scrollView respondsToSelector:@selector(setContentInsetAdjustmentBehavior:)]) {
if (@available(iOS 11.0, *)) {
_scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentNever;
}
}
#endif

If automaticallyAdjustsScrollIndicatorInsets worked consistently, I think it would be correct to enable it by default. But as it works now, I think it may be a net negative for new users—a source of frustration.

For existing users, it's debatable. There are definitely apps where changing the default will make things better, and apps where it will make things worse.

I'm interested to hear your opinion on this. I'm happy to proceed either way. (Or to be corrected if I got any part of the background wrong.)

@lunaleaps
Copy link
Contributor

I think I lean towards setting the property to false by default and adding a property to ScrollView to override it for the following reasons:

  • The lack of documentation of what automaticallyAdjusts means
  • The fact that you have the flexibility to customize scroll indicator insets
  • To users of React Native who are not familiar with iOS, I could see this bringing confusion of why there is always some default offset (even if they customize their insets) and I suspect it would be higher effort for them to find the override than users who expect automatic inset adjustment from the platform.

This is a somewhat related article digging into what automaticallyAdjustsScrollViewInsets (different property) means and I'm totally extrapolating here but the fact that the view hierarchy (it being nested in UINavigationController) here affects the value of the automatic adjustment in this case just speaks to the cryptic nature of the potential conditions here.

I think by setting automaticallyAdjustsScrollIndicatorInsets default to false with the option to override provides the most flexibility and broad default expectation.

In addition, using the default=true value that Apple sets, this is a breaking change then to all React Native surfaces that have custom insets.. isn't it?

The con of changing our default value is that it goes against the platform, but I think that's the strongest argument for it? I think that's fair but I think the argument weakens to the point of RN being a framework that's clear to use.

@justinwh
Copy link
Contributor Author

In addition, using the default=true value that Apple sets, this is a breaking change then to all React Native surfaces that have custom insets.. isn't it?

In current React Native, all apps have automaticallyAdjustsScrollIndicatorInsets = true and can't turn it off. (This affects all ScrollViews with or without custom insets.)

I'm in favor of changing the new default to false because I think it's the right value going forward (i.e. the least-unexpected behavior for new apps).

However, changing it to false is a breaking(-ish) change for existing apps. Developers will want to check their ScrollViews and set the prop to true if the automatic adjustment is, in fact, what they wanted. That is potentially another argument against changing the default (though in my opinion it's still outweighed by the upside.)

@p-sun
Copy link
Contributor

p-sun commented May 20, 2021

I chatted with @fkgozali, my engineering manager on the React Native team, and we are still in favor of adding the automaticallyAdjustsScrollIndicatorInsets as a prop and keeping default as true. Apple has had automaticallyAdjustsScrollViewInsets default to true since iOS 7.0.

For existing users, it's debatable. There are definitely apps where changing the default will make things better, and apps where it will make things worse.

I think in this case it's more important not to break existing apps than to potentially fix an issue with extra insets that existing apps haven't prioritized fixing. If an existing app has extra scroll indicator insets, it would have had these extra insets ever since they built that screen.

Changing the default to false is good for the developers for consistency, but this could be bad for the users of the apps. I believe Apple thought about this and decided to value usability over developer consistency when they decided to separate automaticallyAdjustsScrollIndicatorInsets and contentInsetAdjustmentBehavior into two variables instead of keeping them together. Adding the automaticallyAdjustsScrollIndicatorInsets prop will allow developers to fix this for their screens as needed.

If an app is using JS navigation or no navigation, like rn-tester, then they may want automaticallyAdjustsScrollIndicatorInsets=false. If an app is using native navigation (i.e. UINavigationController or UITabBarController), and the React Native view is under either or both of these, then they likely want automaticallyAdjustsScrollIndicatorInsets=true and contentInsetAdjustmentBehavior='automatic'. Changing the default to false could break a lot of screens that use native navigation.

I don't know why iOS decides to add this inset. The SectionList in rn-tester doesn't underlay the device safe area.

The extra inset added with automaticallyAdjustsScrollIndicatorInsets in the rn-tester app is the status bar height. If you rotate the rn-tester app to landscape, you'll see that both the status bar and the inset will disappear.

@lunaleaps
Copy link
Contributor

It sounds like either option would break some people's current usage of the ScrollView. Can we update the PR (#29809) to make this a prop as we'll need that regardless?

I think for default value, again I think both arguments are strong and it seems like it comes to deciding whether we want to go on either

  • What is the "majority" use case? Which could still be argued both ways false/true
  • Follow the guidelines Apple has set

I think from comments from @p-sun and @justinwh I don't have strong opinions.. but either way we should add some good documentation about this prop.

@p-sun
Copy link
Contributor

p-sun commented May 22, 2021

Yes, adding that prop would be fantastic!

justinwh added a commit to justinwh/react-native that referenced this issue May 31, 2021
…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
justinwh added a commit to justinwh/react-native that referenced this issue Jun 21, 2021
…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
facebook-github-bot pushed a commit that referenced this issue Jul 8, 2021
#29809)

Summary:
iOS 13 added a new property to `UIScrollView`: `automaticallyAdjustsScrollIndicatorInsets`, which is `YES` by default.  The property changes the meaning of the `scrollIndicatorInsets` property.  When `YES`, any such insets are **in addition to** whatever insets would be applied by the device's safe area.  When `NO`, 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, when `NO` it does not.

There are two problems with the default `YES` setting:

1. It means applying `scrollIndicatorInsets` to a `ScrollView` has a different effect on iOS 13 versus iOS 12.
2. It limits developers' control over `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

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Changed] - ScrollView scrollIndicatorInsets to not automatically add safe area on iOS13+

Pull Request resolved: #29809

Test Plan:
Updated the RNTester example to explain what to expect. Also removed the `pageScreen` modal example for now as mentioned in my Github comment.

{F628636466}

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):

![before](https://user-images.githubusercontent.com/428831/91644197-ea03a700-ea07-11ea-9489-be27820930eb.png)

![after](https://user-images.githubusercontent.com/428831/91644200-eff98800-ea07-11ea-8788-daf1e783639d.png)

Reviewed By: p-sun

Differential Revision: D28229603

Pulled By: lunaleaps

fbshipit-source-id: 2e774ae150b1dc41680b8b7886c7ceac8808136a
@leotm
Copy link
Contributor

leotm commented Jan 19, 2022

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Apr 10, 2023
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Triage 🔍 Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests

8 participants