Merge branch 'claude/search-modal-closing-cd05c4'

src/DodoSSH.Client.App/Views/MainWindow.axaml.cs

@@ -79,9 +79,15 @@ internal sealed partial class MainWindow : Window
     /// </summary>
     /// <remarks>
     /// <para>
-    /// A tunnelled handler rather than <c>KeyBindings</c>, because three of these four keys have to be
+    /// Ctrl+K is here rather than on the palette because it has to work when the palette is not showing, and
-    /// intercepted before the control under the pointer sees them: Escape and the arrows belong to the
+    /// it is a plain handler rather than a <c>KeyBinding</c> so that toggling stays one code path with the
-    /// palette while it is open, and the palette's own text box would otherwise eat them.
+    /// rest of the chord set.
+    /// </para>
+    /// <para>
+    /// The palette's own keys are forwarded rather than answered: <see cref="QuickConnect"/> intercepts them
+    /// on their way down while the focus is inside it, and this is the net for when it is not — a press that
+    /// arrives with nothing focused, or from a control on the screen behind, still has to close the palette
+    /// rather than fall through to whatever is underneath it.
     /// </para>
     /// <para>
     /// None of this reaches the terminal, and it does not need to. Once the WebView's child window holds
@@ -104,61 +110,12 @@ internal sealed partial class MainWindow : Window
         }
         else if (viewModel.IsSearching)
         {
-            HandlePaletteKey(viewModel, e);
+            Palette.HandleKey(e);
         }
 
         base.OnKeyDown(e);
     }
 
-    /// <remarks>
-    /// The selection is moved here rather than by letting the list take focus, because the list taking
-    /// focus is exactly what would stop the query box receiving the next character typed.
-    /// </remarks>
-    private static void HandlePaletteKey(MainWindowViewModel viewModel, KeyEventArgs e)
-    {
-        switch (e.Key)
-        {
-            case Key.Escape:
-                viewModel.CloseSearchCommand.Execute(null);
-                e.Handled = true;
-                break;
-
-            case Key.Enter:
-                viewModel.ConnectToSearchResultCommand.Execute(null);
-                e.Handled = true;
-                break;
-
-            case Key.Down:
-                Move(viewModel, 1);
-                e.Handled = true;
-                break;
-
-            case Key.Up:
-                Move(viewModel, -1);
-                e.Handled = true;
-                break;
-
-            default:
-                break;
-        }
-    }
-
-    /// <remarks>Clamped rather than wrapped: a list that jumps from the last row to the first loses people.</remarks>
-    private static void Move(MainWindowViewModel viewModel, int delta)
-    {
-        if (viewModel.SearchResults.Count == 0)
-        {
-            return;
-        }
-
-        var current = viewModel.SelectedSearchResult is { } selected
-            ? viewModel.SearchResults.IndexOf(selected)
-            : -1;
-
-        viewModel.SelectedSearchResult =
-            viewModel.SearchResults[Math.Clamp(current + delta, 0, viewModel.SearchResults.Count - 1)];
-    }
-
     private void Attach(MainWindowViewModel? viewModel)
     {
         if (shell is { } previous)
@@ -216,11 +173,18 @@ internal sealed partial class MainWindow : Window
             return;
         }
 
-        // The palette is a text box somebody is expected to start typing into immediately, so opening it
+        // Closing only. Opening also has to move the keyboard — the palette is a text box somebody is
-        // has to move the caret there — including out of the terminal, which needs the Win32 half as well.
+        // expected to start typing into immediately — but the palette does that for itself when it becomes
+        // visible, which is a moment this handler is measurably ahead of: it runs from the view model's
+        // PropertyChanged, before the binding that reveals the control, and Focus() on a control that is
+        // still collapsed is a no-op that is not replayed when it is revealed.
         if (string.Equals(e.PropertyName, nameof(MainWindowViewModel.IsSearching), StringComparison.Ordinal))
         {
-            ReleaseKeyboardTo(viewModel.IsSearching ? Palette.QueryBox : KeyboardHome);
+            if (!viewModel.IsSearching)
+            {
+                ReleaseKeyboardTo(KeyboardHome);
+            }
+
             return;
         }
 

