Skip to content

fix(detail): pin slide image to fixed-height box to stop per-switch resize flicker#534

Closed
Zheaoli wants to merge 4 commits into
mainfrom
fix/detail-img-pin-height
Closed

fix(detail): pin slide image to fixed-height box to stop per-switch resize flicker#534
Zheaoli wants to merge 4 commits into
mainfrom
fix/detail-img-pin-height

Conversation

@Zheaoli

@Zheaoli Zheaoli commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Problem

After #533 pinned the detail-view image-area container to a fixed sm:h-[90vh] box, switching photos still showed a visible jump/flicker. Live DevTools profiling on a prod build pinpointed the cause as the <img> element resizing per aspect ratio inside the fixed container.

Fix (two commits)

1. Pin the image to sm:h-full (was object-contain md:max-h-[90vh]): the image element should equal the container height and letterbox the photo via object-contain, instead of sizing to the photo's contained height (which differs per aspect → resize).

2. Give the ProgressiveImage root wrapper a height (<div className="relative">relative sm:h-full): height:100% only resolves when every ancestor up to the fixed-height slide has a definite height. The wrapper between the slide and the <img> had none, so it collapsed to content height and the img's h-full fell back to the image's own contained height (~607px landscape, ~1366px portrait — and overflowing, since commit 1 removed the max-h cap). This was the residual "frantic flicker". With the wrapper height set, the chain is continuous (container → viewport → slide → wrapper → img) and the img is a constant 90vh for every aspect.

Also shortened the preview fade-in 1s → 150ms.

Verification

  • Walked the live height chain: the break was at <div className="relative"> (607px, content height). Injecting height:100% on it made the slide img go 607 → 810px (= container) for all visible slides — confirming fix v0.5.0 #2 before shipping.
  • Container measured constant 810px; img is what varied by aspect.
  • data-flip-target is on the slide div (unchanged box); the open transition's containedBox(box, aspect) letterboxes within it, so the FLIP landing is unaffected.
  • Mobile (<sm) unchanged: no fixed height, content-sized.
  • Build confirmed prod (pnpm build / NODE_ENV=production / node server.js), so the flicker was real, not a dev-mode artifact.

On the src re-set (deferred, likely benign)

Profiling showed each visible slide's <img src> is re-set to the same value ~11×/switch (carousel re-render). Tested live: this fires a load event but keeps img.complete === true and the natural dimensions throughout, i.e. Chrome retains the decoded pixels with no visible blank. So it does not appear to cause a visible flash. If any residual flash remains after this height fix, the next step (separate PR) is memoizing the slides so a switch doesn't re-render unchanged images — kept out of this PR because it touches the keep-mounted WebGL zoom lifecycle and isn't justified by a confirmed visible flash.


Update: third commit — letterbox the blur placeholder (0ed3ca6)

Once the image is pinned to a constant 90vh box, the blur placeholder also needs to letterbox. next/image derives the blur's background-size from style.objectFit (get-img-props.js: backgroundSize = imgStyle.objectFit), not the className. With only className="object-contain", objectFit was undefined → the blur defaulted to cover and filled the whole 90vh box while the real image letterboxed smaller → a visible "blur fills → image shrinks" jump on load (worst for portraits). Confirmed live (blur background-size: cover) and against Next's source.

Fix: style={{ objectFit: 'contain' }} on the preview image → blur letterboxes the same way → smooth sharpen+fade load, no size jump.

