fix(modal): prevent ion-content collapsing at content-based heights by brandyscarney · Pull Request #31413 · ionic-team/ionic-framework

brandyscarney · 2026-08-28T20:00:55Z

Issue number: resolves #31149

What is the current behavior?

Setting --height to any of the following on an ion-modal containing an ion-content results in the ion-content collapsing to 0 height: auto, fit-content, min-content, or max-content.

What is the new behavior?

Checks if modal --height is set to auto, fit-content, min-content, or max-content and styles the ion-content, ion-nav and .ion-page appropriately if so
Watches for changes to --height on ion-modal to dynamically add and remove the class that sizes the ion-content to its content
Updates modal --max-height to 100% which allows the content to scroll instead of overflowing and being clipped
Adds a preview test for different scenarios where ion-modal has --height set
Adds e2e tests for the various --height scenarios

Does this introduce a breaking change?

Yes
No

Other information

Dev build: 9.0.1-dev.11788212611.154b1596

Preview: Modal: Content Height

Docs PR: ionic-team/ionic-docs#4689

resolves #31149

vercel · 2026-08-28T20:01:01Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
ionic-framework	Ready	Preview	Sep 2, 2026 9:21pm UTC

brandyscarney · 2026-08-31T21:42:42Z

+    }
+
+    const height = getComputedStyle(modal).getPropertyValue('--height').trim();
+    return CONTENT_SIZED_HEIGHTS.includes(height);


Yes this PR is still a work in progress! 🙂

thetaPC

Mainly waiting on the breaking change question to be resolved.

thetaPC · 2026-09-02T17:37:17Z

+   * overlay, giving the wrapper's flex children something to shrink 
+   * toward so `ion-content` scrolls instead of overflowing.
+   */
+  --max-height: 100%;


Is this considered a breaking change since consumers are used to having it as auto?

No, I wouldn't consider this a breaking change because auto was never a valid value for max-height in the first place.

auto isn't listed as a valid value in the docs for max-height. As a result, max-height: auto is invalid and the property fell back to its initial value, none. If you inspect any .modal-wrapper prior to this change you will see the max-height is computed as none:

That means the actual change is none → 100%.

From there, the cases where the computed value actually changes are all cases that were already broken:

--height: 100% (the default) and every built-in variant (calc(100% - 40px), sheet, card, inset heights) are all ≤ 100%, so the clamp has no effect and rendering remains identical.

The iOS card modal sets --max-height: 1000px explicitly, so it's unaffected.

A --height taller than the overlay (e.g. 800px in a 600px viewport, or a content-based height with tall content) previously overflowed the host. Since :host has contain: strict, that overflow was clipped at both the top and bottom, leaving some of the content unreachable. Clamping the height so ion-content scrolls instead is a fix.

Anyone who explicitly sets --max-height: auto still ends up with none, since their override is just as invalid as the old default was. And setting --max-height to anything else will still take precedence.

Additionally, CSS variable defaults are not tracked in the public API. api.txt records CSS custom property names only, so there are no generated docs or API diff changes.

thetaPC

LGTM

ShaneK

Awesome work! Looks really good overall, and the test page covers a lot of ground. I did find a few cases where the content still collapses though. Setting --height from an ancestor rule, using --height: AUTO, and reconnecting across a height change all miss the class and leave a header-only modal, so the original bug is still reachable. Details in the comments, and let me know if I'm missing something!

ShaneK · 2026-09-04T20:23:39Z


    // Re-observe on reattach, since componentDidLoad only fires once.
    this.setupFullscreenResizeObserver();
+    this.setupSizeToContentObserver();


Suggested change

this.setupSizeToContentObserver();

this.setupSizeToContentObserver();

this.updateSizeToContent();

This re-arms the observer but nothing recomputes the class, and the only thing that ever writes sizeToContent is 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 sticks content-sizing on and leaves a full-height modal with contain: none.

Anything that unmounts and remounts the modal body across a height change hits it, so *ngIf, v-if and friends. That'd cover it, though making sizeToContent a @State would be sturdier.

ShaneK · 2026-09-04T20:23:42Z

+    }
+
+    this.sizeToContentObserver = new MutationObserver(() => this.updateSizeToContent());
+    this.sizeToContentObserver.observe(modal, { attributes: true, attributeFilter: ['style', 'class'] });


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 getComputedStyle on the modal happily reports --height: fit-content:

body.compact ion-modal { --height: fit-content; }, class on body

ion-modal { --height: var(--dlg-h); } with --dlg-h toggled on :root, same shape as ion-palette-dark

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 ion-modal to push the signal down? It's already re-reading --height on present, breakpoint and resize.

ShaneK · 2026-09-04T20:23:45Z

+      return false;
+    }
+
+    const height = getComputedStyle(modal).getPropertyValue('--height').trim();


Suggested change

const height = getComputedStyle(modal).getPropertyValue('--height').trim();

const height = getComputedStyle(modal).getPropertyValue('--height').trim().toLowerCase();

Height keywords are case-insensitive, so --height: AUTO is valid CSS but doesn't match here. You get no content-sizing and a 44px header-only modal.

ShaneK · 2026-09-04T20:23:48Z

          [mode]: true,
          'content-fullscreen': this.fullscreen,
-          'content-sizing': hostContext('ion-popover', this.el),
+          'content-sizing': (this.sizeToContent = this.shouldSizeToContent()),


Assigning to this.sizeToContent inside the class object is the only one of its kind in core/src, and anyone tidying it into a plain call later would kill the runtime reactivity without a test to catch them.

The bigger one is getComputedStyle running from render, which Stencil does in the write task, so every ion-content render forces a style recalc. The comment on updateSizeToContent() argues against exactly that and uses a readTask.

