From 3258cd2a7f6620b73ba6024ecc45db28349bf3f4 Mon Sep 17 00:00:00 2001
From: Antoine Beyeler <49431240+abey79@users.noreply.github.com>
Date: Sun, 17 Mar 2024 17:12:41 +0100
Subject: [PATCH] Fix two `ScrollArea` bugs: leaking scroll target and broken
 animation to target offset (#4174)

This PR fixes two issues related to `ScrollArea`.

1) When a `ScrollArea` would have `drag_to_scroll` set to `false` (e.g.
because some custom logic is at play or some other reason), it would not
animate to the `target_offset`, effectively making
`Response::scroll_to_me()` ineffective.
2) Single-direction `ScrollArea`s would leak the `scroll_target`'s other
direction. In certain specific circumstances (e.g. an horizontal area
nested in a vertical one, or inversely), this _could_ work as intended,
but in many other cases it could cause unwanted effects. With this PR,
both `scroll_target` directions are consumed by nearest enclosing
`ScrollArea`, regardless of the actually enabled scroll axes.
---
 crates/egui/src/containers/scroll_area.rs | 89 ++++++++++++-----------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/crates/egui/src/containers/scroll_area.rs b/crates/egui/src/containers/scroll_area.rs
index 27128f67..c35cca34 100644
--- a/crates/egui/src/containers/scroll_area.rs
+++ b/crates/egui/src/containers/scroll_area.rs
@@ -553,6 +553,7 @@ impl ScrollArea {
         }
 
         let viewport = Rect::from_min_size(Pos2::ZERO + state.offset, inner_size);
+        let dt = ui.input(|i| i.stable_dt).at_most(0.1);
 
         if (scrolling_enabled && drag_to_scroll)
             && (state.content_is_too_large[0] || state.content_is_too_large[1])
@@ -577,48 +578,50 @@ impl ScrollArea {
                 }
             } else {
                 for d in 0..2 {
-                    let dt = ui.input(|i| i.stable_dt).at_most(0.1);
+                    // Kinetic scrolling
+                    let stop_speed = 20.0; // Pixels per second.
+                    let friction_coeff = 1000.0; // Pixels per second squared.
 
-                    if let Some(scroll_target) = state.offset_target[d] {
+                    let friction = friction_coeff * dt;
+                    if friction > state.vel[d].abs() || state.vel[d].abs() < stop_speed {
                         state.vel[d] = 0.0;
-
-                        if (state.offset[d] - scroll_target.target_offset).abs() < 1.0 {
-                            // Arrived
-                            state.offset[d] = scroll_target.target_offset;
-                            state.offset_target[d] = None;
-                        } else {
-                            // Move towards target
-                            let t = emath::interpolation_factor(
-                                scroll_target.animation_time_span,
-                                ui.input(|i| i.time),
-                                dt,
-                                emath::ease_in_ease_out,
-                            );
-                            if t < 1.0 {
-                                state.offset[d] =
-                                    emath::lerp(state.offset[d]..=scroll_target.target_offset, t);
-                                ctx.request_repaint();
-                            } else {
-                                // Arrived
-                                state.offset[d] = scroll_target.target_offset;
-                                state.offset_target[d] = None;
-                            }
-                        }
                     } else {
-                        // Kinetic scrolling
-                        let stop_speed = 20.0; // Pixels per second.
-                        let friction_coeff = 1000.0; // Pixels per second squared.
+                        state.vel[d] -= friction * state.vel[d].signum();
+                        // Offset has an inverted coordinate system compared to
+                        // the velocity, so we subtract it instead of adding it
+                        state.offset[d] -= state.vel[d] * dt;
+                        ctx.request_repaint();
+                    }
+                }
+            }
+        }
 
-                        let friction = friction_coeff * dt;
-                        if friction > state.vel[d].abs() || state.vel[d].abs() < stop_speed {
-                            state.vel[d] = 0.0;
-                        } else {
-                            state.vel[d] -= friction * state.vel[d].signum();
-                            // Offset has an inverted coordinate system compared to
-                            // the velocity, so we subtract it instead of adding it
-                            state.offset[d] -= state.vel[d] * dt;
-                            ctx.request_repaint();
-                        }
+        // Scroll with an animation if we have a target offset (that hasn't been cleared by the code
+        // above).
+        for d in 0..2 {
+            if let Some(scroll_target) = state.offset_target[d] {
+                state.vel[d] = 0.0;
+
+                if (state.offset[d] - scroll_target.target_offset).abs() < 1.0 {
+                    // Arrived
+                    state.offset[d] = scroll_target.target_offset;
+                    state.offset_target[d] = None;
+                } else {
+                    // Move towards target
+                    let t = emath::interpolation_factor(
+                        scroll_target.animation_time_span,
+                        ui.input(|i| i.time),
+                        dt,
+                        emath::ease_in_ease_out,
+                    );
+                    if t < 1.0 {
+                        state.offset[d] =
+                            emath::lerp(state.offset[d]..=scroll_target.target_offset, t);
+                        ctx.request_repaint();
+                    } else {
+                        // Arrived
+                        state.offset[d] = scroll_target.target_offset;
+                        state.offset_target[d] = None;
                     }
                 }
             }
@@ -753,11 +756,13 @@ impl Prepared {
         let content_size = content_ui.min_size();
 
         for d in 0..2 {
+            // We always take both scroll targets regardless of which scroll axes are enabled. This
+            // is to avoid them leaking to other scroll areas.
+            let scroll_target = content_ui
+                .ctx()
+                .frame_state_mut(|state| state.scroll_target[d].take());
+
             if scroll_enabled[d] {
-                // We take the scroll target so only this ScrollArea will use it:
-                let scroll_target = content_ui
-                    .ctx()
-                    .frame_state_mut(|state| state.scroll_target[d].take());
                 if let Some((target_range, align)) = scroll_target {
                     let min = content_ui.min_rect().min[d];
                     let clip_rect = content_ui.clip_rect();