feat(modal): mount/unmount on open change

[timfuhrmann]

Closes #18935

Note

This is a first draft, without comments/annotations, test coverage whatsoever. So far I have only covered Modal without the dialog element. In case we want to move forward with this I am happy to update Dialog, ComposedModal and other relevant components, add test coverage etc. I would like to discuss my current approach first, before I progress though.

As of right now all modals remain mounted in disregard of its open state. This is due to the exit animation, as an unmount will interrupt any transition/animation. Instead of having the modal mounted at all times, this PR proposes to defer an unmount until the exit animation has finished instead.

The current behaviour is a bottleneck in terms of state management and sometimes even performance. Also, it makes devs grab for bad react practices (e.g. resetting state on close, introduction of useEffects to sync initial values, bad composition, etc.) which tremendously increase complexity, the probability of bugs, visual jank and all sorts of side effects, leading to a deep cut in quality. Other component libs use an isExiting state to defer unmounting the modal. This PR follows a similar approach.

This also will tremendously reduce the complexity of modal or other related components, since any logic included, such as event handlers, useEffects, etc. will never run as long as the modal is not opened.

Happy to elaborate on the reasons why and their results, if necessary.

Changelog

New

• usePresence: Hook to defer the update of isOpen to false until all of the ref's animations have finished

const [isPresent, isExiting] = usePresence(ref, open);

• ModalPresence & ModalDialog (naming TBD): these are not 100% necessary internally, however they greatly improve possibilities of composition alongside the monolithic export. Because while the modals' children are unmounted, its calling parent is not. This is fine for components where composition is not a problem (e.g. ComposedModal), since everything can be moved to their children already. But when the component is monolithic and stateful, and state needs to be accessible in the modal's head or footer, it has to be defined in the calling parent. As of right now these states outside of the modal's children would not be unmounted. Exposing both ModalPresence and ModalDialog help to offer this flexibility for the the monolithic modal as well, are not mandatory to use though.

// use it just like before, when no state is necessary or you are fine with not unmounting it
const FooModal = ({ onSubmit }) => {
  const [barState, setBarState] = useState()
  return <Modal onRequestSubmit={() => onSubmit(barState)}  />;
}

// or move presence boundary above calling parent if you want state to be unmounted
const FooModal = ({ open, onSubmit }) => {
  return (
    <ModalPresence open={open}>
      <FooModalDialog onSubmit={onSubmit} />
    </ModalPresence>
  );
}
const FooModalDialog = ({ onSubmit }) => {
  const [barState, setBarState] = useState()

  // queries, mutations, local data management etc. will be cleaned up properly on close

  return <ModalDialog onRequestSubmit={() => onSubmit(barState)} />;
}

Changed

• Updated tests that had been running on modal's closed state, but where actually testing the open state

Testing / Reviewing

1. Go to modal's story with state manager
2. Modal is not present in DOM
3. Open modal
4. Modal is present in DOM
5. Fill input
6. Close modal
7. Exit animation finishes
8. Modal is removed from DOM
9. Open modal again
10. Input is empty

PR Checklist

As the author of this PR, before marking ready for review, confirm you:

• Reviewed every line of the diff
• Updated documentation and storybook examples
• Made sure previous test are passing
• Wrote passing tests that cover this change
• Addressed any impact on accessibility (a11y)
• Tested for cross-browser consistency
• Validated that this code is ready for review and status checks should pass

cc @tay1orjones

[netlify]

✅ Deploy Preview for v11-carbon-web-components ready!

Name	Link
🔨 Latest commit	32d823e
🔍 Latest deploy log	https://app.netlify.com/projects/v11-carbon-web-components/deploys/685aaecf04866f0008dce40d
😎 Deploy Preview	https://deploy-preview-19531--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[netlify]

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	32d823e
🔍 Latest deploy log	https://app.netlify.com/projects/v11-carbon-react/deploys/685aaecfda32710008506875
😎 Deploy Preview	https://deploy-preview-19531--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[tay1orjones]

The direction here looks great. I'm mostly concerned about backwards compatibility and ensuring these changes can be taken by all consumers without any impact. If we can't ensure that, we'll need to guard it somehow.

• A new or existing feature flag
  • Dialog is already marked unstable and behind a flag in Modal/ComposedModal
• A new prop, sometimes can be an existing one if it's an enum or something.
• Opt in through composition
  • Rough idea could be setting a boolean in a new context that Modal could then read from and modify it's internal logic based on if it's wrapped with ModalPresence or not.

I'm curious how that sounds, or what you think might make more sense

[timfuhrmann]

The direction here looks great. I'm mostly concerned about backwards compatibility and ensuring these changes can be taken by all consumers without any impact. If we can't ensure that, we'll need to guard it somehow.

@tay1orjones Good point, backwards compatibility is something I have not considered with this PR yet. Without providing a way to opt in, this indeed is a breaking change, especially for tests, as most teams have not been necessarily opening the modal, before testing its contents.

I like your suggestions and I am always a fan of composition, a feature flag also sounds like a good option. However the direction might depend on whether you are planning to deprecate the version that's always present in the DOM in the future or if you prefer to keep both versions?

Edit:
I actually like a combination of both composition and feature flag. A feature flag makes sense in terms of future adoption as well as generally opting into the new behaviour of the modal. If a user decides to use ModalPresence as described above though, I would assume that he willingly opts in too. Hence ModalPresence could internally include <FeatureFlags enableFoo="true">, instead of a boolean. This should make handling easier as we will consistently only have to check for a feature flag, and the user is already able to use ModalPresence without having to explicitly opt in every time again (in case of incremental migration). What do you think?

[timfuhrmann]

Update: I updated this PR to provide backwards compatibility. As of right now, the feature has to be enabled via feature flag. However, I am still thinking about a way to auto opt-in as mentioned above.

All tests are updated to cover both versions of the modal.

[timfuhrmann]

Update: ModalPresence now also supports auto opt-in when used standalone. @tay1orjones What do you think about this approach to backwards compatibility?

// Backwards compatible solution, no API changes
const WithoutPresence = () => {
  const [isOpen, setIsOpen] = useState(false);
  return <Modal open={isOpen} onRequestClose={() => setIsOpen(false)} />;
};

// Modal will manually opt in via feature flags, including styles
const WithFeatureFlags = () => {
  const [isOpen, setIsOpen] = useState(false);
  return (
    <FeatureFlags enablePresence>
      <Modal open={isOpen} onRequestClose={() => setIsOpen(false)} />
    </FeatureFlags>
  );
};

// Modal will auto opt-in, necessary styles will be applied automatically
const WithComposition = () => {
  const [isOpen, setIsOpen] = useState(false);
  return (
    <ModalPresence open={isOpen}>
      <ModalDialog onRequestClose={() => setIsOpen(false)} />
    </ModalPresence>
  );
};