Merge pull request #4139 from icsharpcode/fix/scroll-children-on-expand

Scroll a node's new children into view when it is expanded

ILSpy.Tests/Controls/SharpTreeViewTests.cs

@@ -18,12 +18,15 @@
 
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading.Tasks;
 
 using Avalonia.Controls;
+using Avalonia.Controls.Primitives;
 using Avalonia.Headless;
 using Avalonia.Headless.NUnit;
 using Avalonia.Input;
 using Avalonia.Threading;
+using Avalonia.VisualTree;
 
 using AwesomeAssertions;
 
@@ -290,6 +293,107 @@ public class SharpTreeViewTests
 		RenderedRows(tree).Should().Equal("A", "C", "B", "B1");
 	}
 
+	/// <summary>Builds <paramref name="rootCount"/> top-level rows in a viewport too short to show
+	/// them all, with one expandable node, so an expansion has somewhere to scroll.</summary>
+	static (Window window, SharpTreeView tree, ScrollViewer scrollViewer, TestNode[] nodes) ShortViewport(
+		int rootCount, int expandableIndex, int childCount)
+	{
+		var nodes = Enumerable.Range(0, rootCount)
+			.Select(i => i == expandableIndex
+				? new TestNode($"N{i}", Enumerable.Range(0, childCount)
+					.Select(c => new TestNode($"N{i}.{c}")).ToArray())
+				: new TestNode($"N{i}"))
+			.ToArray();
+		var root = new TestNode("root", nodes);
+		var tree = new SharpTreeView { ShowRoot = false, Root = root };
+		var window = new Window { Content = tree, Width = 300, Height = 180 };
+		window.Show();
+		Dispatcher.UIThread.RunJobs();
+		return (window, tree, tree.GetVisualDescendants().OfType<ScrollViewer>().First(), nodes);
+	}
+
+	/// <summary>Expands a node the way a user does, with Right on its focused row.</summary>
+	static void PressRightOn(Window window, SharpTreeView tree, SharpTreeNode node)
+	{
+		tree.SelectedItem = node;
+		Dispatcher.UIThread.RunJobs();
+		tree.ContainerFromItem(node)?.Focus();
+		Dispatcher.UIThread.RunJobs();
+		window.KeyPress(Key.Right, RawInputModifiers.None, PhysicalKey.ArrowRight, null);
+		Dispatcher.UIThread.RunJobs();
+	}
+
+	[AvaloniaTest]
+	public void Expanding_A_Node_Scrolls_Children_That_Do_Not_Fit_Into_View()
+	{
+		var (window, tree, scrollViewer, nodes) = ShortViewport(rootCount: 15, expandableIndex: 5, childCount: 5);
+		var parent = nodes[5];
+		scrollViewer.Offset.Y.Should().Be(0, "nothing has moved the viewport yet");
+
+		PressRightOn(window, tree, parent);
+
+		tree.IsNodeFullyVisible(parent.Children[^1])
+			.Should().BeTrue("the expansion reveals children below the viewport, so the view scrolls to show them");
+		tree.IsNodeFullyVisible(parent)
+			.Should().BeTrue("the scroll is bounded by the expanded node: it never leaves the viewport");
+	}
+
+	[AvaloniaTest]
+	public void Expanding_A_Node_Whose_Children_Already_Fit_Leaves_The_Viewport_Alone()
+	{
+		var (window, tree, scrollViewer, nodes) = ShortViewport(rootCount: 15, expandableIndex: 1, childCount: 2);
+		var parent = nodes[1];
+
+		PressRightOn(window, tree, parent);
+
+		scrollViewer.Offset.Y.Should().Be(0, "the children fit below the node, so there is nothing to scroll to");
+		tree.IsNodeFullyVisible(parent.Children[^1]).Should().BeTrue();
+	}
+
+	[AvaloniaTest]
+	public void Expanding_A_Node_With_More_Children_Than_Fit_Keeps_The_Node_Visible()
+	{
+		var (window, tree, _, nodes) = ShortViewport(rootCount: 15, expandableIndex: 5, childCount: 30);
+		var parent = nodes[5];
+
+		PressRightOn(window, tree, parent);
+
+		tree.IsNodeFullyVisible(parent)
+			.Should().BeTrue("showing every child would push the node off the top, so the scroll stops at the node");
+		tree.IsNodeFullyVisible(parent.Children[0])
+			.Should().BeTrue("as many children as fit are shown below it");
+	}
+
+	[AvaloniaTest]
+	public async Task Clicking_The_Expander_Scrolls_The_Children_Into_View()
+	{
+		// The mouse path never passes through SharpTreeView: the row template's toggle writes
+		// IsExpanded straight to the node, so the reveal hangs off the toggle's Click.
+		var (window, tree, _, nodes) = ShortViewport(rootCount: 15, expandableIndex: 5, childCount: 5);
+		var parent = nodes[5];
+
+		await window.ClickAsync(() => tree.ContainerFromItem(parent)?.GetVisualDescendants()
+			.OfType<ToggleButton>().FirstOrDefault(b => b.Name == "PART_Expander"));
+		Dispatcher.UIThread.RunJobs();
+
+		parent.IsExpanded.Should().BeTrue("precondition: the click toggled the node open");
+		tree.IsNodeFullyVisible(parent.Children[^1])
+			.Should().BeTrue("a click on the expander reveals the children, just as the keyboard does");
+	}
+
+	[AvaloniaTest]
+	public void Expanding_A_Node_In_Code_Does_Not_Move_The_Viewport()
+	{
+		// Revealing a node expands its ancestors first (ScrollIntoNodeView, TreeSelectionBinder)
+		// and positions the viewport itself afterwards; a scroll per ancestor would fight that.
+		var (_, _, scrollViewer, nodes) = ShortViewport(rootCount: 15, expandableIndex: 5, childCount: 5);
+
+		nodes[5].IsExpanded = true;
+		Dispatcher.UIThread.RunJobs();
+
+		scrollViewer.Offset.Y.Should().Be(0, "only a user gesture reveals the children");
+	}
+
 	static List<string> RenderedRows(SharpTreeView tree)
 	{
 		var rows = new List<string>();

ILSpy/Controls/TreeView/SharpTreeView.cs

@@ -59,7 +59,6 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 			AvaloniaProperty.Register<SharpTreeView, bool>(nameof(ShowLines), defaultValue: true);
 
 		TreeFlattener? flattener;
-		bool doNotScrollOnExpanding;
 		string searchBuffer = string.Empty;
 		DispatcherTimer? searchResetTimer;
 
@@ -92,6 +91,12 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 			AddHandler(DragDrop.DragOverEvent, OnDragOver);
 			AddHandler(DragDrop.DropEvent, OnDrop);
 			AddHandler(DragDrop.DragLeaveEvent, (_, _) => HideInsertMarker());
+			// The row template's expander writes IsExpanded straight to the node, so an expansion
+			// made with the mouse never passes through this control; its Click is what identifies
+			// one. Only a gesture scrolls: code that expands nodes to reveal a selection (see
+			// ScrollIntoNodeView) or to open every match of a filter positions the viewport itself,
+			// and a scroll per expanded node would fight it.
+			AddHandler(Button.ClickEvent, OnExpanderClick, RoutingStrategies.Bubble, handledEventsToo: true);
 		}
 
 		public SharpTreeNode? Root {
@@ -184,32 +189,47 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 			node.ActivateItem(args);
 			if (!e.Handled && node.ShowExpander)
 			{
-				node.IsExpanded = !node.IsExpanded;
+				SetExpanded(node, !node.IsExpanded);
 				e.Handled = true;
 			}
 		}
 
