Observe an animated reveal when deciding the viewport moved - #207
Conversation
…workspace A window moved to an adjacent workspace can land selected in a column outside the destination viewport: parked offscreen, given no frame write, yet holding keyboard focus. These tests record the viewport state observed in that reproduction and the reveal behavior around it. The fixture is the destination geometry from the reproduction: four 1150pt columns with a 20pt gap at x = 0, 1170, 2340, 3510, seen through a 2560pt viewport, with the moved window landing in the last column. In that state the rebase step of ensureSelectionVisible leaves viewOffsetPixels at -3530 with viewStart at -20, so column 3 starts roughly 970pt beyond the right viewport edge. Five tests assert the reveal contract under disabled motion: the rebase is view-neutral, the moved column is parked, a parked destination column is revealed, a scroll is scheduled, and scroll lock does not exempt a fully parked target. Three further tests separate the two motion configurations, because they behave differently in a way that matters for the surrounding code. With motion disabled, animateToOffset takes its static fallback and both current and target land on the snap. With motion enabled — the value MotionPolicy.snapshot() returns, so the one the runtime move path uses — it installs a spring: the target moves while the current offset stays put for the display-link driver to interpolate. The plan-build step in NiriLayoutHandler tests abs(viewOffsetPixels.current() - offsetBefore) > 1 to decide whether to emit a .startNiriScroll directive, so it observes the former but not the latter. These are diagnostic rather than a regression guard for a landed fix. The runtime defect is timing-dependent and does not reproduce under trace capture.
A window moved to another workspace could stay parked off the destination viewport while still holding keyboard focus, so the user saw a different window than the one they moved. ensureSelectionVisible rebases the viewport offset so retargeting activeColumnIndex does not shift what is on screen, then calls scrollToReveal to bring the newly selected column into view. The rebase is view-neutral by design, so that reveal is the only step that can move the viewport onto the moved window's column. How it moves depends on the motion snapshot. With animations disabled animateToOffset writes a static offset and the current offset changes immediately. With animations enabled — the value MotionPolicy.snapshot() returns, so the one this path uses — it installs a spring: the target moves and the current offset stays put for the display-link driver to interpolate. The plan-build step decided whether the viewport had moved by comparing only the current offset against its pre-reveal value. For an animated reveal that comparison is false, so viewportNeedsRecalc stayed unset and the .startNiriScroll directive gated on it was never emitted. Nothing called startScrollAnimation, nothing registered in scrollAnimationByDisplay, and nothing ever drove the spring — leaving the target pointing at the revealed column while the viewport sat where it started. Compare both the current and target offsets against their pre-reveal values, via one shared helper used by both gate sites so they cannot drift apart. A static reveal is still observed through the current-offset delta; an animated one is now observed through the target delta. This is a candidate fix: the mismatch it removes is measured by tests, but the runtime defect is timing-dependent and has not yet been confirmed fixed in a real reproduction.
|
Warning Review limit reached
Next review available in: 56 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Context after the investigation that produced this PR concluded. This does not fix the moved-window visibility bug it was written for. That bug is fixed by #209: the AX focus-confirmation path was skipping the reveal entirely on a token-only re-confirmation test. Since the reveal never ran, there was no scheduled scroll for the gate fixed here to miss — this change is downstream of the actual cause. The mismatch it removes is real and measured.
But I have no runtime evidence of the gate's blindness costing anything in practice — every observed instance reached Marking ready so the reasoning is reviewable. Fine to close if the maintainers would rather not carry a change whose effect has never been observed. |
|
Note on the commit list: this PR shows two commits because it builds on #204 — it modifies the test file #204 adds ( The commit belonging to this PR is "Observe an animated reveal when deciding the viewport moved"; the diagnostic-tests commit belongs to #204. If #204 is closed rather than merged, this PR needs rebasing to carry the test-file changes itself. |
|
Merge order: #204 before this PR. This PR modifies the test file #204 adds ( If #204 is closed rather than merged, this PR needs rebasing to carry those test-file changes itself before it can merge. The other PRs from this investigation are independent and can merge in any order: #209 (the actual fix), #202, #203, #206. |
1 participant
Symptom
A window moved to another workspace could stay parked off the destination viewport while still holding keyboard focus — so the user saw a different window than the one they moved, and keystrokes went somewhere invisible.
Mechanism
ensureSelectionVisiblerebases the viewport offset so retargetingactiveColumnIndexdoes not shift what is on screen, then callsscrollToRevealto bring the newly selected column into view. The rebase is view-neutral by design, so that reveal is the only step that can move the viewport onto the moved window's column.How it moves depends on the motion snapshot:
animateToOffsetbehaviorMotionPolicy.snapshot()returns.enabledunconditionally, so the runtime move path always takes the spring branch.The plan-build step decided whether the viewport had moved by comparing only the current offset against its pre-reveal value:
For an animated reveal that comparison is false. So
viewportNeedsRecalcstayed unset, the.startNiriScrolldirective gated on it was never emitted,startScrollAnimationwas never called, nothing registered inscrollAnimationByDisplay, and nothing ever drove the spring — leaving the target pointing at the revealed column while the viewport sat where it started. The layout pass then correctly parked the window offscreen and wrote no frame for it, while the focus handoff still granted it focus.Change
Compare both the current and target offsets against their pre-reveal values, through one shared helper used by both gate sites so they cannot drift apart. A static reveal is still observed through the current-offset delta; an animated one is now observed through the target delta.
Evidence
Both motion configurations are pinned by tests on the geometry from the reproduction (four 1150pt columns, 20pt gap, 2560pt viewport, moved window landing in the last column):
revealWithMotionEnabledSchedulesAnimationWithoutMovingCurrentOffset— the spring path moves the target, leaves the current offset, and reportsisAnimatingrevealWithMotionDisabledLandsStaticallyOnTheSnap— the static path lands both on the snapanimatedRevealIsObservableFromTheOffsetDeltas/staticRevealIsObservableFromTheOffsetDeltas— the combined check observes bothSix earlier candidate mechanisms for this symptom were investigated and eliminated; they are recorded in the discovery document in #206.
Status
Candidate fix, not confirmed. The mismatch it removes is measured by tests, but the runtime defect is timing-dependent — it does not reproduce under runtime trace capture — so it has not been confirmed fixed in a real reproduction. Draft until it has been.
Note this supersedes #203 as the candidate fix for this symptom. Instrumentation showed #203's z-order raise never executes in this scenario; that PR closes a real but separate gap.
Greptile Summary
The PR broadens viewport-movement detection to observe both current and animated target offsets, allowing animated reveals to schedule the scroll driver after moving a window between workspaces.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete changed-code failure established.
The shared predicate preserves static reveal detection while adding the target-offset delta required to observe spring-based reveals, and no accepted blocking or non-blocking defect remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Move as Window transfer participant Reveal as ensureSelectionVisible participant Offset as View offset participant Plan as Layout plan participant Driver as Display-link driver Move->>Reveal: Select moved window's column Reveal->>Offset: Rebase current and target Reveal->>Offset: Animate target toward visible snap Offset-->>Plan: Current unchanged, target changed Plan->>Plan: viewportMoved() observes target delta Plan->>Driver: Emit startNiriScroll Driver->>Offset: Interpolate spring to targetReviews (1): Last reviewed commit: "Observe an animated reveal when deciding..." | Re-trigger Greptile