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.

Sign up for GitHub

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

fix(modal)!: use fixed positioning on host to prevent Safari from clipping content in certain layouts #9545

Merged
merged 5 commits into from
Nov 19, 2024
+24 −39
Merged

fix(modal)!: use fixed positioning on host to prevent Safari from clipping content in certain layouts #9545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15ab733
fix(modal): use fixed positioning on host to prevent Safari from clip…
jcfranco Jun 8, 2024
b7ce24a
drop slottedInShell prop
jcfranco Jun 12, 2024
2001f50
merge pre-lumina dev
jcfranco Nov 18, 2024
f39a8ed
migrate modal files
jcfranco Nov 18, 2024
a96b827
merge dev
jcfranco Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/calcite-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
"@arcgis/lumina": "4.32.0-next.20",
"@esri/calcite-ui-icons": "4.0.0-next.0",
"@floating-ui/dom": "1.6.12",
"@floating-ui/utils": "0.2.8",
"@types/color": "3.0.6",
"@types/sortablejs": "1.15.8",
"color": "4.2.3",
Expand Down
25 changes: 7 additions & 18 deletions packages/calcite-components/src/components/modal/modal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@

:host {
--calcite-modal-scrim-background: #{rgba($calcite-color-neutral-blk-240, $calcite-opacity-85)};
@apply absolute flex inset-0 opacity-0 z-overlay;
}

:host {
@apply fixed flex inset-0 opacity-0 z-overlay;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we need to do this for the dialog component as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to test drive this with modal first. If everything works out smoothly, we could apply to other components before 4.0 since it wouldn't be breaking (see #9545 (comment)).

visibility: hidden !important;
transition:
visibility 0ms linear var(--calcite-internal-animation-timing-slow),
Expand All @@ -29,14 +32,13 @@

.container {
@apply text-color-2
fixed
absolute
inset-0
flex
items-center
justify-center
overflow-y-hidden
opacity-0
z-overlay;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we confirm this overlay isn't needed in some complex slot / stack situations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

opacity-0;
visibility: hidden !important;
transition:
visibility 0ms linear var(--calcite-internal-animation-timing-slow),
Expand Down Expand Up @@ -66,7 +68,7 @@

.scrim {
--calcite-scrim-background: var(--calcite-modal-scrim-background, var(--calcite-color-transparent-scrim));
@apply fixed inset-0 flex overflow-y-hidden;
@apply absolute inset-0 flex overflow-y-hidden;
}

.modal {
Expand Down Expand Up @@ -165,7 +167,6 @@
padding-inline: var(--calcite-modal-padding-internal);
flex: 0 0 auto;
calcite-icon {
@apply pointer-events-none;
vertical-align: -2px;
}
&:focus {
Expand Down Expand Up @@ -402,16 +403,4 @@ slot[name="primary"] {
}
}

/**
* Conditional styles for when Modal is slotted in Shell
*/

.container--embedded {
position: absolute;
pointer-events: auto;
calcite-scrim {
position: absolute;
}
}

@include base-component();
22 changes: 9 additions & 13 deletions packages/calcite-components/src/components/modal/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
JsxNode,
setAttribute,
} from "@arcgis/lumina";
import { getNearestOverflowAncestor } from "@floating-ui/utils/dom";
import {
ensureId,
focusFirstTabbable,
Expand Down Expand Up @@ -154,14 +155,6 @@ export class Modal
/** When `true`, prevents the component from expanding to the entire screen on mobile devices. */
@property({ reflect: true }) docked: boolean;

/**
* This internal property, managed by a containing calcite-shell, is used
* to inform the component if special configuration or styles are needed
*
* @private
*/
@property() embedded = false;

/** When `true`, disables the default close on escape behavior. */
@property({ reflect: true }) escapeDisabled = false;

Expand Down Expand Up @@ -330,7 +323,6 @@ export class Modal
this.mutationObserver?.disconnect();
this.cssVarObserver?.disconnect();
deactivateFocusTrap(this);
this.embedded = false;
}

// #endregion
Expand Down Expand Up @@ -424,7 +416,7 @@ export class Modal
this.titleId = ensureId(this.titleEl);
this.contentId = ensureId(this.contentEl);

if (!this.embedded) {
if (getNearestOverflowAncestor(this.el) === document.body) {
if (totalOpenModals === 0) {
initialDocumentOverflowStyle = document.documentElement.style.overflow;
}
Expand Down Expand Up @@ -458,9 +450,14 @@ export class Modal
}
}

totalOpenModals--;
if (getNearestOverflowAncestor(this.el) === document.body) {
totalOpenModals--;
if (totalOpenModals === 0) {
this.removeOverflowHiddenClass();
}
}

this.opened = false;
this.removeOverflowHiddenClass();
}

private removeOverflowHiddenClass(): void {
Expand Down Expand Up @@ -498,7 +495,6 @@ export class Modal
class={{
[CSS.container]: true,
[CSS.containerOpen]: this.opened,
[CSS.containerEmbedded]: this.embedded,
}}
>
<calcite-scrim class={CSS.scrim} onClick={this.handleOutsideClose} />
Expand Down
4 changes: 4 additions & 0 deletions packages/calcite-components/src/components/shell/shell.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,8 @@ slot[name="panel-top"]::slotted(calcite-shell-center-row) {
inset: 0;
}

slot[name="modals"]::slotted(calcite-modal) {
position: absolute;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to confirm we don't need the pointer-events rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@include base-component();
6 changes: 0 additions & 6 deletions packages/calcite-components/src/components/shell/shell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { PropertyValues } from "lit";
import { LitElement, property, Fragment, h, state, JsxNode } from "@arcgis/lumina";
import { slotChangeGetAssignedElements, slotChangeHasAssignedElement } from "../../utils/dom";
import type { Dialog } from "../dialog/dialog";
import type { Modal } from "../modal/modal";
import type { Sheet } from "../sheet/sheet";
import type { Alert } from "../alert/alert";
import { styles } from "./shell.scss";
Expand Down Expand Up @@ -135,11 +134,6 @@ export class Shell extends LitElement {

private handleModalsSlotChange(event: Event): void {
this.hasModals = !!slotChangeHasAssignedElement(event);
slotChangeGetAssignedElements(event)?.map((el) => {
if (el.tagName === "CALCITE-MODAL") {
(el as Modal["el"]).embedded = true;
}
});
}

private handlePanelTopChange(event: Event): void {
Expand Down
4 changes: 2 additions & 2 deletions packages/calcite-components/src/custom-theme.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ const kitchenSink = (args: Record<string, string>, useTestValues = false) =>
${chips} ${pagination} ${slider}
</div>
<div class="demo-column">
${datePicker} ${tabs} ${label} ${link} ${loader} ${calciteSwitch} ${avatarIcon} ${avatarInitials} ${avatarThumbnail}
${progress} ${handle} ${textArea} ${popover} ${tile} ${tooltip}
${datePicker} ${tabs} ${label} ${link} ${loader} ${calciteSwitch} ${avatarIcon} ${avatarInitials}
${avatarThumbnail} ${progress} ${handle} ${textArea} ${popover} ${tile} ${tooltip}
</div>
${alert} ${navigation}
</div>
Expand Down
Loading