+		void OnExpanderClick(object? sender, RoutedEventArgs e)
+		{
+			if (e.Source is ToggleButton { Name: "PART_Expander" } expander
+				&& expander.DataContext is SharpTreeNode { IsExpanded: true } node)
+			{
+				HandleExpanding(node);
+			}
+		}
+
+		/// <summary>Expands or collapses <paramref name="node"/> as a user gesture, so an expansion
+		/// reveals its children the way <see cref="HandleExpanding"/> describes.</summary>
+		void SetExpanded(SharpTreeNode node, bool expanded)
+		{
+			if (node.IsExpanded == expanded)
+				return;
+			node.IsExpanded = expanded;
+			if (expanded)
+				HandleExpanding(node);
+		}
+
 		/// <summary>
-		/// Called when a visible node expands so its newly shown children are scrolled into view
+		/// Scrolls the rows a just-expanded node revealed into view, the way the native Windows
-		/// (without scrolling the node itself off the top).
+		/// tree control does: far enough to show the new children, but never so far that the
+		/// expanded node itself leaves the viewport. Both steps only move the viewport when their
+		/// row lies outside it, so expanding a node whose children already fit below it does not
+		/// scroll at all.
 		/// </summary>
-		internal void HandleExpanding(SharpTreeNode node)
+		void HandleExpanding(SharpTreeNode node)
 		{
-			if (doNotScrollOnExpanding)
-				return;
 			SharpTreeNode lastVisibleChild = node;
-			while (true)
+			while (lastVisibleChild.Children.LastOrDefault(c => c.IsVisible) is { } child)
-			{
-				var child = lastVisibleChild.Children.LastOrDefault(c => c.IsVisible);
-				if (child == null)
-					break;
 				lastVisibleChild = child;
-			}
+			if (lastVisibleChild == node)
-			if (lastVisibleChild != node)
+				return;
-			{
+			ScrollRowIntoView(lastVisibleChild, centre: false);
-				ScrollRowIntoView(lastVisibleChild, centre: false);
+			ScrollRowIntoView(node, centre: false);
-				Dispatcher.UIThread.Post(() => ScrollRowIntoView(node, centre: false), DispatcherPriority.Loaded);
-			}
 		}
 
 		/// <summary>Scrolls the node into view (unless <paramref name="scroll"/> is false) and gives it
