fix(modal): prevent ion-content collapsing at content-based heights - #31413
fix(modal): prevent ion-content collapsing at content-based heights#31413brandyscarney wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| } | ||
|
|
||
| const height = getComputedStyle(modal).getPropertyValue('--height').trim(); | ||
| return CONTENT_SIZED_HEIGHTS.includes(height); |
There was a problem hiding this comment.
Yes this PR is still a work in progress! 🙂
thetaPC
left a comment
thetaPC
left a comment
There was a problem hiding this comment.
Mainly waiting on the breaking change question to be resolved.
| * Compared as a suffix so a value carrying only a vendor prefix is still | ||
| * recognized, such as the `-moz-fit-content` that Firefox needs before 94. | ||
| * | ||
| * TODO: replace with `CONTENT_SIZED_HEIGHTS.includes(height)` once the |
There was a problem hiding this comment.
Can we create a ticket to associate this todo?
There was a problem hiding this comment.
I am not even sure we need to do this really. It doesn't hurt to keep this in indefinitely, and I'm not sure what the goal of the ticket would be. Dropping Firefox support for what reason?
| * overlay, giving the wrapper's flex children something to shrink | ||
| * toward so `ion-content` scrolls instead of overflowing. | ||
| */ | ||
| --max-height: 100%; |
There was a problem hiding this comment.
Is this considered a breaking change since consumers are used to having it as auto?
There was a problem hiding this comment.
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: 1000pxexplicitly, so it's unaffected. - A
--heighttaller than the overlay (e.g.800pxin a600pxviewport, or a content-based height with tall content) previously overflowed the host. Since:hosthascontain: strict, that overflow was clipped at both the top and bottom, leaving some of the content unreachable. Clamping the height soion-contentscrolls 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.
| * for that and re-evaluate. Viewport driven changes are already covered by | ||
| * the `resize` listener. | ||
| */ | ||
| private setupSizeToContentObserver() { |
There was a problem hiding this comment.
Unless I missed it, we should also add the limitation note in this file just in case we forget about it since it's only in docs.
There was a problem hiding this comment.
I assume you're talking about the limitation of setting --height on ion-modal and it not looking for height on nested elements. Let me know if I am misunderstanding: 6738339
2 participants
Issue number: resolves #31149
What is the current behavior?
Setting
--heightto any of the following on anion-modalcontaining anion-contentresults in theion-contentcollapsing to0height:auto,fit-content,min-content, ormax-content.What is the new behavior?
--heightis set toauto,fit-content,min-content,ormax-contentand styles theion-content,ion-navand.ion-pageappropriately if so--heightonion-modalto dynamically add and remove the class that sizes theion-contentto its content--max-heightto100%which allows the content to scroll instead of overflowing and being clippedion-modalhas--heightset--heightscenariosDoes this introduce a breaking change?
Other information
Dev build:
9.0.1-dev.11788212611.154b1596Preview: Modal: Content Height
Docs PR: ionic-team/ionic-docs#4689