search

cloudscape-design / components

React components for Cloudscape Design System
https://cloudscape.design/
Apache License 2.0
2.39k stars 158 forks source link

[Feature Request]: Make Modal Scroll Locking Configurable #423

Closed AJStacy closed 2 years ago

AJStacy commented 2 years ago

Description

Currently when using the CloudScape Modal component it targets the <body> tag by default when injecting itself into the DOM when visible.

The CloudScape Modal component currently provides an attribute for altering where the Modal DOM nodes are injected through the modalRoot property.

The problem is that although you are able to specify an alternate modal root node, the document <body> tag is still targeted for padding and scroll locking purposes.

When using the Modal component I would expect that if I specify a different modalRoot that the target modal root node would also be the target for scroll locking and padding.

The source of the problem is here: https://github.com/cloudscape-design/components/blob/main/src/modal/body-scroll.ts

Possible Solutions:

  1. Pass the modalRoot value into the enableScrolling(), disableScrolling(), etc. functions and default them to the body if a modalRoot isn't provided as a parameter.
  2. Create a scrollLock boolean property on the Modal component that allows the user to disable those features (thus preventing document.body alterations).

Backstory:

This problem arose through a project that is implementing micro-frontends using Web Components and Shadow DOM to wrap multiple React applications. The Modal window nodes must be rendered inside of the web component in order to receive styles and to prevent them from leaking out into the global scope. By using the modalRoot property I was able to change where the modal would render and it fixed our issue. However, CloudScape is modifying the document body which is unacceptable for a micro-frontend use case as it is modifying an item within the global application scope. Everything the micro-frontend application does needs to be limited to the scope of the shadow root of the web component.

Screenshot:

This screenshot illustrates how the modal DOM nodes are being applied inside of my web component shadow DOM using the modalRoot prop of the Modal, however, when the modal is opened, styles and classes for scroll locking are applied to the document body.

Screen Shot 2022-11-01 at 10 00 45 AM

Reproduced Issue Code:

https://codesandbox.io/s/modalrootdemo-hlz2j8

Code of Conduct

just-boris commented 2 years ago

Is this problem related to this issue https://github.com/theKashey/react-focus-lock/issues/188 (already fixed, even though github displays it as open).

If not, could you provide a reproducible example so we can do a proper investigation?

AJStacy commented 2 years ago

This is unrelated to the focus-lock issue. This issue surrounds how Modal windows (and Table Preferences) inject classes, styles, and child nodes on the document.body tag by default.

With Modal components you can set the modalRoot (https://cloudscape.design/components/modal/?tabId=api) which changes where the underlying React Portal injects the DOM nodes for a modal, but the component still attempts to apply classes and styles to the document <body> tag.

Ideally when you change the modalRoot on a Modal component it would also change the target for the scroll locking styles and classes.

I will update the ticket with screenshots showing the behavior.

just-boris commented 2 years ago

Could you share a reproducible demo on codesandbox (starting off this template), so we can make a deeper investigation?

AJStacy commented 2 years ago

Here is a link to the reproduced issue: https://codesandbox.io/s/modalrootdemo-hlz2j8

just-boris commented 2 years ago

Checked the demo. What do you find expected and not expected in this demo? Not in terms of class placement, but in the end user experience?

AJStacy commented 2 years ago

If you read the backstory section above, I need to be able to restrict all style, class, and DOM changes to be within a Web Component for purposes of use with a micro-frontend architecture. Styles and dom nodes applied to the top-level body tag are unacceptable for these kinds of integrations.

In my current use case, we will have multiple wrapped react applications running side-by-side each using Cloudscape Design. If both apps try to control the body simultaneously it will not only conflict with the shell application, but the remote applications can conflict.

The desired behavior would be for the element specified as the modalRoot to also be the target for scroll locking styles.

just-boris commented 2 years ago

We only add styles to <body> along with using position:fixed to display the modal full-screen and prevent the underlying document from scrolling.

It by design has to be attached there, can't see an alternative.

P.S. modal dialogs are by their definition are blocking all underlying content. So using a monolith or a distributed frontend does not matter for the final UX.

AJStacy commented 2 years ago

@just-boris , respectfully, this 100% can be accomplished. In order to help illustrate the problem and solution, I've went ahead and forked cloudscape components. I have created a branch with the small number of changes that would be required to accommodate this issue. I've also created a demo page where you can simulate the experience I am talking about.

https://github.com/cloudscape-design/components/pull/444/files

DaemonCahill commented 2 years ago

Hi @AJStacy!

We discussed this issue internally this morning. On a purely technical level we understand why you want to encapsulate all component behavior to a given shadow root. That being said, we have several features throughout Cloudscape that necessitate breaking the encapsulation between components. It is still unclear to us what specific collision you expect might happen on the body element with multiple micro-frontend applications. By design the modal component should render all other content on the page inert which should effectively prevent this from happening. Which brings us to the second proposed solution allowing for opting out of the scroll lock. At the moment we don't have any intention of implementing this as an option for our users.

just-boris commented 2 years ago

Closing due to inactivity. Feel free to reopen with additional comments