src/DodoSSH.Client.App/Views/QuickConnect.axaml

@@ -21,7 +21,13 @@
     an overlay across the middle of this window would be painted underneath it and take no clicks.
   -->
 
-  <Border Background="#CC0A0C0B">
+  <!--
+    The wash is named and takes a press, because clicking away from a palette is how every palette closes.
+    The handler answers only for presses whose source is the wash itself, which is what separates "outside"
+    from "inside": a press anywhere on the card below reports that control as its source and bubbles through
+    here on its way to the window.
+  -->
+  <Border x:Name="Backdrop" Background="#CC0A0C0B" PointerPressed="OnBackdropPressed">
     <Border Width="520" VerticalAlignment="Top" Margin="0,90,0,0"
             Background="{StaticResource Chrome}" BorderBrush="{StaticResource BorderMid}"
             BorderThickness="1" CornerRadius="6">
@@ -39,8 +45,8 @@
         </Border>
 
         <!--
-          Arrow keys move the selection and Enter takes it; the window's key handler owns both, because a
+          Arrow keys move the selection and Enter takes it; the code-behind owns both, because a ListBox that
-          ListBox that took focus would take the arrow keys away from the box being typed into.
+          took focus would take the arrow keys away from the box being typed into.
         -->
         <ListBox x:Name="Results" MaxHeight="280" Focusable="False"
                  ItemsSource="{Binding SearchResults}"

src/DodoSSH.Client.App/Views/QuickConnect.axaml.cs

@@ -1,17 +1,174 @@
+using Avalonia;
 using Avalonia.Controls;
+using Avalonia.Input;
+using Avalonia.Interactivity;
+using Avalonia.Threading;
+using DodoSSH.Client.App.ViewModels;
 
 namespace DodoSSH.Client.App.Views;
 
 /// <summary>The Ctrl+K host search, overlaid on the window.</summary>
 /// <remarks>
