Skip to content

fix(desktop): make lightbox zoom controls interactive - #6710

Open
kalvinnchau wants to merge 2 commits into
mainfrom
sanic/fix-image-zoom-controls
Open

fix(desktop): make lightbox zoom controls interactive#6710
kalvinnchau wants to merge 2 commits into
mainfrom
sanic/fix-image-zoom-controls

Conversation

@kalvinnchau

@kalvinnchau kalvinnchau commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace the decorative lightbox zoom icons with accessible Zoom out and Zoom in buttons.
  • Route button clicks through the existing 1x–3x, 5% stepped zoom state without dismissing the viewer, including clicks on SVG icon descendants.
  • Extract the zoom toolbar so markdown.tsx remains below the repository file-size threshold.

Testing

  • pnpm build:e2e
  • pnpm exec playwright test image-attachment-gallery.spec.ts --project=smoke — 10 passed
  • pnpm typecheck
  • Full desktop unit suite — 5,397 passed
  • Pre-push checks — file-size gate, desktop checks, typecheck, and desktop tests passed
  • git diff --check

UI evidence

The rendered lightbox flow is covered by the gallery smoke test: Zoom out is disabled at 100%, Zoom in changes the value to 105%, Zoom out returns it to 100%, and the dialog remains open throughout. Existing gallery, keyboard, spoiler, context-menu, and close-path assertions also pass.

@kalvinnchau
kalvinnchau requested a review from a team as a code owner August 24, 2026 18:53
@kalvinnchau
kalvinnchau force-pushed the sanic/fix-image-zoom-controls branch from ac47987 to 57f76e6 Compare August 24, 2026 19:46
Replace decorative lightbox zoom icons with accessible buttons wired to the existing clamped zoom state. Keep controls from closing the dialog, including SVG icon targets, and cover the regression in the gallery E2E spec.

Signed-off-by: sanic <c57bf9b4275088b2b33db7f746975407210f159fbd8bd733c0e532375f69aa80@buzz.block.builderlab.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>
Signed-off-by: Kalvin Chau <kalvin@block.xyz>

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Combined review — two independent source reviews plus live headless E2E, all at head 57f76e6. No blocking findings.

MINOR — repeated 5% steps miss the displayed endpoint by one click due to floating-point accumulation. The button handlers add/subtract IMAGE_LIGHTBOX_ZOOM_STEP and rely on updateZoomclampImageLightboxZoom, which clamps but never rounds. 40 Zoom in clicks from 1 yield 2.9999999999999973: the label renders 300% while disabled={zoom >= IMAGE_LIGHTBOX_MAX_ZOOM} stays false until click 41; the mirror case holds for Zoom out from an exact 3. Fix by snapping stepped values to the 5% grid before clamping (integer-percent math is simplest); that also lets the exact-endpoint disabled state be asserted after exactly 40 steps in the spec.

Everything else checked out under live E2E: buttons step exactly 5% without dismissing the dialog, direct clicks on nested SVG descendants work (the Element widening in the capture guards is the right call — SVG targets are not HTMLElement), keyboard Enter/ArrowRight behave, 100-click runs clamp and disable at both bounds, and tab order skips the disabled button. The extraction preserves slider behavior mechanically and keeps markdown.tsx under the size gate.

Cross-PR: textually conflicts with #6705 in desktop/src/shared/ui/markdown.tsx; whichever lands second must keep both ImageLightboxZoomControls and #6705's ImageGalleryStatus/openThreadImageGallery, then rerun the gallery smoke row.

Scale the stored zoom offset by the zoom ratio in imageLightboxZoomStateAtZoom so the image point at the frame center stays anchored when the slider or wheel changes zoom, instead of leaving a fixed pixel offset that drifted and exposed empty frame gutter.

Co-authored-by: Kalvin Chau <kalvin@block.xyz>
Signed-off-by: Kalvin Chau <kalvin@block.xyz>
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.

2 participants