fix(modal): use fixed positioning on host to prevent Safari from clipping content in certain layouts #9545
Conversation
…ping content in certain layouts
jcfranco
added
the
pr ready for visual snapshots
github-actions
bot
added
the
bug
| * Conditional styles for when Modal is slotted in Shell | ||
| */ | ||
|
|
||
| .container.slotted-in-shell { |
There was a problem hiding this comment.
Are these no longer needed? if so, what about the class for slotted-in-shell? We can remove it if not necessary. I think there is a prop as well.
There was a problem hiding this comment.
At least based on screenshot tests and shallow, local testing, it doesn't look like these are needed anymore after the modal style tweaks. This could benefit from some additional testing too. cc @geospatialem
Good catch on the prop. I'll take a look. 👀
There was a problem hiding this comment.
I managed to eliminate the prop by dynamically checking if the overflowing container is the document body.
| * Conditional styles for when Modal is slotted in Shell | ||
| */ | ||
|
|
||
| .container.slotted-in-shell { |
There was a problem hiding this comment.
We set these classes for Alert and Sheet as well in Shell. Should we change those components as well and remove the internal props we set for these?
There was a problem hiding this comment.
I think so. Once we successfully move modal to this pattern, we can look at doing the same for similar components.
There was a problem hiding this comment.
Note for future reference: slottedInShell was renamed to embedded in #9778.
| @@ -129,4 +129,8 @@ slot[name="panel-top"]::slotted(calcite-shell-center-row) { | |||
| inset: 0; | |||
| } | |||
|
|
|||
| slot[name="modals"]::slotted(calcite-modal) { | |||
| position: absolute; | |||
There was a problem hiding this comment.
Is there any way to confirm we don't need the pointer-events rule?
There was a problem hiding this comment.
pointer-events: none are applied to the modal when closed. Once open, this is set to the default, so I think the shell-slotted override isn't necessary.
| inset-0 | ||
| flex | ||
| items-center | ||
| justify-center | ||
| overflow-y-hidden | ||
| opacity-0 | ||
| z-overlay; |
There was a problem hiding this comment.
Can we confirm this overlay isn't needed in some complex slot / stack situations?
There was a problem hiding this comment.
The overlay z-index is already applied on the host, so modals should already be on that layer. Do you have any examples of complex stacking we could use for testing?
* adds `@floating-ui/utils` dep
Yeah, I can see this being risky because of all the different modal configurations and changes going into this. Since the associated issue does have a workaround, we could consider this for v3. cc @geospatialem |
|
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
|
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
github-actions
bot
added
the
Stale
Related Issue: #9416
Summary
This restores
modal's host positioning tofixedto avoid a Safari bug that causes content to be clipped.Note: screenshot test was not added for this scenario since our testing environment uses Chrome.