@@ -252,10 +272,8 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 		public void ScrollIntoNodeView(SharpTreeNode node)
 		{
 			ArgumentNullException.ThrowIfNull(node);
-			doNotScrollOnExpanding = true;
 			foreach (var ancestor in node.Ancestors())
 				ancestor.IsExpanded = true;
-			doNotScrollOnExpanding = false;
 			CenterNodeInView(node);
 		}
 
@@ -403,7 +421,7 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 				{
 					case Key.Left:
 						if (node.IsExpanded)
-							node.IsExpanded = false;
+							SetExpanded(node, false);
 						else if (node.Parent != null && !node.Parent.IsRoot)
 							SelectAndFocus(node.Parent);
 						else
@@ -412,7 +430,7 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 						break;
 					case Key.Right:
 						if (!node.IsExpanded && node.ShowExpander)
-							node.IsExpanded = true;
+							SetExpanded(node, true);
 						else if (node.Children.Count > 0)
 							SelectAndFocus(node.Children.First(c => c.IsVisible));
 						else
@@ -420,16 +438,18 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 						e.Handled = true;
 						break;
 					case Key.Add:
-						node.IsExpanded = true;
+						SetExpanded(node, true);
 						e.Handled = true;
 						break;
 					case Key.Subtract:
-						node.IsExpanded = false;
+						SetExpanded(node, false);
 						e.Handled = true;
 						break;
 					case Key.Multiply:
 						node.IsExpanded = true;
 						ExpandRecursively(node);
+						// The whole subtree is open now, so this reveals as much of it as fits.
+						HandleExpanding(node);
 						e.Handled = true;
 						break;
 					case Key.Enter: