Remove most of `Modal` state updates 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):

https://user-images.githubusercontent.com/21055725/211768547-d3ff49b9-2ce6-4f36-9f0f-91ad102fab28.mov

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

https://user-images.githubusercontent.com/21055725/211768801-df57b295-fa04-4bd8-af35-0057553bf15b.mov

Simply removing the state updates for Modal seems to be solving the problem (at the cost of the modal appearing one frame later), however, I'm not that familiar with the code so it may have some unintended consequences. Updating the implementation of ModalHostHelper::getModalHostSize may be a better solution.

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: be38fbb3f895e36f256a25e9dd86ef76a05797c6 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[bot]]

	Fails
:no_entry_sign:	:clipboard: Missing Changelog - Please add a Changelog to your PR description. See Changelog format

Generated by :no_entry_sign: dangerJS against 5db452f732fc17bca174279e0d8a1a1e91c6f512

[facebook-github-bot]

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

[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[bot]]

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. https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ModalHostShadowNode.kt#L28 which seems to be invoked only once
2. https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt#L425-L427 which is being called from onSizeChanged: https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt#L409

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:

https://github.com/facebook/react-native/assets/21055725/e5c95d26-9dd9-46b9-8bb4-05318e3ec5d9

---

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

1. Inside updateState: https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostManager.kt#L140
2. https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt#L418 which, again, is being called from onSizeChanged: https://github.com/facebook/react-native/blob/84ff977b3726f14e3c045103e2b7c176cabcc185/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt#L409

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 https://github.com/facebook/react-native/commit/57d1f8ae1580874b26374583c4f5cac059d79dd4 (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?