erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions

fix: stop the advanced-options scrollbar from snapping back every frame

Browse source 
UiMarkupList.OnDraw/DrawColumns re-ran their "keep the selected row
visible" clamp on EVERY draw call, not only when SelectedIndexSource
actually changed. A real bound list (MossTank's Advanced Options,
selected="{SelectedAdvancedOptionIndex}") holds a stable selected index
while the user operates the scrollbar, so the very next frame's OnDraw
saw that (unchanged) row now outside the just-scrolled view and yanked
_topRow straight back to it — undoing the arrow click or thumb drag
before it was ever visible. This is exactly the "sometimes works"
symptom: a scroll that happens to keep the selected row in view
survives; one that moves it out of view gets reverted on the next
frame. Gate the clamp on an observed change in the selected index
(int.MinValue sentinel so the first frame still reveals it), so a
stable selection no longer fights user-driven scrolling while a
genuinely new selection is still auto-revealed once.

New tests build a real nested Panel > Group > List tree at non-zero
offsets and drive it exclusively through UiRoot's public mouse entry
points, ruling out a coordinate-frame bug in the hit-test/dispatch
chain as well as proving the fix for both single-column and
<column>-mode lists (arrow click and thumb drag).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-09-07 18:20:53 +02:00
parent 3bb3ec930d
commit 0027690306
2 changed files with 285 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

50
src/AcDream.App/UI/UiMarkupList.cs
View file

@ -116,6 +116,28 @@ public sealed class UiMarkupList : UiElement
private int _topRow;
private IReadOnlyList<UiMarkupListColumn>? _columns;
/// <summary>
/// Owner live-client report 2026-09-07 ("Scrolling in advanced options
/// does not work... or it works sometimes"): the last
/// <see cref="SelectedIndexSource"/> value the "keep selection visible"
/// clamp below (<see cref="OnDraw"/>/<see cref="DrawColumns"/>) actually
/// reacted to. A real bound list (e.g. MossTank's Advanced Options,
/// <c>selected="{SelectedAdvancedOptionIndex}"</c>) keeps a STABLE
/// selected index while the user scrolls elsewhere with the scrollbar —
/// re-running the reveal clamp on EVERY frame regardless of whether
/// selection actually changed snapped <see cref="_topRow"/> straight back
/// to the (unchanged) selected row on the very next draw, undoing the
/// scrollbar arrow/track/thumb interaction the same frame it happened.
/// Gating the clamp on an observed CHANGE in the selected index — the
/// only time retail HudList-style lists scroll to reveal a row — lets a
/// stable selection coexist with the user scrolling away from it via the
/// bar, while still auto-revealing a genuinely NEW selection exactly
/// once. Sentinel <see cref="int.MinValue"/> so the very first draw with
/// ANY selected index (including the valid -1 "nothing selected") still
/// runs the clamp once.
/// </summary>
private int _lastRevealedSelected = int.MinValue;
/// <summary>
/// Pixel-based scroll projection used ONLY to feed
/// <see cref="UiScrollbar.ThumbRect"/>'s geometry math (thumb
@ -164,12 +186,16 @@ public sealed class UiMarkupList : UiElement
: 0f;
int visibleRows = VisibleRows;
int selected = SelectedIndexSource();
if (selected >= 0 && selected < items.Count)
if (selected != _lastRevealedSelected)
{
if (selected < _topRow)
_topRow = selected;
else if (selected >= _topRow + visibleRows)
_topRow = selected - visibleRows + 1;
_lastRevealedSelected = selected;
if (selected >= 0 && selected < items.Count)
{
if (selected < _topRow)
_topRow = selected;
else if (selected >= _topRow + visibleRows)
_topRow = selected - visibleRows + 1;
}
}
ClampTop(items.Count, visibleRows);
@ -383,12 +409,16 @@ public sealed class UiMarkupList : UiElement
ComputeColumnLayout(columns, contentWidth);
int selected = SelectedIndexSource();
if (selected >= 0 && selected < rowCount)
if (selected != _lastRevealedSelected)
{
if (selected < _topRow)
_topRow = selected;
else if (selected >= _topRow + visibleRows)
_topRow = selected - visibleRows + 1;
_lastRevealedSelected = selected;
if (selected >= 0 && selected < rowCount)
{
if (selected < _topRow)
_topRow = selected;
else if (selected >= _topRow + visibleRows)
_topRow = selected - visibleRows + 1;
}
}
ClampTop(rowCount, visibleRows);

245
tests/AcDream.App.Tests/UI/UiMarkupListScrollSelectionPersistenceTests.cs Normal file
View file

