Feature: minimum padding

[Shaninnik]

Summary

When building mobile apps I was often faced with the requirement to set a padding on container views (most of the time bottom) that is either equals to bottom safe area or provided value if it is greater than safe area. I've always used something like this:

const safeArea = useSafeAreaInsets();
... style={[
          { paddingBottom: Math.max(safeArea.bottom, 24) }
        ]}

This works well, but adds big performance impact by using hook. It was extremely noticeable in apps that support device orientation, when changing orientation caused every screen that calls useSafeAreaInsets to rerender (sometimes up to 4 times).

This PR adds same functionality on native side. New minPadding property on SafeAreaView is absolutely optional and PR will not break any existing app. There was a request to add similar feature #308

I hope someone else find find it useful and it will be merged to main branch.

Test Plan

New minPadding property is added to example app, with a switch to turn it off or on. With it turned on expected outcome is that devices with bottom safe area greater than 24px are not affected, and devices without bottom safe area will have a 24px bottom padding.

[jacobp100]

Thanks for this! We have a ticket #308 that discussed another way to expose the same thing this does - but via another API. I’m definitely open to suggestions on how to approach this

I think this way definitely fixes the problem, but I worry in practise it would be easy to get it wrong. In almost every case, you’d need to remove the padding, and move it to this new prop. But I worry people would forget the first part. Also - a minor issue, but this prop also applies to margin if you change the mode

Let me know what you think anyway!

[Shaninnik]

I might be missing something but I don't see the issue with padding. People who already use padding can continue doing so and ignore this update, they will just get safeArea + padding, the same way as they have it now.

308 is basically the same: useful to take the maximum of the two margins instead but about margin. I chose to use padding because it is default mode. In fact I have never ever used margin mode in any of my apps.

I will re-test with margin mode, it definitely should not be applied with margins.

[jacobp100]

I might be missing something but I don't see the issue with padding. People who already use padding can continue doing so and ignore this update, they will just get safeArea + padding, the same way as they have it now.

I mean if they want the minimum padding behaviour. I’m not sure there’s a use-case for supporting both padding and min-padding at the same time. If we do support both at the same time, the developer is gonna have to think more, and I worry in many cases they’ll get it wrong

I think the real example is floating buttons at the bottom of the display. You need the minimum for the iPhone SE, but don’t want any additional padding on phones without the home buttons. Are there any other cases I missed?

it definitely should not be applied with margins.

Why not? It seems useful

[Shaninnik]

Oh I see what you mean now. My reasoning was that padding is something that applies on top of the inset. Initial logic s safeAreaInset + padding, so I just followed it, simply adding additional constraint to safe area insets. I agree that I don't see a use case for minimum padding + padding, but perhaps there is for someone? And it does not overwrite existing logic. I am open to changes though, I can make minPadding completely overwrite safeArea+padding if you think this is better.

I think the real example is floating buttons at the bottom of the display. You need the minimum for the iPhone SE, but don’t want any additional padding on phones without the home buttons. Are there any other cases I missed?

Yeah, this is the exact use case I need it for. And for content inside scroll views.

Why not? It seems useful

Well.... because it has padding name in it...

I think to avoid both issues you mentioned perhaps I should leave the logic exactly the same as it is and just rename minPadding to something better? For example minSafeArea. Then the logic is:

Padding mode:
SafeAreaView padding = max(minSafeArea, safeArea) + padding

Margin mode:
SafeAreaView margin = max(minSafeArea, safeArea) + margin

I should also update docs to mention new property.

What do you think?

[jacobp100]

Sorry - I just realised that linked issue didn't actually explicitly demonstrate an API. One example would be to make the edges prop (optionally) take an object:-

<SafeAreaView
  style={{ padding: ... }}
  // Naming TBC - just for example
  edges={{ top: 'off', left: 'additive', right: 'additive', bottom: 'maximum' }}
/>

When you pass in an array (the current API), every value in the array would be additive, and every other value would be off

Let me know what you think! I'm happy to look at other APIs too. It's worth spending the time now, because once this gets merged we're gonna be stuck with that decision 🤣

[Shaninnik]

I wanted solution to be as less intrusive for others as possible, and my proposed one is, most people can just ignore it and nothing will change for them. For people who are trying to achieve the same thing with useSafeAreaInsets and { paddingBottom: Math.max(safeArea.bottom, 24) } this will be very a very familiar logic and I am sure they will be comfortable with it, they also are not forced to upgrade, they can continue doing it this way as it works and just causes some slow rerenders on device orientation changes. People who currently use padding with SafeAreaView and live with increased paddings on devices with safe area most probably not going to change it because their design is already based on the fact of double padding and it does not bother them (I think this is the case you are worried about)

Changing edges property from array to object is not an option, it is a breaking change and I am 100% sure that we don't want that. Having it as a union Edges[] | {top: 'off' | 'additive' | 'maximum', left:..., right:..., bottom:...} I think is the same level of confusing as dealing with padding and you will have to list all edges all the time. For example I have a use use case right now where I need only a bottom edge from safe area with minimum padding. Instead of being like this:

