[codex] Fix PhotosCrop zoom bounce canvas tracking#309
Merged
Conversation
3 tasks
muukii
added a commit
that referenced
this pull request
Jun 17, 2026
## Summary Fixes a regression where, after changing the zoom in **Crop** mode and switching to **Blur** (tool/mask) mode, the crop output rendered **small and pinned to the top-left**. Toggling the Crop/Blur tabs again corrected it; zooming the other way produced the inverse misplacement. ## Root cause `makeToolCropDisplayViewport` derives the Metal canvas placement from **presentation-layer** conversions (`currentLayerRect(... usesPresentationLayers:)`), gated only on `toolSurface.isInteractiveZoomGestureActive == false`. This was introduced in #309 to make the mask canvas follow the post-release zoom bounce. The Crop→Blur switch applies the tool viewport **once, synchronously**, in the same runloop turn that `updateToolScrollGeometry` just reconfigured the freshly-un-hidden tool scroll view (non-animated `setZoomScale`, `centerContentInViewport`, etc.): `setFeatureFocus` → `applySurfaceMode(syncsToolViewportFromCrop: true)` → `updateToolScrollGeometry` (model set synchronously) → `updateToolCropDisplayViewport()` (one-shot, no display link). At that instant no pinch is active, so `usesPresentationLayers == true`, but Core Animation has not yet committed the new geometry to the presentation layers — they still hold the **previous** Blur session's geometry. The one-shot samples that stale transform → small + top-left placement, and because no display link runs afterward, the stale frame sticks until the next mode switch (the "switch back and forth fixes it" symptom). `#308` (GPU-resident source / per-generation content bake), originally suspected, is **not** involved: it never touches `CropView.swift`, and `EditingCanvasContentBake` maps the bake back to the original `extent`, so it changes layer *content*, never viewport *placement*. ## Fix Presentation-layer reads are only meaningful while the viewport display link is actively re-sampling an in-flight bounce. Gate them on whether that link is running: ```swift let usesPresentationLayers = toolSurface.viewportRendering.isRunning && toolSurface.isInteractiveZoomGestureActive == false ``` This flips **only** the link-idle one-shot paths (i.e. the mode switch) back to the model layers, which the synchronous reconfigure already made authoritative. Every display-link-driven path (interactive pinch, post-release bounce, deceleration) is unchanged, so #309's bounce tracking still works. ## Scope / notes - Scoped to the **tool** path only. `makeCropDisplayViewport` has the same presentation-read pattern, but the crop path is not a reported repro and its animator-driven rotation computes the viewport inside a `UIViewPropertyAnimator` block (where presentation reads are intended), so it is intentionally left untouched. - The tool path's animator case early-returns (the tool surface is hidden in Crop mode), so the gate is safe there. ## Test plan - [x] `xcodebuild -scheme BrightroomUI -destination 'platform=iOS Simulator,name=iPhone 17 Pro'` builds. - [x] Verified on device: Crop zoom change → switch to Blur now displays the crop output correctly (no small/top-left misplacement, no need to toggle tabs). - [ ] Editing canvas renders black on the Simulator, so visual confirmation is device-only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
customZoomsnap during bounce-back.Root Cause
The viewport display link was already running, but
makeCropDisplayViewport()disabled presentation-layer reads for the full zoom interaction, including the post-release bounce. During bounce-back UIKit animates the presentation layer while model values are already clamped, so the canvas snapped instead of following the visible image. Tool/Mask mode also computed its viewport from modelcontentOffset/contentInset/zoomScale, which lost the presentation-time centering during min-zoom shrink.Validation
git diff --checkSwiftUIDemolaunched oniPhone 17 Pro (27.0)simulator from the cwd worktree; build completed with no errors. Launch session:77a4d4780.