fix: replace GeometryReader height cap with HeightCapLayout to eliminate flicker#24595
fix: replace GeometryReader height cap with HeightCapLayout to eliminate flicker#24595devin-ai-integration[bot] wants to merge 3 commits into
Conversation
The > operator causes an infinite feedback loop for images exactly 300pt tall: GeometryReader reports 300 → 300 > 300 is false → frame(height: nil) → image expands to natural height → GeometryReader reports >300 → frame(height: 300) → GeometryReader reports 300 → loop repeats. Using >= stabilizes the loop: once capped at 300, 300 >= 300 stays true, so the frame stays at 300 and GeometryReader reports no change. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Pre-existing accessibility gap: onTapGesture without VoiceOver traits
The .onTapGesture at lines 87-93 opens the image URL in the browser but lacks .accessibilityAddTraits(.isButton), .accessibilityLabel(), and .accessibilityAction {} required by the project's accessibility rules in clients/AGENTS.md. This is a pre-existing issue not introduced by this PR (the tap gesture code is unchanged), but worth noting since the view body was restructured. A follow-up could add proper accessibility traits so VoiceOver users can activate the image link.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed this is a valid accessibility gap, but as noted it's entirely pre-existing and unrelated to this one-character fix. Worth tracking for a future accessibility pass across inline media views.
There was a problem hiding this comment.
Agreed — this is pre-existing and out of scope for this PR. The .onTapGesture should have .accessibilityAddTraits(.isButton) and an .accessibilityLabel for VoiceOver. Good candidate for a follow-up accessibility pass.
…ate flicker The GeometryReader + PreferenceKey pattern requires a state round-trip: first render has imageIntrinsicHeight=0, so the image renders at full natural height, then GeometryReader measures and triggers a second render with the cap applied — visible as a single-frame flicker. HeightCapLayout uses the Layout protocol to measure the child once, cap the reported height, and place within capped bounds — all in a single layout pass. No @State, no PreferenceKey, no flicker. This also removes the >= vs > oscillation bug entirely since there is no feedback loop to oscillate. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
devin-ai-integration
Bot
changed the title
When HeightCapLayout caps a tall image, the width from the uncapped measurement doesn't account for aspect ratio at the capped height. A resizable image with .fit needs to narrow when height is reduced. Now sizeThatFits re-measures the child with the capped height as the proposal, so the child can report the correct width for its aspect ratio in the same layout pass — no horizontal settling. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
1 participant
Summary
Replaces the GeometryReader + PreferenceKey height-capping pattern in
InlineImageEmbedView(introduced in #24589) with a customHeightCapLayoutusing the Layout protocol.The previous approach required a
@Stateround-trip: first render starts withimageIntrinsicHeight = 0, so the image renders at full natural height, then GeometryReader measures and triggers a second render with the cap — visible as a single-frame vertical flicker for tall images. It also had an oscillation bug (>vs>=boundary).HeightCapLayoutresolves both issues in a single layout pass:sizeThatFits.fitnarrows to preserve aspect ratio)No
@State, noPreferenceKey, no vertical flicker, no horizontal settling, no oscillation.This follows the same pattern as
WidthCapLayoutinInlineSurfaceRouter.swift(also from #24589).Review & Testing Checklist for Human
Notes
HeightCapLayoutisprivatetoInlineImageEmbedView.swift. The two-pass measurement insizeThatFitsis needed because when height is capped, the width from the uncapped measurement doesn't account for aspect ratio at the new height. Re-measuring with the capped height lets the child (.aspectRatio(contentMode: .fit)) report the correct narrower width..clipped()is still applied after the layout as a safety net, though the image should already be sized correctly by the proposal.>(not>=): images exactly at 300pt don't need capping, and since there's no state feedback loop, there's no oscillation risk.Link to Devin session: https://app.devin.ai/sessions/bca2b6aae31f43afb41df0dae571ae1d
Requested by: @Jasonnnz