-/// The keys it responds to are handled by <see cref="MainWindow"/> rather than here, because two of them —
+/// <para>
-/// Ctrl+K to open and Escape to close — have to work when this control does not exist yet or has just
+/// Everything a palette does once it is open belongs here rather than in <see cref="MainWindow"/>: the four
-/// stopped existing. Handling the arrows in the same place keeps the whole chord set in one method.
+/// keys it answers to, the press outside it that dismisses it, and taking the keyboard the moment it appears.
+/// Opening it is the window's business, because Ctrl+K has to work when this control is not showing.
+/// </para>
+/// <para>
+/// The split used to fall the other way, and the cost was that none of it could be tested. Showing
+/// <see cref="MainWindow"/> initialises WebView2, which refuses the headless dispatcher's thread — see
+/// <c>LayoutHarnessTests.WhyTheWindowItselfIsNeverShown</c> — so behaviour that lived on the window could only
+/// be checked by hand. A <see cref="UserControl"/> hosts in a bare window and takes real key and pointer
+/// input.
+/// </para>
 /// </remarks>
 internal sealed partial class QuickConnect : UserControl
 {
-    public QuickConnect() => InitializeComponent();
+    public QuickConnect()
+    {
+        InitializeComponent();
 
-    /// <summary>The box, so the window can put the caret in it the moment the palette opens.</summary>
+        // Tunnelled, and deliberately: the query box below is on the route these keys take, and a text box
+        // that grows a use for Enter or the arrows — a multi-line box, a completion list — would take them
+        // before a bubbling handler here ever ran. The palette owns them while it is open, so it says so at
+        // the point on the route where nothing else has had a chance yet.
+        AddHandler(KeyDownEvent, OnPaletteKey, RoutingStrategies.Tunnel);
+    }
+
+    /// <summary>The box, so the caret can be put in it the moment the palette opens.</summary>
     internal TextBox QueryBox => Query;
+
+    private MainWindowViewModel? Shell => DataContext as MainWindowViewModel;
+
+    /// <summary>
+    /// Answers one of the palette's keys, wherever in the window it was pressed.
+    /// </summary>
+    /// <remarks>
+    /// Internal because <see cref="MainWindow"/> calls it too, for the case this control's own tunnelled
+    /// handler cannot see: a key routes through here only while the focus is inside the palette, and the
+    /// window is what catches Escape when it is not.
+    /// </remarks>
+    internal void HandleKey(KeyEventArgs e)
+    {
+        ArgumentNullException.ThrowIfNull(e);
+
+        if (Shell is not { IsSearching: true } shell)
+        {
+            return;
+        }
+
+        switch (e.Key)
+        {
+            case Key.Escape:
+                shell.CloseSearchCommand.Execute(null);
+                e.Handled = true;
+                break;
+
+            case Key.Enter:
+                shell.ConnectToSearchResultCommand.Execute(null);
+                e.Handled = true;
+                break;
+
+            case Key.Down:
+                Move(shell, 1);
+                e.Handled = true;
+                break;
+
+            case Key.Up:
+                Move(shell, -1);
+                e.Handled = true;
+                break;
+
+            default:
+                break;
+        }
+    }
+
+    /// <summary>
+    /// Takes the keyboard, so the palette can be typed into the instant it appears.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Both halves, for the reason <see cref="NativeKeyboardFocus"/> gives: the terminal's WebView is a native
+    /// child window that keeps Win32 focus even after it is collapsed, so focusing an Avalonia control without
+    /// the Win32 call produces a box with a caret in it that silently receives nothing.
+    /// </para>
+    /// <para>
+    /// Done here rather than by the window, and that is the fix rather than a tidying. <c>Focus()</c> on a
+    /// collapsed control is measurably a no-op that is not replayed when the control is revealed, and the
+    /// window's own attempt ran from the view model's <c>PropertyChanged</c> — ahead of the binding that makes
+    /// this control visible, so it focused a control that was still collapsed and the keyboard stayed wherever
+    /// it was.
+    /// </para>
+    /// <para>
+    /// Becoming visible is still too early on its own, which is why this is posted rather than called. A
+    /// control that has never been laid out has no visual children — measured: at the instant
+    /// <c>IsVisible</c> turns true the query box reports <c>IsAttachedToVisualTree() == false</c>, and focus is
+    /// refused to anything not in the tree. Layout runs at a higher priority than this callback, so by the
+    /// time it is picked up the box exists.
+    /// </para>
+    /// </remarks>
+    private void TakeKeyboard()
+    {
+        // The palette can have been dismissed between the post and the callback — a press on the wash, or a
+        // second Ctrl+K — and stealing the keyboard back into a control nobody can see would be worse than
+        // arriving late.
+        if (!IsVisible)
+        {
+            return;
+        }
+
+        if (TopLevel.GetTopLevel(this) is Window window)
+        {
+            NativeKeyboardFocus.ReturnTo(window);
+        }
+
+        Query.Focus();
+    }
+
+    /// <remarks>Clamped rather than wrapped: a list that jumps from the last row to the first loses people.</remarks>
+    private static void Move(MainWindowViewModel shell, int delta)
+    {
+        if (shell.SearchResults.Count == 0)
+        {
+            return;
+        }
+
+        var current = shell.SelectedSearchResult is { } selected
+            ? shell.SearchResults.IndexOf(selected)
+            : -1;
+
+        shell.SelectedSearchResult =
+            shell.SearchResults[Math.Clamp(current + delta, 0, shell.SearchResults.Count - 1)];
+    }
+
+    /// <inheritdoc />
+    protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
+    {
+        // Base first, so the visibility this control has just been given has already reached its descendants:
+        // focus is refused to anything not effectively visible, and the box below is a descendant.
+        base.OnPropertyChanged(change);
+
+        if (change.Property == IsVisibleProperty && change.GetNewValue<bool>())
+        {
+            Dispatcher.UIThread.Post(TakeKeyboard, DispatcherPriority.Loaded);
+        }
+    }
+
+    private void OnPaletteKey(object? sender, KeyEventArgs e) => HandleKey(e);
+
+    /// <remarks>
+    /// Only a press on the wash itself. Presses on the card bubble through here as well, and closing on those
+    /// would make the palette impossible to click into.
+    /// </remarks>
+    private void OnBackdropPressed(object? sender, PointerPressedEventArgs e)
+    {
+        if (!ReferenceEquals(e.Source, Backdrop) || Shell is not { IsSearching: true } shell)
+        {
+            return;
+        }
+
+        shell.CloseSearchCommand.Execute(null);
+        e.Handled = true;
+    }
 }

