fix: popover not showing when using new architecture

[mlazari]

NativeModules.UIManager.measure(...) does not work when new architecture is enabled.
ref.current.measureInWindow(...) works on both old and new architecture and returns the same values, except one case: when the status bar is translucent on Android, it substracts the height of the status bar from the "y" value (see #182 (comment)). This means we need to adjust the verticalOffset on Android when the status bar is translucent. To ensure that we always know whether the status bar is translucent or not, we'll default the value of statusBarTranslucent to true when not passed explicitly (which is more similar to iOS) instead of passing undefined to RN Modal.
Since the only intended purpose the verticalOffset prop was to correct manually the position on Android and now it's adjusted internally, we don't need that prop anymore and we are removing it.

[AdamGerthel]

Nice! Will have a look later today

[mlazari]

Actually I just realized this is still not ideal, since having statusBarTranslucent being always false makes it impossible to have the popover transparent overlay extend all the screen, without showing the opaque status bar (e.g. you can see here the difference between translucent and non-translucent: #182 (comment)).

[AdamGerthel]

Hmm, would it make sense to measure the navigation bar height and include that value in the Rect calculations? I've done so in several projects in similar situations. Example:

import { Dimensions } from 'react-native'

const windowSize = Dimensions.get('window')
const screenSize = Dimensions.get('screen')
const navigationBarHeight = screenSize.height - windowSize.height

[mlazari]

@AdamGerthel Is that different from StatusBar.currentHeight? I thought about including that in Rect calculations, but in that case getRectForRef needs to know the value of the statusBarTranslucent prop (so that it includes it only when the status bar is translucent), so it has to be passed to it from where it's called I guess.

[AdamGerthel]

@AdamGerthel Is that different from StatusBar.currentHeight? I thought about including that in Rect calculations, but in that case getRectForRef needs to know the value of the statusBarTranslucent prop (so that it includes it only when the status bar is translucent), so it has to be passed to it from where it's called I guess.

Ah, sorry, I wasn't thinking straight. Navigation Bar is the one at the bottom. StatusBar.currentHeight should work.

So if translucent on StatusBar impacts the positioning as you say, then yes, we'll need to factor it in, which means that statusBarTranslucent will have to become a required prop, right?

[mlazari]

Alternatively we could probably adjust verticalOffset instead of changing Rect calculations? (If on Android and statusBarTranslucent is true - add StatusBar.currentHeight to it).

[AdamGerthel]

Alternatively we could probably adjust verticalOffset instead of changing Rect calculations? (If on Android and statusBarTranslucent is true - add StatusBar.currentHeight to it).

As a new default value on verticalOffset prop you mean?

[mlazari]

I was thinking something like this (din't test it yet):

Change

const verticalOffset = (this.props.verticalOffset ?? 0) - displayAreaOffset.y;

To

    const verticalOffsetAndroidAdjustment = Platform.OS === 'android' && this.props.statusBarTranslucent ? (StatusBar.currentHeight ?? 0) : 0;
    const verticalOffset = (this.props.verticalOffset ?? 0) - displayAreaOffset.y + verticalOffsetAndroidAdjustment;

[mlazari]

I just pushed my updates, just need to re-check them with the test app.

[mlazari]

I just tested all combinations that I could think of, and looks good to me.

Old Arch

Status bar on screen: non-translucent
Status bar on popover: non-translucent

old-arch-non-translucent-both.mov

Status bar on screen: non-translucent
Status bar on popover: translucent

old-arch-translucent-popover-only.mov

Status bar on screen: translucent
Status bar on popover: non-translucent

old-arch-non-translucent-popover-only.mov

Status bar on screen: translucent
Status bar on popover: translucent

old-arch-translucent-both.mov

New Arch

Status bar on screen: non-translucent
Status bar on popover: non-translucent

new-arch-non-translucent-both.mov

Status bar on screen: non-translucent
Status bar on popover: translucent

new-arch-translucent-popover-only.mov

Status bar on screen: translucent
Status bar on popover: non-translucent

new-arch-non-translucent-popover-only.mov

Status bar on screen: translucent
Status bar on popover: translucent

new-arch-translucent-both.mov

The cases where status bar translucency on screen is different from the status bar translucency on the popover used to work correctly only with the use of verticalOffset. With these changes that manual adjustment is not needed anymore.

[mlazari]

This would be a breaking change for people who were using the verticalOffset={Platform.OS === 'android' ? -StatusBar.currentHeight : 0 } workaround, since now it's positioned correctly by default and that moves it away from the correct position. verticalOffset is not needed anymore for fixing that, but I didn't remove it so that people that might be using it for moving the popup vertically for some other reason can continue using it.

[AdamGerthel]

Nice work!

I think we should release this as a new major version since it's breaking. I think we should take the opportunity to remove verticalOffset as well, since it only exists for that purpose. I can do that separately though.

[AdamGerthel]

While I'd like to specify the package manager, I think we can leave this change out from this PR and I'll upgrade to the latest yarn version in another commit.

[mlazari]

Ok, I'll remove it. yarn kept adding it, so I decided to commit it, but I agree it's not related to these changes.

[mlazari]

I removed it

[mlazari]

@AdamGerthel If we remove verticalOffset and it's used only for that purpose, then this would not be a breaking change from the point of view of the behaviour (passing in verticalOffset would not have any effect). Maybe just from the point of view that a prop is removed (maybe causing typescript issues?).

[AdamGerthel]

@AdamGerthel If we remove verticalOffset and it's used only for that purpose, then this would not be a breaking change from the point of view of the behaviour (passing in verticalOffset would not have any effect). Maybe just from the point of view that a prop is removed (maybe causing typescript issues?).

But there could be users that are using verticalOffset in other ways, passing other values. It would be breaking for them.

[mlazari]

But there could be users that are using verticalOffset in other ways, passing other values. It would be breaking for them.

True, I was using your assumption that it only exists for that purpose.

[mlazari]

Should I remove the verticalOffset prop in this PR too?

[AdamGerthel]

Should I remove the verticalOffset prop in this PR too?

Ok, might as well 👍

[mlazari]

Done, doing a quick re-check now to ensure everything still works properly.

[mlazari]

Checked, looks good to me.

[AdamGerthel]

Nice work 🙌

[mlazari]

BTW, the addition of the default value (true) for statusBarTranslucent can be considered a breaking change too for users that don't pass a value explicitly.

[arpitdalal]

Thank you both for working on this!

[mlazari]

@arpitdalal Thank you too!

[AdamGerthel]

FYI, the 6.0.0 release did not include the change. I hadn't figured out where NPM got the dist files from and made an error. 6.0.1 should be good to go.