// my proposal. 100% agree that `minPadding` from my initial PR is a bad name, hence `minSafeArea`
<SafeAreaView edges={['bottom']} minSafeArea={{ bottom: 24 }}> 

I will have to be like this:

// your proposal
<SafeAreaView style={{paddingBottom: 24}} edges={{ top: 'off', left: 'off', right: 'off', bottom: 'maximum' }}> 

Honestly I would rather stick with existing implementation of edges as I (and everyone) got already used to it and it does not force you to list every edge. Also dealing with native bridge for union already gives me nightmares.

Other options, all naming is just an example

/// edgeLogic: {{top?: 'additive' | 'maximum'...}} - if no value supplied default is 'additive'
<SafeAreaView style={{paddingBottom: 24}} edges={['bottom']} edgeLogic={{bottom: 'maximum'}}>  

/// edgeLogic: 'maximum' | 'additive' - default 'additive'. Very simple option but will apply same logic for all edges
<SafeAreaView style={{paddingBottom: 24}} edges={['bottom']} edgeLogic={'maximum}> 

This is a fairly simple PR for a fairly simple and very niche feature. There was only one other person who complained, and this lib is used by millions. I don't think we should overcomplicate it. I am ready to make whatever change you think is the best and get it merged sooner rather than spending weeks talking about it 😅

[jacobp100]

Changing edges property from array to object is not an option, it is a breaking change and I am 100% sure that we don't want that

Sorry - I probably wasn't quite clear enough here. I meant we'd support both the array form and the object at the same time, and have some Array.isArray logic in the component to normalise both forms into the same format, that then gets passed to the native component. We could do this as a non-breaking change.

Type would be:-

type EdgeMode = 'off' | 'additive' | 'maximum';
type EdgeName = 'top' | 'right' | 'bottom' | 'left';
type Edges = EdgeName[] | Partial<Record<EdgeName, EdgeMode>>;

const edges1: Edges = ['top']
const edges2: Edges = {top: 'additive'}

Does that make sense?

[Shaninnik]

It 100% does and this is exactly how imagined it. The downside is that default has to be 'additive', because it is what will be used if you pass array, and if I want to disable all edges except one I will have to explicitly set 'off' for everything:

edges={{ top: 'off', left: 'off', right: 'off', bottom: 'maximum' }}

This is the exact opposite of how current array edges property works.

[Shaninnik]

But I am fine with either option. No big preference here, just want to make sure that we all agree on a solution before I make changes.

[jacobp100]

and if I want to disable all edges except one I will have to explicitly set 'off' for everything

If we went this way, we’d support letting undefined edges default to off, so it would just be { bottom: ‘maximum’ } - like in the edges2 example above

If you go this way too - it’ll be easiest to add some JS code to the SafeAreaView component in a useMemo that does all the normalisation, then there will be minimal conversion in native code

I thought this would be better that the edgeLogic prop because it removes duplication. Happy to hear other ideas too though

I’m also happy to listen to other suggestions

[Shaninnik]

@jacobp100 I have started doing changes we discussed, it is a bit more involved than my initial PR. I've made a work in progress commit with JS and Android parts almost done. Just need to sort out iOS part, I admit I am not very good with Objective C so need to spend some time.

A couple of notes:

1) I don't think we need 'off' in JS part in type EdgeMode = 'off' | 'additive' | 'maximum'; since we agreed that they default to 'off' if not passed in object. It is enough to have type EdgeMode = 'additive' | 'maximum'; and passing something like edges={{bottom: 'maximum'}}

2) I don't think we need useMemo in SafeAreaView. It is not expensive calc and useMemo will depend on edges property anyway and I don't expect people are memoing props they pass to SafeAreaView

    ? edges.reduce<EdgeRecord>((accum, edge) => {
        accum[edge] = 'additive';
        return accum;
      }, {})
    : edges;

3) I am not sure about web version of SafeAreaView, does it even need to support 'maximum' mode?

[jacobp100]

For 1 - the user needs to be able to do it conditionally - e.g. { top: condition ? 'off' : 'maximum' }

For 2 - RN has some memoization logic so the setProps handler won't be called if the the prop doesn't change

For 3 - We technically can support this, but I don't mind if you want to skip that for this PR, we can document that and I can file a new issue

I'm happy to help with iOS stuff. Let me know if you have any questions

[Shaninnik]

For 1: is anything wrong with {top: condition ? undefined : 'maximum'}? or condition ? {} : { top : 'maximum'} Undefined is already an off state, same as not passing an edge in the array

For 2: Maybe, but it will not work on JS side. I expect 99.9% of users will use it like that (passing edges property inline):

<SafeAreaView
      edges={{ bottom: 'maximum' }}
    >

This nets a new object reference for each rerender and useMemo will be useless, it will not memo anything and will run every render creating new reference for nativeEdges:

I am unsure about what degree of memoization RN has for passing props to native components, if it is a deep compare then it literally does not matter if we useMemo or not, if it by reference and it prevents native component updates then we need edgeBitmask trick similar to SafeAreaView.web or React.memo the entire SafeAreaView. But I think it is out of scope of this PR as it will be additional memoization that is not present in current version of the library. I can do it as separate PR after we merge this one.

