Set size of Modal only once on Android

[j-piasecki]

Summary

On the old architecture, when using a Modal with no animation and translucent status bar, a layout flash is noticeable when the modal is opened. Here's the video of this happening (it's visible for one frame):

old_arch.mov

On the new architecture, the issue is more problematic as the modal keeps the wrong height:

new_arch.mov

This PR updates the logic related to setting modal size, to set only the initial state based on the helper method, and the following updates will be based on the dimensions received in onSizeChanged method.

Issue relevant to this PR (with more details): #35804

Changelog

[Android] [Fixed] - Fixed wrong modal height when statusBarTranslucent is set to true

Test Plan

I've tested the changes on the following snippet:

import React, {useState} from 'react';
import {View, Button, Modal} from 'react-native';

const App = () => {
  const [open, setOpen] = useState(false);

  return (
    <View style={{flex: 1, backgroundColor: 'yellow'}}>
      <Button
        title="Open"
        onPress={() => {
          setOpen(true);
        }}
      />

      <Modal
        visible={open}
        animationType="none"
        statusBarTranslucent={true}>
        <View style={{flex: 1, backgroundColor: 'red'}}>
          <Button
            title="Close"
            onPress={() => {
              setOpen(false);
            }}
          />
        </View>
      </Modal>
    </View>
  );
};

export default App;

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,576,595	+22
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,946,164	+16
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: be38fbb
Branch: main

[jacdebug]

From blame looks like this is added for fixing some app freezes, here the comment from original commit,

In some cases, the size of the content view changes before we add views to the
Modal. That means that the size of that view will not be set through the
onSizeChanged method. This can result in some apps apparently freezing,
since the dialog is created, but there are no actual views in it.
For that reason, we still need the ModalHostShadowNode to set the size of the
initial view, so that by the time the view gets added it already has the correct
size.
There's a new helper class so that we can reuse the modal size computation.

cc @cortinico ref: D3892795

[cortinico]

cc @cortinico ref: D3892795

I'm not sure that's the right diff number @jacdebug

[jacdebug]

cc @cortinico ref: D3892795

I'm not sure that's the right diff number @jacdebug

Ah sorry missing the context, its the change where original fix was added.

[cjshearer]

I'm impacted by this and would like to see it merged. Anything holding this up (other than conflicts)?

[kelset]

@cortinico @jacdebug is there anything specific blocking this?

or @j-piasecki could rebase/address conflict and then it'd be ready to go?

[cortinico]

@cortinico @jacdebug is there anything specific blocking this?

Nope this should be good to go. Let's rebase and merge it

[github-actions]

	Fails
🚫	📋 Missing Changelog - Please add a Changelog to your PR description. See Changelog format

Generated by 🚫 dangerJS against 5db452f

[facebook-github-bot]

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[sammy-SC]

I think the renderer needs this information. What if React Native's viewport is not full screen but only partially covers the screen? In that case, we need to feed the information about available size to React Native so it knows that whatever is within the modal, is not constrained by RN's viewport size.

[WoLewicki]

Is there another direction that we could go to resolve the issue then @sammy-SC ?

[sammy-SC]

I would try to investigate why is the height wrong in the new architecture. The height seems to be off exactly by the height of the status bar. Could it be that ModalHostHelper.getModalHostSize(view.getContext()) returns height of React Native's view port instead of the desired size of the modal? Where is the height calculated for the old architecture and why is it correct? Maybe we missed something in the implementation for the new architecture.

[lyswhut]

Any news?
Hope to fix it in v0.72

I enabled the new architecture and set the StatusBartransLucent as true, and then got the wrong modal height

[github-actions]

This PR 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.

[j-piasecki]

I still have it at the back of my head, I'll try to get back to it shortly when I have some time.

[j-piasecki]

On the old arch, the modal size is set in two places:

1. react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ModalHostShadowNode.kt

   Line 28 in 84ff977

   override fun addChildAt(child: ReactShadowNodeImpl, i: Int) {

   which seems to be invoked only once

2. react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt

   Lines 425 to 427 in 84ff977

   	this@DialogRootViewGroup.reactContext.reactApplicationContext
   	.getNativeModule(UIManagerModule::class.java)
   	?.updateNodeSize(viewTag, viewWidth, viewHeight)

   which is being called from onSizeChanged:

   react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt

   Line 409 in 84ff977

   updateFirstChildView()

The size set in the first point is the same as on the new arch (without status bar height), and the next one is correct. This causes a visible blink, where the wrong layout is visible for one frame:

334412883-8fa9f086-c46e-4a23-84fc-1aa3dc7928bb.mov

---

On the new arch, the modal size is also set in two places:

1. Inside updateState:

   react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostManager.kt

   Line 140 in 84ff977

   view.updateState(modalSize.x, modalSize.y)

2. react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt

   Line 418 in 84ff977

   updateState(viewWidth, viewHeight)

   which, again, is being called from onSizeChanged:

   react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt

   Line 409 in 84ff977

   updateFirstChildView()

The difference here is that the first step on the new architecture is executed multiple times, which causes the values from onSizeChanged to be overridden with ones returned from the helper. The step inside onSizeChanged for the new arch was added in 57d1f8a (titled Update Modal State when there is a change on the size of the screen).

The state being updated constantly with the values returned by the helper seems a bit weird to me, given that the second path also exists and it's effectively useless because of that. I've updated the PR, so now it updates the state with values from the helper only initially.

cc. @sammy-SC @cortinico

[Foreverjie]

Any update on this?