-
Notifications
You must be signed in to change notification settings - Fork 13.3k
fix(modal): prevent ion-content collapsing at content-based heights #31413
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
base: main
Are you sure you want to change the base?
Changes from all commits
4e8ecc2
e63841c
21e6c23
51e8923
54b1596
d5b4e75
7298645
6738339
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -46,8 +46,10 @@ export class Content implements ComponentInterface { | |||||
| private scrollEl?: HTMLElement; | ||||||
| private backgroundContentEl?: HTMLElement; | ||||||
| private isMainContent = true; | ||||||
| private sizeToContent = false; | ||||||
| private resizeTimeout: ReturnType<typeof setTimeout> | null = null; | ||||||
| private fullscreenResizeObserver?: ResizeObserver; | ||||||
| private sizeToContentObserver?: MutationObserver; | ||||||
| private inheritedAttributes: Attributes = {}; | ||||||
|
|
||||||
| private tabsElement: HTMLElement | null = null; | ||||||
|
|
@@ -190,6 +192,7 @@ export class Content implements ComponentInterface { | |||||
|
|
||||||
| // Re-observe on reattach, since componentDidLoad only fires once. | ||||||
| this.setupFullscreenResizeObserver(); | ||||||
| this.setupSizeToContentObserver(); | ||||||
| } | ||||||
|
|
||||||
| componentDidLoad() { | ||||||
|
|
@@ -222,6 +225,7 @@ export class Content implements ComponentInterface { | |||||
| } | ||||||
|
|
||||||
| this.destroyFullscreenResizeObserver(); | ||||||
| this.destroySizeToContentObserver(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -258,6 +262,51 @@ export class Content implements ComponentInterface { | |||||
| this.fullscreenResizeObserver.observe(this.el); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A modal's `--height` can be changed at runtime with no event to react | ||||||
| * to, either by setting the property directly or by toggling a class that | ||||||
| * changes which rule wins. Both of those mutate an attribute on the modal, | ||||||
| * so watch for that and re-evaluate. Viewport driven changes are already | ||||||
| * covered by the `resize` listener. | ||||||
| */ | ||||||
| private setupSizeToContentObserver() { | ||||||
|
thetaPC marked this conversation as resolved.
|
||||||
| if (!Build.isBrowser || typeof MutationObserver === 'undefined') { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| if (this.sizeToContentObserver !== undefined) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const modal = this.el.closest('ion-modal'); | ||||||
| if (modal === null) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| this.sizeToContentObserver = new MutationObserver(() => this.updateSizeToContent()); | ||||||
| this.sizeToContentObserver.observe(modal, { attributes: true, attributeFilter: ['style', 'class'] }); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JSDoc says this covers toggling a class that changes which rule wins, but that only holds when the class is on the modal. Two shapes that both collapse to a 44px header-only modal while
I don't think any observer on the modal can catch those, since neither one mutates an attribute on it. Would it be easier for |
||||||
| } | ||||||
|
|
||||||
| private destroySizeToContentObserver() { | ||||||
| if (this.sizeToContentObserver !== undefined) { | ||||||
| this.sizeToContentObserver.disconnect(); | ||||||
| this.sizeToContentObserver = undefined; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Re-renders when the overlay is no longer sized the way the last render | ||||||
| * assumed. Read in a `readTask` because resolving the custom property forces | ||||||
| * a style recalculation. | ||||||
| */ | ||||||
| private updateSizeToContent() { | ||||||
| readTask(() => { | ||||||
| if (this.shouldSizeToContent() !== this.sizeToContent) { | ||||||
| forceUpdate(this); | ||||||
| } | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| private destroyFullscreenResizeObserver() { | ||||||
| if (this.fullscreenResizeObserver !== undefined) { | ||||||
| this.fullscreenResizeObserver.disconnect(); | ||||||
|
|
@@ -310,6 +359,38 @@ export class Content implements ComponentInterface { | |||||
| return forceOverscroll === undefined ? mode === 'ios' && isPlatform('ios') : forceOverscroll; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Whether to size the component to its content height. | ||||||
| * | ||||||
| * This applies inside popovers and modals with a content-based `--height`, | ||||||
| * where the overlay does not provide the content with a definite height | ||||||
| * to fill. | ||||||
| * | ||||||
| * Only `--height` is consulted. Styling the wrapper directly, such as | ||||||
| * `ion-modal::part(content) { height: fit-content; }`, does not change | ||||||
| * `--height` and therefore cannot be observed. `--height` is the only | ||||||
| * supported way to opt into content-based sizing. | ||||||
| */ | ||||||
| private shouldSizeToContent() { | ||||||
| if (hostContext('ion-popover', this.el)) { | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| const modal = this.el.closest('ion-modal'); | ||||||
| if (modal === null) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| const height = getComputedStyle(modal).getPropertyValue('--height').trim(); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Height keywords are case-insensitive, so |
||||||
|
|
||||||
| /** | ||||||
| * Compared as a suffix so a value carrying a vendor prefix is still | ||||||
| * recognized, such as `-webkit-fit-content` or the `-moz-fit-content` | ||||||
| * that Firefox needed before 94. | ||||||
| */ | ||||||
| return CONTENT_SIZED_HEIGHTS.some((value) => height.endsWith(value)); | ||||||
| } | ||||||
|
|
||||||
| private resize() { | ||||||
| /** | ||||||
| * Only force update if the component is rendered in a browser context. | ||||||
|
|
@@ -320,6 +401,13 @@ export class Content implements ComponentInterface { | |||||
| * TODO: Remove if STENCIL-834 determines Stencil will account for this. | ||||||
| */ | ||||||
| if (Build.isBrowser) { | ||||||
| /** | ||||||
| * A window resize can cross a media query that changes the modal's | ||||||
| * `--height`. The content's own offsets are unchanged, so neither branch | ||||||
| * below re-renders and the class from the last render would go stale. | ||||||
| */ | ||||||
| this.updateSizeToContent(); | ||||||
|
|
||||||
| if (this.fullscreen) { | ||||||
| readTask(() => this.readDimensions()); | ||||||
| } else if (this.cTop !== 0 || this.cBottom !== 0) { | ||||||
|
|
@@ -538,7 +626,7 @@ export class Content implements ComponentInterface { | |||||
| class={createColorClasses(this.color, { | ||||||
| [mode]: true, | ||||||
| 'content-fullscreen': this.fullscreen, | ||||||
| 'content-sizing': hostContext('ion-popover', this.el), | ||||||
| 'content-sizing': (this.sizeToContent = this.shouldSizeToContent()), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assigning to The bigger one is Could |
||||||
| overscroll: forceOverscroll, | ||||||
| [`content-${rtl}`]: true, | ||||||
| })} | ||||||
|
|
@@ -579,6 +667,12 @@ export class Content implements ComponentInterface { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * `ion-modal` `--height` values that size the modal to its contents, leaving | ||||||
| * children an indefinite height to resolve against. | ||||||
| */ | ||||||
| const CONTENT_SIZED_HEIGHTS = ['auto', 'fit-content', 'min-content', 'max-content']; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||||||
|
|
||||||
| const getParentElement = (el: any) => { | ||||||
| if (el.parentElement) { | ||||||
| // normal element with a parent element | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,12 @@ | |
| --max-width: auto; | ||
| --height: 100%; | ||
| --min-height: auto; | ||
| --max-height: auto; | ||
| /** | ||
| * Clamps a content-sized `--height` (auto, fit-content, ...) to the | ||
| * overlay, giving the wrapper's flex children something to shrink | ||
| * toward so `ion-content` scrolls instead of overflowing. | ||
| */ | ||
| --max-height: 100%; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this considered a breaking change since consumers are used to having it as
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I wouldn't consider this a breaking change because
That means the actual change is From there, the cases where the computed value actually changes are all cases that were already broken:
Anyone who explicitly sets Additionally, CSS variable defaults are not tracked in the public API.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A content-sized modal with overflowing content now fills the screen, but |
||
| --overflow: hidden; | ||
| --border-radius: 0; | ||
| --border-width: 0; | ||
|
|
@@ -87,8 +92,16 @@ ion-backdrop { | |
| /** | ||
| * The wrapper receives programmatic focus for screen readers but should not | ||
| * show a visible focus ring, which is meant only for keyboard navigation. | ||
| * | ||
| * A flex layout is required for the wrapper to size itself to its content | ||
| * when the modal is content-sized (`--height` is auto, fit-content, ...). | ||
| * This makes it so that the content can scroll when it overflows the wrapper. | ||
| */ | ||
| .modal-wrapper { | ||
| display: flex; | ||
|
|
||
| flex-direction: column; | ||
|
|
||
| outline: none; | ||
| } | ||
|
|
||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-arms the observer but nothing recomputes the class, and the only thing that ever writes
sizeToContentis the render expression. Stencil doesn't re-render on reconnect, so the old value just sits there.A modal at the default height is 800px. Detach the content, set
--height: fit-content, put it back, and you get 44px with the content at 0, which is the bug this PR is fixing. Any unrelated style write on the modal afterwards snaps it to 244px, so the observer's fine, it's just that nothing evaluates on reconnect. The other direction stickscontent-sizingon and leaves a full-height modal withcontain: none.Anything that unmounts and remounts the modal body across a height change hits it, so
*ngIf,v-ifand friends. That'd cover it, though makingsizeToContenta@Statewould be sturdier.