Repository navigation
fix(desktop): use object-scale-down for inline markdown images - #4686
Conversation
Fixes block#4384. Switches object-contain to object-scale-down in ProgressiveImage.tsx's shared IMAGE_CLASS so small dim-less badges don't upscale to fill the fallback reserve box on first view. Both classes scale down to fit; only object-scale-down also refuses to scale up. Verified: - pnpm check (biome + file-sizes + px-text + pubkey-truncation) — clean - npx tsc --noEmit — clean Option 1 from the issue (collapse the frame to the natural size on decode) is a follow-up that requires maintainer sign-off on whether a shrink-only layout shift is acceptable; the issue itself says "Happy to open a PR for option 1 if the direction is acceptable". Signed-off-by: kushaim <carlossilvajimenez@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Reviewed at b6b8728e23a365d01e283d4b63608052ce3c5b84 on Wes's behalf.
No actionable findings. This is the intended minimal form of #4384's option 2: object-scale-down retains contain-style downscaling for content larger than its frame while preventing small dim-less images from being enlarged inside the frozen 384×256 first-view reserve. The image element, trigger hit area, reserve geometry, lightbox source box, thumbnail/full-image sequencing, and mosaic object-cover override remain unchanged.
Residual, non-blocking tradeoffs:
- A first-view small dim-less image remains centered in a large blank/clickable reserve; a later mount can use cached intrinsic dimensions. The PR accurately discloses that layout-preserving tradeoff.
- There is no direct first-view tiny-badge regression assertion.
desktop/src/shared/ui/markdown/utils.tsstill says the caller letterboxes viaobject-contain; that comment is now stale, but does not affect behavior.
I did not duplicate the repository's CI-equivalent suites locally. CI was still running when this review was submitted, with no failures in the completed checks I inspected. This comment is a technical review, not an approval; Wes retains approval authority.
…#4686) Fixes block#4384. A dim-less markdown image with no cached decode gets `DEFAULT_IMAGE_RESERVE = { height: 256, width: 384 }` and `useFixedReserveBox: true` in `resolveImageReserveBox`. The frame is that big on first view, and the inline `<img>` uses `object-contain` which scales UP to fill the frame — so a 46×20 shields badge renders as a giant blurry block. The same message renders at the natural size on the second view because `rememberDecodedImageDimensions` populates the cache, but the inconsistency between first and subsequent view is the bug. Switches `object-contain` → `object-scale-down` on `ProgressiveImage.tsx`'s shared `IMAGE_CLASS`. Both `object-contain` and `object-scale-down` scale down to fit, but only `object-scale-down` also refuses to scale up — a small image now sits centered in the reserve box on first view, never upscaled. The layout-shift guarantee for large images is preserved (they still scale down to fit the reserve), matching the issue's stated constraint. Chose option 2 ("Alternatively/minimally") from the issue over option 1 because: - It's a one-class-name change with zero new state, zero new effects, and zero risk to the existing layout-shift guarantee that the surrounding `useFrozenImageReserve` is explicitly designed to preserve. - Option 1 (collapse the frame on decode) is the issue author's preferred direction but requires deciding whether the layout shift is acceptable *shrink*-only — a direction question that should be confirmed with the maintainer first. The issue explicitly says "Happy to open a PR for option 1 if the direction is acceptable" — that's the sign-off step, not this PR. If maintainers want the full first-view-equals-second-view behavior, option 1 is a follow-up that builds on this base. Not added: a regression test. The current desktop test infra (`desktop/src/shared/ui/markdown/markdown.test.mjs` and siblings) doesn't cover `ProgressiveImage` directly; a DOM-style snapshot would need a new test harness. The change is one CSS class name; the visual regression is plain to eyeball in the screenshots CI already captures. Verified: - `pnpm check` (biome + file-sizes + px-text + pubkey-truncation) — clean - `npx tsc --noEmit` — clean Signed-off-by: kushaim <carlossilvajimenez@gmail.com> Co-authored-by: kushaim <carlossilvajimenez@gmail.com>
Fixes #4384.
A dim-less markdown image with no cached decode gets
DEFAULT_IMAGE_RESERVE = { height: 256, width: 384 }anduseFixedReserveBox: trueinresolveImageReserveBox. The frame isthat big on first view, and the inline
<img>usesobject-containwhich scales UP to fill the frame — so a 46×20 shields badge renders
as a giant blurry block. The same message renders at the natural
size on the second view because
rememberDecodedImageDimensionspopulates the cache, but the inconsistency between first and
subsequent view is the bug.
Switches
object-contain→object-scale-downonProgressiveImage.tsx's sharedIMAGE_CLASS. Bothobject-containand
object-scale-downscale down to fit, but onlyobject-scale-downalso refuses to scale up — a small image now sitscentered in the reserve box on first view, never upscaled. The
layout-shift guarantee for large images is preserved (they still
scale down to fit the reserve), matching the issue's stated
constraint.
Chose option 2 ("Alternatively/minimally") from the issue over
option 1 because:
effects, and zero risk to the existing layout-shift guarantee
that the surrounding
useFrozenImageReserveis explicitlydesigned to preserve.
preferred direction but requires deciding whether the layout
shift is acceptable shrink-only — a direction question that
should be confirmed with the maintainer first. The issue
explicitly says "Happy to open a PR for option 1 if the
direction is acceptable" — that's the sign-off step, not
this PR.
If maintainers want the full first-view-equals-second-view
behavior, option 1 is a follow-up that builds on this base.
Not added: a regression test. The current desktop test infra
(
desktop/src/shared/ui/markdown/markdown.test.mjsandsiblings) doesn't cover
ProgressiveImagedirectly; aDOM-style snapshot would need a new test harness. The change
is one CSS class name; the visual regression is plain to
eyeball in the screenshots CI already captures.
Verified:
pnpm check(biome + file-sizes + px-text + pubkey-truncation) — cleannpx tsc --noEmit— clean