Fix Modal onDismiss callback is called too early

[bernhardoj]

Description

Currently, onDismiss is called right after setting the isRendered state to false which leads to onDismiss being called before the modal is completely dismissed. This makes chaining action after the modal is dismissed fails, for example, focusing an input.

Test plan

1. Add a text input to the modal example
2. Add onDismiss props to the Modal which will focus the text input
3. Open the modal. Verify the modal shows
4. Close the modal
5. Verify the modal closes and the text input is focused

Currently, the 5th step fails and this PR is trying to fix it.

Before:

Screen.Recording.2023-07-17.at.17.21.37.mov

After:

Screen.Recording.2023-07-17.at.17.19.50.mov

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2618014:

Sandbox	Source
react-native-web-examples	Configuration

[necolas]

Is there any way we can add unit test coverage for this?

[bernhardoj]

@necolas sorry, I have no clue. If you have any ideas in mind, I can try them.

[necolas]

Try to create a unit test that reproduces the test case and fails, then show that it passes with your patch. You didn't share a test case for the issue, so I don't know what it takes to reproduce, but it seems like it's related to animations. A unit test combining animation props with onDismiss might surface the issue

https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/Modal/__tests__/index-test.js

[bernhardoj]

Ignore my previous comment, I think I finally understand what should I do 😅

[bernhardoj]

Here is the test that I come up with:

test('focus text input after modal is dismissed, () => {
  const input = React.createRef();
  const onDismissCallback = jest.fn(() => input.current.focus());
  const { rerender } = render(
    <>
      <TextInput ref={input} />
      <Modal onDismiss={onDismissCallback} visible={true} />
    </>
  );
  expect(onDismissCallback).toBeCalledTimes(0);
  rerender(
    <>
      <TextInput ref={input} />
      <Modal onDismiss={onDismissCallback} visible={false} />
    </>
  );
  expect(onDismissCallback).toBeCalledTimes(1);
  expect(input.current.isFocused()).toBe(true);
});

The test is to make sure focusing text input inside onDismiss callback works (which is what this PR is trying to achieve). It will fail on master and pass on this PR. However, I'm still not sure if this is the correct way because we depend on another component, that is TextInput. Let me know if it's okay!

[bernhardoj]

@necolas hi, what do you think about the above unit test?

[bernhardoj]

@necolas I've added the unit test that is different from what I posted above. Please kindly check it and let me know if I miss something 🙇

[bernhardoj]

@necolas hi, how is the review going?

[necolas]

Looks good, thanks

[bernhardoj]

Thanks for the review!