tests/DodoSSH.Client.App.Layout.Tests/QuickConnectTests.cs

@@ -0,0 +1,298 @@
+using Avalonia;
+using Avalonia.Controls;
+using Avalonia.Headless;
+using Avalonia.Input;
+using Avalonia.Threading;
+using Avalonia.VisualTree;
+using DodoSSH.Client.App.ViewModels;
+using DodoSSH.Client.App.Views;
+using DodoSSH.Client.Session;
+using DodoSSH.Client.Session.Tests;
+using DodoSSH.Client.Ssh;
+using DodoSSH.Client.Storage;
+using DodoSSH.Client.Terminal;
+using DodoSSH.Crypto;
+using NSubstitute;
+
+namespace DodoSSH.Client.App.Layout.Tests;
+
+/// <summary>
+/// How the quick-connect palette answers a keyboard and a pointer.
+/// </summary>
+/// <remarks>
+/// <para>
+/// This is the suite the palette shipped without, and the reason it shipped without one is that all of this
+/// used to live on <c>MainWindow</c> — which cannot be shown here at all, because attaching the terminal's
+/// WebView initialises WebView2 on a thread it refuses. See
+/// <see cref="LayoutHarnessTests.WhyTheWindowItselfIsNeverShown"/>. A <c>UserControl</c> hosts in a bare
+/// window, takes real key and pointer input, and can therefore be held to what it promises.
+/// </para>
+/// <para>
+/// Three things were wrong and each has a test here: nothing answered a press outside the palette, so the one
+/// gesture everybody tries first did nothing; the caret never reached the query box, because the window
+/// focused it from the view model's <c>PropertyChanged</c> — ahead of the binding that reveals the control,
+/// and focus on a collapsed control is a no-op; and the keys were answered only by a handler on the window,
+/// which anything on the route could have taken first.
+/// </para>
+/// <para>
+/// A real <see cref="MainWindowViewModel"/> over a real unlocked vault, for the same reason the layout suite
+/// uses one: compiled bindings resolve against the declared type, and the palette's list is populated by the
+/// vault's own hosts. Nothing here reaches a network — the connect the Enter test performs fails inside the
+/// vault's own error handling, which is fine, because what Enter promises is to take the highlighted result
+/// and close.
+/// </para>
+/// </remarks>
+public sealed class QuickConnectTests : IAsyncLifetime
+{
+    private const string Passphrase = "a sufficiently long passphrase";
+    private const string ServerUrl = "https://dodossh.example";
+
+    /// <remarks>Far below the shipped profile: nothing here attacks a wrap.</remarks>
+    private static readonly Argon2Profile CheapProfile =
+        Argon2Profile.FromStoredParameters(memoryKibibytes: 8 * 1024, passes: 1, parallelism: 1);
+
+    private readonly FakeAccountServer server = new();
+    private readonly StubKeyBinding keyBinding = new();
+    private readonly VaultKnownHostStore knownHosts = new();
+
+    private ClientCacheFactory caches = null!;
+    private TerminalWorkspace workspace = null!;
+    private VaultSession session = null!;
+    private VaultViewModel vault = null!;
+    private MainWindowViewModel shell = null!;
+
+    private static CancellationToken Token => TestContext.Current.CancellationToken;
+
+    /// <inheritdoc />
+    public async ValueTask InitializeAsync()
+    {
+        caches = ClientCacheFactory.ForMemory($"palette-{Guid.CreateVersion7():N}");
+        await caches.MigrateAsync(Token);
+
+        await new AccountProvisioner(server, keyBinding, caches, TimeProvider.System, CheapProfile)
+            .EnrollAsync(ServerUrl, Passphrase, "laptop", "Personal", Token);
+
+        var outcome = await new SessionOpener(caches, TimeProvider.System).UnlockAsync(Passphrase, Token);
+        outcome.IsUnlocked.ShouldBeTrue(outcome.Message);
+        session = outcome.Session!;
+
+        workspace = new TerminalWorkspace(
+            new InMemoryTerminalAssetProvider(new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)),
+            Substitute.For<ISshConnectionFactory>(),
+            TimeProvider.System);
+
+        await knownHosts.OpenAsync(session, Token);
+
+        vault = new VaultViewModel(session, workspace, knownHosts, static () => null);
+
+        await SeedAsync();
+
+        shell = new MainWindowViewModel(
+            ClientPaths.Default,
+            caches,
+            workspace,
+            knownHosts,
+            Substitute.For<IDeviceKeyStore>(),
+            (_, _) => throw new NotSupportedException("nothing here signs in"),
+            TimeProvider.System,
+            CheapProfile)
+        {
+            // The state the palette is only ever open in. Assigned rather than reached through the unlock
+            // path, which would be a second enrollment and a second Argon2 pass for no extra coverage.
+            State = ShellState.Unlocked,
+            Vault = vault,
+        };
+    }
+
+    /// <inheritdoc />
+    public async ValueTask DisposeAsync()
+    {
+        await shell.DisposeAsync();
+        knownHosts.Close();
+        await workspace.DisposeAsync();
+        await session.DisposeAsync();
+        caches.Dispose();
+    }
+
+    /// <remarks>
+    /// The gesture everybody tries first, and the one that did nothing at all: the wash took no pointer input,
+    /// so the only ways out of the palette were a key and the button that opened it.
+    /// </remarks>
+    [Fact]
+    public async Task APressOnTheWashClosesThePalette()
+    {
+        await OnThePaletteAsync((_, window) =>
+        {
+            // The bottom-left corner: the card is 520 wide, centred, and starts 90 pixels down, so nothing
+            // here belongs to it.
+            window.MouseDown(new Point(12, 520), MouseButton.Left);
+
+            shell.IsSearching.ShouldBeFalse();
+        });
+    }
+
+    /// <remarks>
+    /// The other half of the same rule, and the one that makes it worth a handler rather than a press anywhere
+    /// closing: a press on the card bubbles through the wash on its way out, so a handler that did not check
+    /// where the press started would close the palette the moment somebody clicked into the box.
+    /// </remarks>
+    [Fact]
+    public async Task APressOnTheCardDoesNotClose()
+    {
+        await OnThePaletteAsync((palette, window) =>
+        {
+            window.MouseDown(Centre(palette.QueryBox, window), MouseButton.Left);
+
+            shell.IsSearching.ShouldBeTrue();
+        });
+    }
+
+    [Fact]
+    public async Task EscapeClosesThePalette()
+    {
+        await OnThePaletteAsync((palette, window) =>
+        {
+            palette.QueryBox.Focus().ShouldBeTrue();
+
+            window.KeyPressQwerty(PhysicalKey.Escape, RawInputModifiers.None);
+
+            shell.IsSearching.ShouldBeFalse();
+        });
+    }
+
+    /// <remarks>
+    /// The second assertion is the whole reason the selection is moved by hand rather than by letting the list
+    /// take focus: a palette whose arrow keys moved the caret out of the query box would stop receiving the
+    /// next character typed.
+    /// </remarks>
+    [Fact]
+    public async Task TheArrowsMoveTheSelectionAndLeaveTheKeyboardInTheBox()
+    {
+        await OnThePaletteAsync((palette, window) =>
+        {
+            palette.QueryBox.Focus().ShouldBeTrue();
+
+            shell.SearchResults.Count.ShouldBeGreaterThan(2, "an empty list would prove nothing");
+            shell.SelectedSearchResult.ShouldBe(shell.SearchResults[0]);
+
+            window.KeyPressQwerty(PhysicalKey.ArrowDown, RawInputModifiers.None);
+            shell.SelectedSearchResult.ShouldBe(shell.SearchResults[1]);
+
+            window.KeyPressQwerty(PhysicalKey.ArrowUp, RawInputModifiers.None);
+            shell.SelectedSearchResult.ShouldBe(shell.SearchResults[0]);
+
+            // Clamped rather than wrapped, which is the palette's own rule.
+            window.KeyPressQwerty(PhysicalKey.ArrowUp, RawInputModifiers.None);
+            shell.SelectedSearchResult.ShouldBe(shell.SearchResults[0]);
+
+            palette.QueryBox.IsFocused.ShouldBeTrue("the arrows must not move the caret out of the box");
+        });
+    }
+
+    /// <remarks>
+    /// What Enter promises is to take the highlighted row: the palette closes and the vault is pointed at that
+    /// host. The connection it then asks for fails in this suite — there is no server and no shell — and it
+    /// fails inside the vault's own handling, which is the point of connecting through the vault's command
+    /// rather than opening a session from the palette.
+    /// </remarks>
+    [Fact]
+    public async Task EnterTakesTheHighlightedResult()
+    {
+        await OnThePaletteAsync((palette, window) =>
+        {
+            palette.QueryBox.Focus().ShouldBeTrue();
+
+            window.KeyPressQwerty(PhysicalKey.ArrowDown, RawInputModifiers.None);
+            var highlighted = shell.SelectedSearchResult.ShouldNotBeNull();
+
+            window.KeyPressQwerty(PhysicalKey.Enter, RawInputModifiers.None);
+
+            shell.IsSearching.ShouldBeFalse();
+            vault.SelectedHost?.EntityId.ShouldBe(highlighted.EntityId);
+        });
+    }
+
+    /// <remarks>
+    /// The palette is a box somebody is expected to start typing into, and for a while it was not: the window
+    /// focused it from the view model's <c>PropertyChanged</c>, which runs before the binding that reveals the
+    /// control, and <c>Focus()</c> on a collapsed control is a no-op that is never replayed. Becoming visible
+    /// is the moment that cannot be too early, so that is where the palette takes the keyboard — and this is
+    /// the test that says so.
+    /// </remarks>
+    [Fact]
+    public async Task ThePaletteTakesTheKeyboardWhenItAppears()
+    {
+        await LayoutHarness.OnTheUiThreadAsync(
+            () =>
+            {
+                var elsewhere = new TextBox();
+                var palette = new QuickConnect { DataContext = shell, IsVisible = false };
+
+                var window = new Window { Content = new Panel { Children = { elsewhere, palette } } };
+                LayoutHarness.Settle(window, 900, 600);
+
+                try
+                {
+                    elsewhere.Focus().ShouldBeTrue();
+
+                    shell.ToggleSearchCommand.Execute(null);
+                    palette.IsVisible = true;
+
+                    // The layout pass the application's dispatcher would run anyway. Without it the query box
+                    // is not in the visual tree yet, which is the whole reason the palette defers this.
+                    Dispatcher.UIThread.RunJobs();
+
+                    palette.QueryBox.IsFocused.ShouldBeTrue();
+                }
+                finally
+                {
+                    window.Close();
+                }
+            },
+            Token);
+    }
+
+    // ---- Helpers ----
+
+    /// <summary>Opens the palette in a window the size the application's is, and runs one body against it.</summary>
+    private Task OnThePaletteAsync(Action<QuickConnect, Window> body) =>
+        LayoutHarness.OnTheUiThreadAsync(
+            () =>
+            {
+                shell.ToggleSearchCommand.Execute(null);
+                shell.IsSearching.ShouldBeTrue("every case here starts with the palette open");
+
+                var palette = new QuickConnect { DataContext = shell };
+                var window = new Window { Content = palette };
+                LayoutHarness.Settle(window, 900, 600);
+
+                try
+                {
+                    body(palette, window);
+                }
+                finally
+                {
+                    window.Close();
+                }
+            },
+            Token);
+
+    private static Point Centre(Visual control, Visual window) =>
+        control.TranslatePoint(new Point(control.Bounds.Width / 2, control.Bounds.Height / 2), window)
+        ?? throw new InvalidOperationException("the control is not in this window's tree");
+
+    /// <remarks>Enough hosts that the arrow keys have somewhere to go.</remarks>
+    private async Task SeedAsync()
+    {
+        for (var i = 0; i < 6; i++)
+        {
+            vault.NewHostCommand.Execute(null);
+            vault.EditorLabel = $"host-{i}";
+            vault.EditorHostname = $"host-{i}.internal";
+            vault.EditorUsername = "deploy";
+            await vault.SaveHostCommand.ExecuteAsync(null);
+        }
+
+        await vault.LoadAsync(Token);
+    }
+}