feat(modal): mount/unmount on open change

[timfuhrmann]

Closes https://github.com/carbon-design-system/carbon/issues/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
• [x] 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[bot]]

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

Name	Link
🔨 Latest commit	c7993f4956bc6fab1485ee7bcd4b8397880715dd
🔍 Latest deploy log	https://app.netlify.com/projects/v11-carbon-web-components/deploys/68cc72a292bc3f00084d01e6
😎 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[bot]]

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	c7993f4956bc6fab1485ee7bcd4b8397880715dd
🔍 Latest deploy log	https://app.netlify.com/projects/v11-carbon-react/deploys/68cc72a2dadfb9000877918c
😎 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.

[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. This should improve DX as the user will be 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? Will add something similar to ComposedModal.

/**
* 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
*/
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, 
* all children and their local states will be unmounted
*/
const CustomModal = ({ onClose }) => {
  const [foo, setFoo] = useState();
  return <Modal onRequestClose={() => setIsOpen(false)} />;
}

const WithComposition = () => {
  const [isOpen, setIsOpen] = useState(false);
  return (
    <ModalPresence open={isOpen}>
      <CustomModal onClose={() => setIsOpen(false)} />
    </ModalPresence>
  );
};

[netlify[bot]]

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	c7993f4956bc6fab1485ee7bcd4b8397880715dd
🔍 Latest deploy log	https://app.netlify.com/projects/carbon-elements/deploys/68cc72a2f9b0af000897537b
😎 Deploy Preview	https://deploy-preview-19531--carbon-elements.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.

[timfuhrmann]

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

/**
* 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
*/
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, 
* all children and their local states will be unmounted
*/
const CustomModal = ({ onClose }) => {
  const [foo, setFoo] = useState();
  return <Modal onRequestClose={() => setIsOpen(false)} />;
}

const WithComposition = () => {
  const [isOpen, setIsOpen] = useState(false);
  return (
    <ModalPresence open={isOpen}>
      <CustomModal onClose={() => setIsOpen(false)} />
    </ModalPresence>
  );
};

Update: Both Modal and ComposedModal are now following said approaches to opt in.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 91.59664% with 20 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 91.92%. Comparing base (30b4f87) to head (c7993f4). :warning: Report is 208 commits behind head on main.

Files with missing lines	Patch %	Lines
...act/src/components/ComposedModal/ComposedModal.tsx	88.82%	19 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19531      +/-   ##
==========================================
+ Coverage   91.39%   91.92%   +0.53%     
==========================================
  Files         485      481       -4     
  Lines       31370    32590    +1220     
  Branches     5430     5529      +99     
==========================================
+ Hits        28670    29958    +1288     
+ Misses       2547     2486      -61     
+ Partials      153      146       -7     

Flag	Coverage Δ
main-packages	85.17% <91.59%> (+0.23%)	:arrow_up:
web-components	97.31% <ø> (+0.14%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[timfuhrmann]

Summary

This PR implements a hook called usePresence that can be applied to whatever element to defer its unmount by subscribing to its ongoing animations. For both Modal & ComposedModal this is hook is integrated via Modal-/ComposedModalPresence, which mounts a React context. While the hook could be used directly in both components without having to create a context, this improves DX by a lot, since a developer can shift the presence boundary to a higher level, especially helpful for monolithic exports such as the Modal or IBM's Tearsheet. However, this addition not mandatory and obviously introduces more complexity than only using the hook would, as it has to handle exclusivity, since only a single modal must read from one context.

TODOs I am aware of:

• [x] Storybook Documentation
• [x] ComposedModal's isOpen state: I have not been able to completely understand the purpose of syncing open with isOpen& wasOpen compared to just using open. The docs are only mentioning controlling the open state by providing open, but nothing in regards to an uncontrolled way of using this component. Anyways, in case it will be necessary, I am happy to adjust my implementation to cover this, if you can fill me in on the expected outcome. Thank you!

[tay1orjones]

Storybook docs were something I was going to mention, I think it'll be really helpful to have those 👍

ComposedModal's isOpen state: I have not been able to completely understand the purpose of syncing open with isOpen& wasOpen compared to just using open. The docs are only mentioning controlling the open state by providing open, but nothing in regards to an uncontrolled way of using this component. Anyways, in case it will be necessary, I am happy to adjust my implementation to cover this, if you can fill me in on the expected outcome. Thank you!

The internal isOpen state exists to support a "semi-controlled" use case where a consumer passes in open={true} and they don't have onClose wired up. In this case, the ComposedModal still should close on a click outside or pressing esc. It was initially added as part of refactoring ComposedModal to be a functional component.

• https://github.com/carbon-design-system/carbon/pull/10060

[timfuhrmann]

@tay1orjones I have implemented both support for the uncontrolled ComposedModal as well as added storybook documentation. Please let me know if you think this is sufficient.