spec: prevent overshoot scrolling code diff to top (#9808)

[lonexreb]

Spec for #9808. Clamp in-progress scroll gestures to the diff container; only new gestures (after a 200ms settling window) bubble up to the parent blocklist. Applies to all blocklist-embedded scroll containers, not just diff.

Closes (spec-only) #9808.

[oz-for-oss]

@lonexreb

I'm starting a first review of this spec-only pull request.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This spec defines a gesture-window clamp for nested scroll containers to prevent in-progress overscroll from moving the parent blocklist.

Concerns

• The behavior contract does not define ownership after a new gesture starts at an inner-scroll boundary, so implementers could bubble only the first delta and then consume the rest of the same gesture.
• The scope expands from code diff to all blocklist-embedded scroll containers, but acceptance criteria, implementation pointers, and tests remain diff-only.
• The scrollbar click acceptance criterion should identify which scrollbar target bypasses the gesture clamp.

Verdict

Found: 0 critical, 2 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

⚠️ [IMPORTANT] B3 conflicts with B1 for a new gesture that begins while the inner container is already at its boundary: after the first delta bubbles, specify whether the remaining deltas in that same ≤200ms gesture keep bubbling to the parent or are consumed by the child.

[oz-for-oss]

⚠️ [IMPORTANT] B6 expands the requirement to every blocklist-embedded scroll container, but the acceptance criteria, implementation pointers, and tests only cover diffs; either limit the scope to diffs or add explicit coverage for code review snippets and embedded markdown plus the shared hook that will enforce it.

[oz-for-oss]

💡 [SUGGESTION] Clarify whether this means the inner diff scrollbar, the parent blocklist scrollbar, or both, because the bypass behavior depends on which scrollable receives the click.

[lonexreb]

/oz-review

[oz-for-oss]

@lonexreb

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This spec defines gesture ownership for wheel/trackpad scrolling inside embedded code-diff containers, including boundary clamping, the 200ms gesture window, acceptance criteria, and tests.

Concerns

• The proposed V1 scope conflicts with the PR description: the description says the behavior applies to all blocklist-embedded scroll containers, while the spec limits the behavior to code-diff containers and excludes other embedded containers. Resolve the scope before implementation so reviewers know which surfaces must be changed and tested.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

⚠️ [IMPORTANT] The PR description says this applies to all blocklist-embedded scroll containers, but this limits V1 to code-diff containers and excludes other embedded containers; align the scope or explain why the broader surfaces are deferred.

[lonexreb]

Round-1 fixes for oz-for-oss review: (1) added Gesture Ownership at Boundaries subsection — boundary-state at gesture start is sole binding decision point; no first-delta bubble then consume; (2) reconciled V1 scope to ALL blocklist-embedded scrollable containers (diff, tree, table, log) with enumerated component identifiers and acceptance/tests covering ≥2 container types; (3) defined scrollbar bypass precisely via role="scrollbar" and ::-webkit-scrollbar* selectors with explicit click vs wheel handling.

[lonexreb]

/oz-review

[oz-for-oss]

@lonexreb

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This spec defines a gesture-ownership model to keep in-progress wheel/trackpad scrolling inside blocklist-embedded scroll containers until a 200ms idle gap, with scrollbar pointer actions bypassing the clamp.

Concerns

• The V1 scope says all blocklist-embedded scrollable containers are covered, but the behavior and validation only define vertical top/bottom scrolling and representative two-container coverage.
• The scrollbar bypass contract relies on ARIA roles and CSS pseudo-elements, which do not match Warp's custom-rendered scrollbar hit-testing model.
• The implementation pointers promise concrete identifiers but still leave implementers to discover the actual component wrappers.

Verdict

Found: 0 critical, 2 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

⚠️ [IMPORTANT] V1 claims all blocklist-embedded scrollable containers, but the contract and tests only define vertical top/bottom behavior and require two representative container types. Specify horizontal/dual-axis ownership semantics and either require coverage for each in-scope container class or narrow the scope.

[oz-for-oss]

⚠️ [IMPORTANT] B5a defines scrollbar targets using ARIA and CSS pseudo-elements, but Warp's scrollbars are custom-rendered Rust elements with track/thumb hit-test bounds. Rewrite this around the scrollable component's rendered scrollbar bounds so implementers can actually detect pointer bypass.

[oz-for-oss]

💡 [SUGGESTION] Replace the placeholder discovery guidance with the actual component or wrapper names for each in-scope container; otherwise different implementers can opt in different subsets despite the all-containers scope.

[lonexreb]

Round-2 fixes: (1) generalized clamp to both vertical AND horizontal axes — per-axis boundary check, axis-lock for diagonal gestures (>=70% threshold then first-boundary-collision tiebreaker for ambiguous diagonals); (2) replaced ARIA/CSS-pseudo scrollbar contract with Warp's actual scrollbar::hit_test() -> ScrollbarTarget API in crates/warpui_core/src/elements/shared_scrollbar.rs — Track/Thumb click bypasses clamp, wheel over same area still respects clamp; (3) Implementation Pointers now enumerate every blocklist-embedded scroll container found in the codebase with file path and component identifier (code_diff_view.rs, diff_viewer.rs, file_tree/view.rs, gfm_table.rs, block_list_viewport.rs); tests cover each container type plus horizontal/diagonal axis cases (T_diff_clamp, T_filetree_clamp, T_table_clamp, T_log_clamp, T_horizontal_axis_clamp, T_diagonal_axis_lock, T_wheel_over_scrollbar).

[lonexreb]

/oz-review

[oz-for-oss]

@lonexreb

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This spec defines gesture ownership for blocklist-embedded scroll containers and includes acceptance criteria, implementation pointers, and a test plan for wheel/trackpad and scrollbar behavior.

Concerns

• Ambiguous diagonal gestures can still resolve ownership after a mid-gesture boundary collision, which conflicts with the binding rule that ownership is decided only at gesture start.
• The 200ms gesture boundary is described inconsistently as <=200ms, >=200ms, and >200ms across the behavior contract, acceptance criteria, and implementation notes.
• Some implementation pointers need correction before implementation: standalone diff scope is inconsistent with blocklist-only scope, the scrollbar hit-test API is referenced as existing but is not present in the current shared scrollbar module, and the suggested owner enum cannot represent mixed per-axis ownership.

Verdict

Found: 0 critical, 5 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

⚠️ [IMPORTANT] Resolving the lock when the inner container hits a boundary mid-gesture conflicts with B-OWN's rule that boundary state at gesture start is the sole binding decision point; define ambiguous diagonal ownership entirely at gesture start or add an explicit exception.

[oz-for-oss]

⚠️ [IMPORTANT] The threshold is inconsistent: B2 keeps events within ≤200ms in the same gesture, A2/T2 use a >200ms gap, but this line says ≥200ms of idleness starts a new gesture; choose one boundary condition and use it everywhere.

[oz-for-oss]

⚠️ [IMPORTANT] Code diff (standalone) conflicts with the stated blocklist-embedded-only scope and the out-of-scope exclusion for top-level app panels; either limit this row to embedded/inline display modes or explain why the standalone full-pane diff is included in V1.

[oz-for-oss]

⚠️ [IMPORTANT] This owner enum cannot represent the specified mixed-axis cases where the parent owns the primary axis while the inner container still consumes the non-primary axis; specify per-axis owner state instead of a single Parent/InnerVertical/InnerHorizontal value.

[oz-for-oss]

⚠️ [IMPORTANT] The spec refers to a ScrollbarTarget hit-test API as the routing mechanism, but the current shared_scrollbar.rs exposes geometry helpers without that API; state that implementation must add this API or point to the existing hit-test path to avoid an unimplementable pointer.

[lonexreb]

Round-3 review fixes (commit e8be75e):

• Diagonal ambiguous-gesture binding-rule fix: ambiguous diagonals now resolve the primary axis at gesture start (not by mid-gesture boundary collision). New deterministic priority: (1) at-boundary axis if exactly one, (2) vertical tie-break if both, (3) vertical default if neither. No mid-gesture ownership transfer.
• 200ms boundary canonicalized: B2 now defines a single rule — same gesture iff gap < 200ms; gap >= 200ms ends the gesture. All references (B3, B-OWN, A2) updated; the earlier <=200ms / >=200ms / >200ms mix is replaced by the single canonical rule.
• Standalone diff scope: app/src/code/diff_viewer.rs is rendered in the Code panel, not the blocklist viewport — explicitly removed from the impl-pointer table and called out in Scope and Out of scope as out of scope for V1.
• Scrollbar hit-test API: verified shared_scrollbar.rs does NOT expose ScrollbarTarget / hit_test() today (only ScrollbarGeometry { track_bounds, thumb_bounds, .. }, compute_scrollbar_geometry, scroll_delta_for_pointer_movement). Spec now declares the helper as NEW in V1 (pure function over the existing geometry fields, no new state) with reference pseudocode.
• Per-axis ownership representation: replaced the flat InnerVertical | InnerHorizontal | Parent | None enum with a GestureOwnership { vertical: AxisOwner, horizontal: AxisOwner, last_event: Option<Instant> } struct so "primary parent-owned, non-primary inner-owned" is representable. T_diagonal_axis_lock updated to cover the new ambiguous sub-cases.

[lonexreb]

/oz-review