Branch HEAD 0ed3ca6 is the complete fix: ① img sm:h-full ② wrapper sm:h-full ③ blur objectFit: 'contain'. Deploy must build this exact HEAD (an earlier deploy shipped only commit 1 → img grew unbounded to ~1366px and overflowed → worse than #533; that was a stale build, not a code error).


Update: fourth commit — keep tone analysis on switch (4901079)

With the image fully stabilized (commits 1–3 verified live: img constant 810, blur letterboxed), the remaining flicker was the info panel, isolated to ToneAnalysis: it returned a spinner unconditionally while loading (if (loading) return <spinner>), so every switch collapsed the whole tone section to a tiny spinner and re-expanded it, shifting the histogram/device rows below (up then back) = "info panel flashes/reloads on switch".

#532 added the loading && !histogram guard to the histogram chart but not to tone analysis, so the histogram stopped flickering while this kept doing it. Fix: same guard — if (loading && !toneData) — only show the spinner on first analysis; keep previous tone values during a switch (the #521 LRU cache returns instantly for revisits). Confirmed via live churn capture (the animate-spin text-sm text-muted-foreground spinner = tone-analysis:223) + three-way review.

Branch HEAD 4901079 is the full detail-flicker fix: ① img sm:h-full ② wrapper sm:h-full ③ blur objectFit: contain ④ tone loading && !toneData guard.

…ch resize flicker

#533 pinned the outer image-area container to a fixed 90vh box, but the
<img> element inside still used `object-contain md:max-h-[90vh]`, so the
element sized itself to each photo's contained height — ~90vh for a portrait
but much shorter for a landscape. The element therefore resized (~810<->607px)
on every aspect-changing switch and the photo visibly jumped/flickered inside
the otherwise-stable container.

Pin the image to `object-contain w-full sm:h-full` so the element always
equals the container height and object-contain letterboxes within it -> no
per-switch resize. Align the breakpoint with the container's `sm:` (#533);
mobile stays content-sized. Also shorten the preview fade-in from 1s to 150ms.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 11, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
picimpact Ready Ready Preview, Comment Jun 12, 2026 1:30pm

…l resolves

The previous commit pinned the <img> to `sm:h-full` so it stays a constant
height and letterboxes via object-contain, but height:100% only resolves when
every ancestor up to the fixed-height slide has a definite height. The
ProgressiveImage root `<div className="relative">` sits between the slide and
the <img> and had no height, so it collapsed to content height and the img's
h-full fell back to the image's own contained height — ~607px for a landscape,
~1366px for a portrait — i.e. it still resized (and now overflowed, since the
max-h cap was removed) on every aspect-changing switch. That was the residual
"frantic flicker".

Add `sm:h-full` to the wrapper so the chain is continuous
(container sm:h-[90vh] -> viewport -> slide -> wrapper -> img), making the img
element a constant 90vh for every aspect ratio. Verified live: the slide img
went 607px -> 810px (=container) once the wrapper had a resolved height.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at fixed height

Now that the image is pinned to a constant 90vh box and letterboxes via
object-contain, the blur placeholder must letterbox too. next/image derives the
blur placeholder's background-size from `style.objectFit`, not the className
(get-img-props: `backgroundSize = imgStyle.objectFit`). With only
`className="object-contain"`, objectFit was undefined so the blur defaulted to
`cover` and filled the whole 90vh box, while the real image letterboxed smaller
— a visible "blur fills → image shrinks" jump on load completion, worst for
portraits in the now-fixed box.

Set `style={{ objectFit: 'contain' }}` on the preview image so the blur
placeholder uses contain and shares the same letterboxed box as the real image
→ load is a smooth sharpen+fade with no size jump.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oesn't flicker

ToneAnalysis returned a spinner unconditionally while `loading`
(`if (loading) return <spinner>`), so every photo switch collapsed the whole
tone section to a tiny spinner and re-expanded it when the new analysis
finished. That shrink/grow shifted the entire info panel's layout (histogram
and device-info rows below jumped up then back) — the "info panel flashes/
reloads on switch" the user reported.

#532 added the `loading && !histogram` guard to the histogram chart but not to
tone analysis, so the histogram stopped flickering while this one kept doing
it. Apply the same guard here: only show the spinner on the first analysis
(`loading && !toneData`); on a switch keep the previous tone values rendered
until the new ones are computed (the in-session LRU cache from #521 still
returns instantly for revisited photos). No size change → no panel shift.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Zheaoli

Zheaoli commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #536. #535 (merged to main) removed the preview fade animation and touches the same MotionImage block, so this branch (based on pre-#535 main) would conflict. #536 re-applies the still-needed fixes (image height pin, blur letterbox, tone-panel guard) cleanly on top of main+#535, keeping #535's fade removal.

@Zheaoli Zheaoli closed this Jun 12, 2026
pull Bot pushed a commit to candies404/PicImpact that referenced this pull request Jun 12, 2026
…tion

Deploys repeatedly drifted from the branch HEAD (a manual build run before
the latest commit, a stale `:latest` pull, the besscroft#534-vs-besscroft#535 mixup), and the
only way to tell what was actually live was to hand-inspect rendered DOM
classes. Make the deployed commit verifiable at a glance.

- build-main: pass the short commit SHA as a `BUILD_SHA` build-arg and also
  tag the pushed image `:git-<sha>` alongside `:latest`, so the image is
  identifiable by commit.
- Dockerfile: accept `BUILD_SHA` (default `unknown`) and set it as a runtime
  env in the runner stage.
- Add GET /api/public/version returning `{ sha }` (the build's commit) so a
  running deploy can be checked with `curl /api/public/version` — it should
  report the commit you expect — instead of guessing whether `:latest` is the
  branch HEAD.

Local/dev builds without the arg report "unknown". No app behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant