Skip to content

fix(ui5-illustrated-message): vertical responsiveness corrected - #13795

Merged
kineticjs merged 9 commits into
mainfrom
imIssues
Jul 28, 2026
Merged

fix(ui5-illustrated-message): vertical responsiveness corrected#13795
kineticjs merged 9 commits into
mainfrom
imIssues

Conversation

@kineticjs

@kineticjs kineticjs commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fixes: 12673

Problem:
When design is Auto, ui5-illustrated-message automatically selects the illustration size (Scene/Dialog/Spot/Dot/Base) by comparing the container width and height with fixed breakpoints. The fixed height thresholds, however, cannot alone reliably determine the correct size because, besides the illustration, the component can also contain a title, subtitle, and actions of arbitrary height.

Solution:
Replace checks against fixed height thresholds with a check of whether the actual content height (illustration + title + subtitle + actions) fits within the container at the current illustration size — decreasing the size if the content overflows.
This also makes the workaround for issue #5852 (where content flickered endlessly between showing and hiding a vertical scrollbar) redundant, since vertical responsiveness now ensures no scrollbar appears in the first place.

@kineticjs
kineticjs temporarily deployed to netlify-preview July 7, 2026 16:32 — with GitHub Actions Inactive
@kineticjs
kineticjs temporarily deployed to netlify-preview July 7, 2026 16:34 — with GitHub Actions Inactive
@sap-ui5-webcomponents-release

Copy link
Copy Markdown

@kineticjs
kineticjs temporarily deployed to netlify-preview July 8, 2026 06:47 — with GitHub Actions Inactive
@kineticjs
kineticjs temporarily deployed to netlify-preview July 8, 2026 12:19 — with GitHub Actions Inactive
@kineticjs
kineticjs marked this pull request as ready for review July 8, 2026 12:45

@NakataCode NakataCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two things that are worth checking:


1. Height cache after theme change

Theme changes re-render via renderDeferred without going through _invalidate, so onInvalidation never fires and _contentHeightForMedia isn't cleared. If a larger-font theme caused a downgrade (e.g. "scene" → "dialog"), switching to a smaller-font theme keeps the stale cached height, and _applyMedia will keep rejecting "scene" even though it would now fit. Since _checkHeightConstraints only writes to the cache on overflow, and the downgraded media fits, nothing re-measures until a resize or content change.

2. Height adaptation for fixed (non-Auto) designs

The old code called _adjustHeightToFitContainer() for all fixed designs, which added the fit-content class so the SVG could scale down into small containers. Now the height logic only runs for design === Auto, and the fit-content CSS was removed. So a fixed design="Dialog" in a small container (e.g. 200px) would overflow instead of scaling the illustration.

A themeAware component re-renders via renderDeferred from reRenderAllUI5Elements,
which bypasses _invalidate. onInvalidation therefore never fires on a theme
switch, and _contentHeightForMedia accumulates stale entries across themes: a
theme whose typography once caused scene -> dialog leaves an oversized scene
height in the cache; switching to a theme whose typography would now fit keeps
the stale entry, so _applyMedia keeps rejecting scene.

Subscribe to attachThemeLoaded in onEnterDOM (detach in onExitDOM) and clear
_contentHeightForMedia in the listener. The framework's own themeAware
re-render then re-measures fresh via onAfterRendering ->
_checkHeightConstraints -> _applyMedia.

Also document the load-bearing scrollHeight > clientHeight guard in
_checkHeightConstraints so a future contributor is not tempted to remove it.

The Cypress test poisons the internal cache with an out-of-range value, fires
setTheme, and asserts the media does not downgrade — pinning the contract that
the cache is cleared on theme change without depending on the pixel-boundary
between two themes' typographies.
@kineticjs
kineticjs temporarily deployed to netlify-preview July 20, 2026 14:02 — with GitHub Actions Inactive
@kineticjs
kineticjs temporarily deployed to netlify-preview July 20, 2026 14:09 — with GitHub Actions Inactive
@kineticjs

kineticjs commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, regarding:

  1. Height cache after theme change

Good finding, added code to clear the cache upon theme change;

  1. Height adaptation for fixed (non-Auto) designs

Here we can discuss the finding with the team; For now, for non-Auto designs I've left it as app-side responsibility to ensure the design they chose fits the container -> for responsive behaviour they can switch to "Auto"

@kineticjs
kineticjs requested a review from NakataCode July 21, 2026 07:45

@plamenivanov91 plamenivanov91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey, I reworked the approach here a bit. The cache idea was solid but I think we can get the same result more simply.

The root issue with the original BREAKPOINTS_HEIGHT wasn't the thresholds themselves — it was that the old code tried to use height and width in two separate passes (width first, then height via requestAnimationFrame), which caused the flicker and the inconsistency. The cache in this PR fixed that, but it brought its own complexity: stale entries, theme invalidation, scrollHeight/clientHeight comparisons, and a growing list of edge cases to guard against.

What I went with instead: just feed both offsetWidth and offsetHeight into a single comparison per tier, same || logic OpenUI5 uses:

if (width <= BP.BASE || height <= BPH.BASE) → BASE
if (width <= BP.DOT || height <= BPH.DOT) → DOT
// ...
offsetHeight handles the height: auto case naturally — when nothing constrains the height, offsetHeight just equals the content height which is always above the thresholds, so it never interferes with width-only behaviour. When the height IS constrained (the bug case), it reflects that immediately on every resize, no measurement needed.

As a nice side effect, this also let us clean up a bunch of other things that were only there to support the old approach — the _adjustHeightToFitContainer method, its requestAnimationFrame call, the ui5-illustrated-message-illustration-fit-content CSS class with its absolute-position trick, and all the _lastKnownOffset* tracking fields. The CSS changes in the PR (moving padding to .ui5-illustrated-message-inner, the min-height: 0 restructure) are all still in — those are good and unrelated to the algorithm.

Also kept BREAKPOINTS_HEIGHT — the values are from the visual design spec (aligned with OpenUI5: 451/296/154/87) and should stay unless design explicitly says otherwise. The previous version removed them as a workaround, not because they were wrong.

@plamenivanov91 plamenivanov91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On second review iteration, seems like the more simply variant I suggested in my previous comment is unstable. We risk losing time in stabilising it (or straight up wasting time), therefore we run with the solution we have on offer in the PR. I have some minor comments on it.

Comment thread packages/fiori/src/IllustratedMessage.ts Outdated
Comment thread packages/fiori/src/IllustratedMessage.ts Outdated
Comment thread packages/fiori/src/IllustratedMessage.ts Outdated
@kineticjs
kineticjs temporarily deployed to netlify-preview July 28, 2026 10:54 — with GitHub Actions Inactive
@plamenivanov91
plamenivanov91 self-requested a review July 28, 2026 11:09
@kineticjs
kineticjs merged commit 4a9418b into main Jul 28, 2026
13 checks passed
@kineticjs
kineticjs deleted the imIssues branch July 28, 2026 11:12
@kineticjs
kineticjs temporarily deployed to netlify-preview July 28, 2026 11:12 — with GitHub Actions Inactive
@sap-ui5-webcomponents-release

Copy link
Copy Markdown

🧹 Preview deployment cleaned up: https://pr-13795--ui5-webcomponents.netlify.app

@sap-ui5-webcomponents-release

Copy link
Copy Markdown

🎉 This PR is included in version v2.25.0 🎉

The release is available on v2.25.0

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IllustratedMessage: component does not resize correctly when the height is being changed

3 participants