@ -0,0 +1,245 @@
using System.Linq;
using System.Numerics;
using AcDream.App.Rendering;
using AcDream.App.Rendering.Gpu;
using AcDream.App.Tests.Rendering.Gpu;
using AcDream.App.UI;
using Xunit;
namespace AcDream.App.Tests.UI;
/// <summary>
/// Owner live-client report 2026-09-07 ("Scrolling in advanced options does
/// not work. Scrollbar shows, but can't click or drag slidebar, or it works
/// sometimes"): root cause is in <see cref="UiMarkupList"/> itself, not the
/// coordinate frame between the popup/group/panel nesting.
///
/// <para>
/// <see cref="UiMarkupList.OnDraw"/>/<see cref="UiMarkupList.DrawColumns"/>
/// run a "keep the selected row visible" clamp UNCONDITIONALLY on every
/// single draw call (every frame), not only when the selection actually
/// changes. A real plugin list (e.g. MossTank's Advanced Options,
/// <c>selected="{SelectedAdvancedOptionIndex}"</c>) keeps a STABLE selected
/// index while the user scrolls elsewhere with the scrollbar — but because
/// that index does not change, the very next frame's <c>OnDraw</c> sees the
/// (unchanged) selected row now outside the just-scrolled view and snaps
/// <c>_topRow</c> straight back to reveal it, undoing the arrow click or
/// thumb drag before the user ever sees the new scroll position. This
/// reproduces exactly the "sometimes works" symptom: a scroll that keeps the
/// stable selected row in view survives (looks like it "worked"); one that
/// moves the selected row out of view gets silently reverted on the next
/// frame.
/// </para>
///
/// <para>
/// Every test here builds a REAL nested Panel &gt; Group &gt; List tree (like
/// MossTank's Advanced Options group living inside its panel) at non-zero
/// offsets at every level, and drives it exclusively through
/// <see cref="UiRoot"/>'s public mouse entry points — the same path
/// <c>GameWindow</c> uses — to also rule out a coordinate-frame bug in the
/// hit-test/dispatch chain (candidate 1 in the investigation). A stray
/// coordinate-frame bug would make the FIRST click (the arrow / thumb press)
/// silently miss the scrollbar; these tests assert that press succeeds AND
/// that the resulting scroll position survives a subsequent redraw (the
/// real regression).
/// </para>
/// </summary>
public sealed class UiMarkupListScrollSelectionPersistenceTests
{
private sealed class TestElement : UiElement { }
private const float ListWidth = 100f;
private const float ListHeight = 100f; // RowHeight 10 => 10 visible rows
private const float RowHeight = 10f;
private const int RowCount = 40;
// Non-zero nesting offsets at every level (panel > group > list), mirroring
// MossTank's own panel(28,42) > group(8,42) > list(4,24) structure.
private const float PanelLeft = 50f, PanelTop = 60f;
private const float GroupLeft = 10f, GroupTop = 20f;
private const float ListLeft = 5f, ListTop = 15f;
private static (uint tex, int w, int h) Resolve(uint id) => (id, 16, 16);
private sealed class Harness
{
public readonly UiRoot Root = new() { Width = 1000f, Height = 800f };
public readonly TestElement Panel;
public readonly TestElement Group;
public readonly UiMarkupList List;
public Harness(UiMarkupList list)
{
List = list;
Panel = new TestElement
{
Left = PanelLeft, Top = PanelTop, Width = 900f, Height = 700f,
Draggable = true,
};
Group = new TestElement
{
Left = GroupLeft, Top = GroupTop, Width = 800f, Height = 600f,
};
List.Left = ListLeft; List.Top = ListTop;
Group.AddChild(List);
Panel.AddChild(Group);
Root.AddChild(Panel);
}
/// <summary>Absolute root-space screen coords for a point expressed in
/// the list's own local space — the same translation
/// <see cref="UiElement.ScreenPosition"/> performs by walking every
/// ancestor's Left/Top.</summary>
public (int x, int y) ToScreen(float localX, float localY) =>
((int)(PanelLeft + GroupLeft + ListLeft + localX),
(int)(PanelTop + GroupTop + ListTop + localY));
public void Click(float localX, float localY)
{
var (x, y) = ToScreen(localX, localY);
Root.OnMouseDown(UiMouseButton.Left, x, y);
Root.OnMouseUp(UiMouseButton.Left, x, y);
}
public void PressMoveRelease(float downLocalX, float downLocalY, float moveLocalX, float moveLocalY)
{
var (dx, dy) = ToScreen(downLocalX, downLocalY);
Root.OnMouseDown(UiMouseButton.Left, dx, dy);
var (mx, my) = ToScreen(moveLocalX, moveLocalY);
Root.OnMouseMove(mx, my);
Root.OnMouseUp(UiMouseButton.Left, mx, my);
}
}
// ── Single-column mode ───────────────────────────────────────────────
[Fact]
public void SingleColumn_DownArrowClick_TopRowSurvivesTheNextDraw()
{
var list = new UiMarkupList
{
Width = ListWidth, Height = ListHeight, RowHeight = RowHeight,
SpriteResolve = Resolve,
// A REALISTIC stable selection (unlike the -1 the existing
// scrollbar tests use) — the user has row 0 selected and is not
// touching selection while scrolling elsewhere via the bar.
SelectedIndexSource = () => 0,
ItemsSource = () => Enumerable.Range(0, RowCount).Select(i => $"row{i}").ToArray(),
};
var h = new Harness(list);
h.Root.DrawSelfAndChildren(NullCtx());
// Down-arrow occupies the bottom 16px of the reserved scrollbar
// column: x in [84,100), y in [84,100).
h.Click(localX: 90, localY: 90);
// Simulate the NEXT render frame — this is where the unconditional
// reveal-selected clamp (pre-fix) reverts the scroll.
h.Root.DrawSelfAndChildren(NullCtx());
int? selected = null;
list.SelectionChanged = row => selected = row;
// Click the first visible row (local y in row 0's band).
h.Click(localX: 10, localY: 2);
Assert.Equal(1, selected);
}
[Fact]
public void SingleColumn_ThumbDrag_TopRowAdvancesThreeRows_AndSurvivesTheNextDraw()
{
var list = new UiMarkupList
{
Width = ListWidth, Height = ListHeight, RowHeight = RowHeight,
SpriteResolve = Resolve,
SelectedIndexSource = () => 0,
ItemsSource = () => Enumerable.Range(0, RowCount).Select(i => $"row{i}").ToArray(),
};
var h = new Harness(list);
h.Root.DrawSelfAndChildren(NullCtx());
// Track: y in [16,84) (Height 100 - 16 up - 16 down). Thumb starts at
// the very top (topRow=0): height = max(8, 68*10/40=17) = 17, so
// y in [16,33). Press at the thumb's own top edge (y=16, zero drag
// offset) and drag down 5px — by the exact geometry in this file's
// header comment, this lands on topRow=3.
h.PressMoveRelease(downLocalX: 90, downLocalY: 16, moveLocalX: 90, moveLocalY: 21);
h.Root.DrawSelfAndChildren(NullCtx());
int? selected = null;
list.SelectionChanged = row => selected = row;
h.Click(localX: 10, localY: 2);
Assert.Equal(3, selected);
}
// ── Multi-column mode ────────────────────────────────────────────────
[Fact]
public void Columns_DownArrowClick_TopRowSurvivesTheNextDraw()
{
var list = new UiMarkupList
{
Width = ListWidth, Height = ListHeight, RowHeight = RowHeight,
SpriteResolve = Resolve,
SelectedIndexSource = () => 0,
Columns = new[]
{
UiMarkupListColumn.Text(
ListWidth, () => Enumerable.Range(0, RowCount).Select(i => $"row{i}").ToArray(), null),
},
};
var h = new Harness(list);
h.Root.DrawSelfAndChildren(NullCtx());
h.Click(localX: 90, localY: 90);
h.Root.DrawSelfAndChildren(NullCtx());
int? selected = null;
list.SelectionChanged = row => selected = row;
h.Click(localX: 10, localY: 2);
Assert.Equal(1, selected);
}
[Fact]
public void Columns_ThumbDrag_TopRowAdvancesThreeRows_AndSurvivesTheNextDraw()
{
var list = new UiMarkupList
{
Width = ListWidth, Height = ListHeight, RowHeight = RowHeight,
SpriteResolve = Resolve,
SelectedIndexSource = () => 0,
Columns = new[]
{
UiMarkupListColumn.Text(
ListWidth, () => Enumerable.Range(0, RowCount).Select(i => $"row{i}").ToArray(), null),
},
};
var h = new Harness(list);
h.Root.DrawSelfAndChildren(NullCtx());
h.PressMoveRelease(downLocalX: 90, downLocalY: 16, moveLocalX: 90, moveLocalY: 21);
h.Root.DrawSelfAndChildren(NullCtx());
int? selected = null;
list.SelectionChanged = row => selected = row;
h.Click(localX: 10, localY: 2);
Assert.Equal(3, selected);
}
private static UiRenderContext NullCtx()
{
var device = new RecordingGpuDevice();
var renderer = new TextRenderer(device, new NullGpuFrameSource(), "unused");
renderer.Begin(new Vector2(1000f, 800f));
return new UiRenderContext(renderer, new Vector2(1000f, 800f));
}
private sealed class NullGpuFrameSource : ICurrentGpuFrameSource
{
public IGpuFrame? CurrentFrame => null;
}
}