From 1f25cec94948d190fd41d6e5fe843985333a7d41 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Tue, 12 May 2026 15:48:25 +0000 Subject: [PATCH] android, desktop: constrain image height in layout (#6959) * android, desktop: constrain image height in layout Clamp the image preview Box's aspect ratio so it never goes below 1/2.33 (matching the height cap already enforced by PriorityLayout). Prevents tall image previews from overflowing into the caption area below. Analogous to iOS #6732. * plans: justify image text overlap fix --- .../common/views/chat/item/CIImageView.kt | 2 +- plans/2026-05-09-fix-image-text-overlap.md | 83 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 plans/2026-05-09-fix-image-text-overlap.md diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/item/CIImageView.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/item/CIImageView.kt index 064b5370bc..8bfbea9fa6 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/item/CIImageView.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/item/CIImageView.kt @@ -182,7 +182,7 @@ fun CIImageView( .then( if (!smallView) { val w = if (previewBitmap.width * 0.97 <= previewBitmap.height) imageViewFullWidth() * 0.75f else DEFAULT_MAX_IMAGE_WIDTH - Modifier.width(w).aspectRatio(previewBitmap.width.toFloat() / previewBitmap.height.toFloat()) + Modifier.width(w).aspectRatio((previewBitmap.width.toFloat() / previewBitmap.height.toFloat()).coerceAtLeast(1f / 2.33f)) } else Modifier ) .desktopModifyBlurredState(!smallView, blurred, showMenu), diff --git a/plans/2026-05-09-fix-image-text-overlap.md b/plans/2026-05-09-fix-image-text-overlap.md new file mode 100644 index 0000000000..f759a454e0 --- /dev/null +++ b/plans/2026-05-09-fix-image-text-overlap.md @@ -0,0 +1,83 @@ +# Fix tall image preview overlapping caption text + +Branch: `nd/fix-image-text-overlap` · commit `0a3dcd249` · analogous to iOS PR [#6732](https://github.com/simplex-chat/simplex-chat/pull/6732). + +## 1. Problem statement + +For a tall image (height/width ≳ 2.33), the chat-bubble caption text was rendered on top of the bottom of the image instead of cleanly below it. The image looked like it had a semi-transparent text watermark across its lower section. Reproduced on Android and desktop with any sufficiently tall image; never reproduced on images with `height ≤ 2.33 × width`. + +iOS already had the analogous fix (PR #6732, commit `b24d003a8`); Android/desktop did not. + +## 2. Solution summary + +One line in `apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/item/CIImageView.kt`. The image preview Box's `Modifier.aspectRatio(width / height)` is wrapped with `.coerceAtLeast(1f / 2.33f)` so the aspect ratio cannot go below the floor at which the layout starts to break. + +```diff +- Modifier.width(w).aspectRatio(previewBitmap.width.toFloat() / previewBitmap.height.toFloat()) ++ Modifier.width(w).aspectRatio((previewBitmap.width.toFloat() / previewBitmap.height.toFloat()).coerceAtLeast(1f / 2.33f)) +``` + +Total diff: 1 file, +1 / −1. + +## 3. Root cause + +`PriorityLayout` (`FramedItemView.kt:480`, added in #6726) caps image-region height at `constraints.maxWidth × 2.33f` and passes that as `maxHeight` to its image child: + +```kotlin +val maxImageHeight = (constraints.maxWidth * 2.33f).toInt().coerceAtMost(constraints.maxHeight) +val imageConstraints = constraints.copy(maxHeight = maxImageHeight) +val imagePlaceable = … .measure(imageConstraints) +``` + +The image child is `CIImageView`'s outer Box, modified (line 185 pre-fix) by `Modifier.width(w).aspectRatio(previewBitmap.width / previewBitmap.height)`. Compose's `aspectRatio` modifier picks a layout size satisfying both the parent's constraints AND the requested ratio — by trying `tryMaxWidth`, `tryMaxHeight`, `tryMinWidth`, `tryMinHeight` in order and returning the first satisfying `IntSize`. + +When the natural ratio is below `1/2.33`, every candidate violates one bound: + +- `tryMaxWidth`: implied `height = W / ratio > W × 2.33 = parentMaxHeight` → fails the height cap. +- `tryMaxHeight`: implied `width = parentMaxHeight × ratio < W` → fails the fixed `.width(W)` lower bound. +- `tryMinWidth` / `tryMinHeight`: same failures. + +`findSize()` returns `IntSize.Zero`, and `aspectRatio` falls through to passing the parent's UNBOUNDED-height-shape constraints down (`wrappedConstraints = constraints` — see Compose Foundation's `AspectRatioNode`). The inner `Image` (`.width(W).contentScale=FillWidth`) then sizes itself by intrinsic aspect, with `height = W × imgH/imgW`, drawing past the layout box `clipToBounds` would have given it. The text below then renders on the same vertical strip as the painter's overflow — visible as overlap. + +`PriorityLayout`'s height cap (added in #6726) prevents the original crash but does not, on its own, prevent this visual overflow, because the cap propagates through `imageConstraints` to a modifier that silently drops it. + +## 4. The fix in detail + +`coerceAtLeast(1f / 2.33f)` raises the ratio's floor to exactly the value where Compose's `tryMaxWidth` succeeds: + +- At `ratio = 1/2.33`, implied `height = W / (1/2.33) = W × 2.33 ≤ parentMaxHeight` (since `W ≤ parentMaxWidth` and `parentMaxHeight = parentMaxWidth × 2.33`). +- `findSize` returns `(W, W × 2.33)`. `wrappedConstraints` becomes `Constraints.fixed(W, W × 2.33)` — both dimensions fixed. +- Inner `Image`'s `paint` modifier sees `hasFixedDimens=true` and returns the constraints unchanged. Image bounds = `(W, W × 2.33)`. +- `Image`'s built-in `clipToBounds` clips the painter's `FillWidth` overflow to the bounded layout. `PriorityLayout` reads `imagePlaceable.measuredHeight = W × 2.33` and places the caption text immediately below. + +For images at or above the floor (ratio ≥ 1/2.33), `coerceAtLeast` is a no-op — the ratio is unchanged, the layout is unchanged, no behavioural diff. The change only takes effect for images that triggered the bug. + +`coerceAtLeast` (not `maxOf`) matches the idiom already used at `FramedItemView.kt:480` (`.coerceAtMost(constraints.maxHeight)`) — both clamp at the bound where the layout starts to break. Reads as "ensure the ratio is at least 1/2.33". + +## 5. Why this specific shape + +- **Why preserve the existing expression untouched.** The pre-fix `previewBitmap.width.toFloat() / previewBitmap.height.toFloat()` is the aspect-ratio computation. Wrapping it `(...).coerceAtLeast(1f / 2.33f)` makes the diff purely additive — a reviewer reading the patch sees "the existing ratio is now floored", with zero risk that the inner expression silently changed. Diff is one line, character-minimal. + +- **Why no `MAX_IMAGE_HEIGHT_RATIO` constant.** Two use sites of `2.33f` after the fix (`PriorityLayout` line 480 and the new clamp). `good-code-v5.md`: *"Three similar lines are better than a premature abstraction."* If a third site appears (link preview, video preview, etc.) the constant earns its place. Until then, local duplication mirrors the convention already in `PriorityLayout`. + +- **Why no edit to `CIVideoView`.** The screenshot showed only the image-preview bug. iOS PR #6732 also touched `CIVideoView` and `CILinkView`, but on Android/desktop the video preview's outer Box has no `aspectRatio` modifier at all — it is sized by its inner `VideoPreviewImageView`'s `.width(width).FillWidth`, which is paint-clamped by `imageConstraints.maxHeight` and reports a correct layout size. If a tall-video overlap is actually reported in the wild, the fix is a separate commit; speculatively replicating the iOS scope would expand the diff and review surface beyond what the bug requires. + +- **Why no `fillMaxSize + ContentScale.Crop` rewrite of the inner `Image`.** A previous iteration of this fix did that to mirror iOS's `scaledToFill` change. It works, but is structurally redundant: once the outer Box's `aspectRatio` is bounded, the inner `Image`'s existing `paint` modifier (`hasFixedDimens=true → return constraints`) and `clipToBounds` already produce the same visual result. The rewrite was extra structure, not bug-fix; reverted. + +- **Why no `PriorityLayout` constant rename.** `2.33f` is already inline at line 480 and works as-is. Extracting it to `MAX_IMAGE_HEIGHT_RATIO` would be a rename bundled with a bugfix — `good-code-v5.md`: *"a rename in a diff signals a meaningful change to the reviewer — a gratuitous rename wastes reviewer attention and can mask real changes."* Out of scope. + +- **Why `coerceAtLeast(1f / 2.33f)` and not `coerceAtLeast(0.43f)`.** The form `1f / 2.33f` makes the relationship to `PriorityLayout`'s `2.33f` height multiplier visually explicit. `0.43f` would be opaque and would drift independently if either value changed. + +## 6. Verification + +Manual sanity (Android debug APK): + +- Send a tall screenshot (height ≫ width) with a caption → caption now sits below the cropped image preview, no overlap. +- Send an image where `height ≤ 2.33 × width` → preview unchanged from pre-fix (the clamp is a no-op for these). +- Tap the cropped preview → fullscreen viewer opens the full image at native aspect (the clamp only affects the inline preview, not the viewer). + +## 7. Risk and rollback + +- **Blast radius** is the single-Box modifier in `CIImageView` for non-`smallView` mode. `smallView` (used by `ChatPreviewView` thumbnails) takes the `else Modifier` branch and is untouched. +- The clamp is a no-op for images that did not trigger the bug, so regression risk on non-tall images is zero by construction. +- Rollback: `git revert 0a3dcd249` and force-push the branch (or just drop the commit before merge).