This does work if users will be memoing edges property:

const edges = React.useMemo(() => {
    return { bottom: 'additive' as const };
  }, []);

<SafeAreaView
      edges={edges}
    >

But I don't think anyone is really doing it that way. Am I wrong?

For 3: I would rather skip it for now as I have never ever even tried react native web before. I can investigate it after we merge and create another PR if it needs some additions to support maximum mode.

I'm happy to help with iOS stuff. Let me know if you have any questions

Going to try it today. I could easily do it in Swift but Objective C... I am sure there will be a lot of swearing involved

[jacobp100]

1 - it matches stuff like contentInset on ScrollViews, missing edges are set to zero, but you can still set defined edges to zero. Not sure about Android, but RCT_ENUM_CONVERTER makes it really easy to default to off when it's not defined

2 - I think people would define it outside the component, rather than useMemo

[Shaninnik]

1 - okay, no problem, I will add off to JS side. There is already off state in Android and it defaults to off so it is not an issue

2- Not sure what you mean. If people define it outside the component or inline it does not matter, it will be new object reference and useMemo inside SafeAreaView will not work, as demonstrated in screenshot. The only way useMemo will work is when people will be memoizing edges property they pass to SafeAreaView itself, and no one is doing that.

If our goal is to memoize SafeAreaView we need to wrap it with React.memo and deep compare passed properties. Current version of SafeAreaView is not memoized and works exactly the same, regardless of my small added function.

[jacobp100]

const edges = ['top']

const Component = () => {
  return <SafeAreaView edges={edges} />
}

[Shaninnik]

I am probably missing what you are trying to achieve:

[jacobp100]

Yeah - you gotta move that _edges variable declaration completely outside the component - before export default function ReactNativeSafeAreaView()

[Shaninnik]

Yeah, right, if _edges is outside then it has a stable reference and useMemo will work in that case. My point is, are library users doing it that way? I don't think so. I always use it inline:

<SafeAreaView edges={['right', 'bottom', 'left']} />

Official docs form SafeAreaView use it inline. I looked at the react-navigation source code and they use it inline too. And I expect almost everyone uses it inline. In that case, like I have said in my comments, useMemo is very misleading and not doing what we are trying to achieve.

I am happy to improve SafeAreaView memoization in a separate PR.

[jacobp100]

A quick search - yes people do it

https://github.com/mattermost/mattermost-mobile/blob/8d475d8a36dae72334a5950d30db217deea72fc2/app/screens/code/index.tsx#L7

https://github.com/tsyirvo/react-native-starter/blob/695628c36dc8391444c77bc5b3f53646953a3031/src/components/ui/SafeView.tsx#L2

https://github.com/AaronZhangL/mobile-native/blob/63232c195736a825a7f52e514eec445d6219790d/src/discovery/v2/PlusDiscoveryScreen.tsx#L40

[Shaninnik]

I think it worth mentioning it in docs in Optimization section then?

Or better, properly memoize SafeAreaView with React.memo?

[jacobp100]

RN already does some memoization - if the prop identity stays the same, the native code handler for the prop isn't called. We don't need to go any further than adding a useMemo for the people who do use stable identities

Also - we can't do React.memo, as this component takes children, whose identity almost always changes

[Shaninnik]

Damn right, I forgot about children.

1) Added useMemo. Played a bit with native debugger and surprisingly for me it does not call updateInsets even if edges value is not stable. Good to know. 2) Added explicit 'off' option 3) Extracted getEdgeValue function to make it pretty 4) Added native iOS stuff, it works as expected but I am totally unsure how garbage that ObjC code that I've added is

[jacobp100]

@Shaninnik Looking good! Added a few comments. Thanks for working on this too - it'll really help people out

[Shaninnik]

Ok, I've got it for fabric, files under common/cpp/react/renderer/components/safeareacontext/ are not codegened and in fact has to be changed as well because we changed edges type. Crazy stuff, now to write RN native components people need to know JS, Java/Kotlin, ObjC/Swift and C++...

[Shaninnik]

@jacobp100 fabric works now, both iOS and Android

[jacobp100]

Amazing work! I’m happy for this to be merged in at least

We need to add a small note in the docs for this - probably a sentence or two is fine

WDYT @janicduplessis (closes #308)

[Shaninnik]

I've pushed a readme update. I am bad at explaining and english is not my native language, feel free to suggest edits

[Shaninnik]

@jacobp100 so what is our next step? Are we waiting for @janicduplessis ? I would like to merge it.

[jacobp100]

Yep! Hopefully shouldn't be too long!

[janicduplessis]

Looks good, thanks @Shaninnik @jacobp100

[janicduplessis]

I will merge this and do a few minor adjustments

[jacobp100]

@Shaninnik Thanks for your work on this! It's in the latest release

[Shaninnik]

Thanks @janicduplessis @jacobp100 appreciate your comments and help on this!