Could sizeToContent be a @State computed in componentWillLoad plus the observer and resize paths? That'd get the read out of the write phase and fix the reconnect staleness too.

ShaneK · 2026-09-04T20:23:51Z

+ * `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'];


The hasCustomModalDimensions helper in safe-area-utils already reads --height off the modal the same way, with its own FULLSCREEN_SIZE_VALUES. Two keyword sets in two components that have to stay in sync, and the --max-height change makes them disagree in at least one case.

ShaneK · 2026-09-04T20:24:07Z

+           * a real box count. Sampled per frame because the window where both
+           * are laid out lasts only as long as the slide.
+           */
+          for (let i = 0; i < 60; i++) {


On timeout this returns an empty array and the caller asserts length > 1, so a failure reads Expected: > 1, Received: 0 whether the fix regressed, the transition never started, or a loaded runner outran 60 frames against a ~540ms transition. A performance.now() deadline and a discriminated result would say which.

The push above is floating and unhandled too, so a rejection comes back as an unhandled page rejection instead of a failure. And I think you can just use HTMLIonNavElement here? Shouldn't need any.

ShaneK · 2026-09-04T20:24:10Z

+          <button class="expand" id="open-toggle">--height: toggled at runtime</button>
+
+          <h2>Known gaps</h2>
+          <button class="expand red" id="open-part" color="danger">::part(content)</button>


Suggested change

<button class="expand red" id="open-part" color="danger">::part(content)</button>

<button class="expand red" id="open-part">::part(content)</button>

The color attribute doesn't do anything on a native button, the red class is doing all of it. Leftover from the ion-button conversion I'd guess.

ShaneK · 2026-09-04T20:24:13Z

+/** 
+ * Danger style for buttons.
+ */
+ion-content button.red:not([class*="sc-ion-"]),


This file loads into every test page and gets injected into every setContent test, and it had exactly two button rules before this, both generic. Could this one live in the page's own style block instead? It's one button on one page and that block's already a hundred lines. Up to you though.

ShaneK · 2026-09-04T20:24:16Z

+   * Override the minimum height a flex item gets, which defaults to 
+   * use the height of its own content. Without this, a modal sized 


Suggested change

* Override the minimum height a flex item gets, which defaults to

* use the height of its own content. Without this, a modal sized

* Override the minimum height a flex item gets, which defaults to

* the height of its own content. Without this, a modal sized

That fixes these two lines and the dropped word. There are 19 in total though, all in the new comment blocks, spread across this file, the new test page, modal.scss and the testing stylesheet. The root .editorconfig sets trim_trailing_whitespace, and nothing catches it since prettier's globs skip scss and css.

ShaneK · 2026-09-04T20:24:20Z

+              </ion-toolbar>
+            </ion-header>
+            <ion-content>
+              <div class="wrapper"><ion-list class="short" lines="none"></ion-list></div>


There's no rule for wrapper anywhere, not on the page and not in the shared testing stylesheet. It came from the custom-dialog page, which does have a margin-bottom for it, and the copy left that behind.

I checked whether it mattered for the no-content modal now that .modal-wrapper is flex, and it doesn't, the delegate wraps slotted content in an .ion-page so the div's never a flex item. All 12 are inert.

fix(modal): prevent ion-content collapsing at content-based heights

4e8ecc2

resolves #31149

github-actions Bot added the package: core @ionic/core package label Aug 28, 2026

vercel Bot deployed to Preview August 28, 2026 20:05 View deployment

test(modal): add content-height preview test

e63841c

vercel Bot deployed to Preview August 28, 2026 20:49 View deployment

brandyscarney added 3 commits August 31, 2026 15:37

test(modal): reorder modals, don't use ionic buttons

21e6c23

fix(content): watch for changes to --height and respect prefixed values

51e8923

test(modal): add e2e tests for various --height values

54b1596

vercel Bot deployed to Preview August 31, 2026 21:37 View deployment

brandyscarney commented Aug 31, 2026

View reviewed changes

brandyscarney mentioned this pull request Sep 1, 2026

docs(modal): reorganize Styling and add Sizing to Content section ionic-team/ionic-docs#4689

Open

brandyscarney marked this pull request as ready for review September 2, 2026 17:26

brandyscarney requested a review from a team as a code owner September 2, 2026 17:26

brandyscarney requested a review from ShaneK September 2, 2026 17:26

thetaPC reviewed Sep 2, 2026

View reviewed changes

brandyscarney added 2 commits September 2, 2026 16:47

style: comment the test config

d5b4e75

style: update comment to drop TODO

7298645

vercel Bot deployed to Preview September 2, 2026 20:55 View deployment

style: update comment to specify the limitation on --height

6738339

brandyscarney requested a review from thetaPC September 2, 2026 21:16

vercel Bot deployed to Preview September 2, 2026 21:21 View deployment

thetaPC approved these changes Sep 2, 2026

View reviewed changes

ShaneK requested changes Sep 4, 2026

View reviewed changes

	this.setupSizeToContentObserver();
	this.setupSizeToContentObserver();
	this.updateSizeToContent();

	const height = getComputedStyle(modal).getPropertyValue('--height').trim();
	const height = getComputedStyle(modal).getPropertyValue('--height').trim().toLowerCase();

	<button class="expand red" id="open-part" color="danger">::part(content)</button>
	<button class="expand red" id="open-part">::part(content)</button>

		* Override the minimum height a flex item gets, which defaults to
		* use the height of its own content. Without this, a modal sized

Uh oh!

Conversation

brandyscarney commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

vercel Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thetaPC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thetaPC left a comment

Choose a reason for hiding this comment

Uh oh!

ShaneK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brandyscarney commented Aug 28, 2026 •

edited

Loading

vercel Bot commented Aug 28, 2026 •

edited

Loading