Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
AJStacy opened this issue Oct 27, 2022 · 10 comments
Closed
2 tasks done

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

AJStacy opened this issue Oct 27, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@AJStacy
Copy link

AJStacy commented Oct 27, 2022

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

@AJStacy AJStacy added the enhancement New feature or request label Oct 27, 2022
@just-boris
Copy link
Member

Is this problem related to this issue theKashey/react-focus-lock#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
Copy link
Author

AJStacy commented Nov 1, 2022

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
Copy link
Member

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

@AJStacy
Copy link
Author

AJStacy commented Nov 1, 2022

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

@just-boris
Copy link
Member

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
Copy link
Author

AJStacy commented Nov 1, 2022

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
Copy link
Member

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
Copy link
Author

AJStacy commented Nov 3, 2022

@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
Copy link
Member

DaemonCahill commented Nov 4, 2022

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants