diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs b/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs index 936a77e..312227a 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs @@ -79,9 +79,15 @@ internal sealed partial class MainWindow : Window /// /// /// - /// A tunnelled handler rather than KeyBindings, because three of these four keys have to be - /// intercepted before the control under the pointer sees them: Escape and the arrows belong to the - /// palette while it is open, and the palette's own text box would otherwise eat them. + /// Ctrl+K is here rather than on the palette because it has to work when the palette is not showing, and + /// it is a plain handler rather than a KeyBinding so that toggling stays one code path with the + /// rest of the chord set. + /// + /// + /// The palette's own keys are forwarded rather than answered: 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. /// /// /// 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); } - /// - /// 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. - /// - 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; - } - } - - /// Clamped rather than wrapped: a list that jumps from the last row to the first loses people. - 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 - // has to move the caret there — including out of the terminal, which needs the Win32 half as well. + // Closing only. Opening also has to move the keyboard — the palette is a text box somebody is + // 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; } diff --git a/src/DodoSSH.Client.App/Views/QuickConnect.axaml b/src/DodoSSH.Client.App/Views/QuickConnect.axaml index d9a07f6..c684a83 100644 --- a/src/DodoSSH.Client.App/Views/QuickConnect.axaml +++ b/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. --> - + + @@ -39,8 +45,8 @@ The Ctrl+K host search, overlaid on the window. /// -/// The keys it responds to are handled by rather than here, because two of them — -/// Ctrl+K to open and Escape to close — have to work when this control does not exist yet or has just -/// stopped existing. Handling the arrows in the same place keeps the whole chord set in one method. +/// +/// Everything a palette does once it is open belongs here rather than in : the four +/// 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. +/// +/// +/// The split used to fall the other way, and the cost was that none of it could be tested. Showing +/// initialises WebView2, which refuses the headless dispatcher's thread — see +/// LayoutHarnessTests.WhyTheWindowItselfIsNeverShown — so behaviour that lived on the window could only +/// be checked by hand. A hosts in a bare window and takes real key and pointer +/// input. +/// /// internal sealed partial class QuickConnect : UserControl { - public QuickConnect() => InitializeComponent(); + public QuickConnect() + { + InitializeComponent(); - /// The box, so the window can put the caret in it the moment the palette opens. + // 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); + } + + /// The box, so the caret can be put in it the moment the palette opens. internal TextBox QueryBox => Query; + + private MainWindowViewModel? Shell => DataContext as MainWindowViewModel; + + /// + /// Answers one of the palette's keys, wherever in the window it was pressed. + /// + /// + /// Internal because 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. + /// + 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; + } + } + + /// + /// Takes the keyboard, so the palette can be typed into the instant it appears. + /// + /// + /// + /// Both halves, for the reason 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. + /// + /// + /// Done here rather than by the window, and that is the fix rather than a tidying. Focus() 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 PropertyChanged — 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. + /// + /// + /// 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 + /// IsVisible turns true the query box reports IsAttachedToVisualTree() == false, 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. + /// + /// + 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(); + } + + /// Clamped rather than wrapped: a list that jumps from the last row to the first loses people. + 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)]; + } + + /// + 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()) + { + Dispatcher.UIThread.Post(TakeKeyboard, DispatcherPriority.Loaded); + } + } + + private void OnPaletteKey(object? sender, KeyEventArgs e) => HandleKey(e); + + /// + /// 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. + /// + 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; + } } diff --git a/tests/DodoSSH.Client.App.Layout.Tests/QuickConnectTests.cs b/tests/DodoSSH.Client.App.Layout.Tests/QuickConnectTests.cs new file mode 100644 index 0000000..916a5e3 --- /dev/null +++ b/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; + +/// +/// How the quick-connect palette answers a keyboard and a pointer. +/// +/// +/// +/// This is the suite the palette shipped without, and the reason it shipped without one is that all of this +/// used to live on MainWindow — which cannot be shown here at all, because attaching the terminal's +/// WebView initialises WebView2 on a thread it refuses. See +/// . A UserControl hosts in a bare +/// window, takes real key and pointer input, and can therefore be held to what it promises. +/// +/// +/// 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 PropertyChanged — 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. +/// +/// +/// A real 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. +/// +/// +public sealed class QuickConnectTests : IAsyncLifetime +{ + private const string Passphrase = "a sufficiently long passphrase"; + private const string ServerUrl = "https://dodossh.example"; + + /// Far below the shipped profile: nothing here attacks a wrap. + 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; + + /// + 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(StringComparer.Ordinal)), + Substitute.For(), + 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(), + (_, _) => 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, + }; + } + + /// + public async ValueTask DisposeAsync() + { + await shell.DisposeAsync(); + knownHosts.Close(); + await workspace.DisposeAsync(); + await session.DisposeAsync(); + caches.Dispose(); + } + + /// + /// 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. + /// + [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(); + }); + } + + /// + /// 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. + /// + [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(); + }); + } + + /// + /// 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. + /// + [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"); + }); + } + + /// + /// 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. + /// + [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); + }); + } + + /// + /// 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 PropertyChanged, which runs before the binding that reveals the + /// control, and Focus() 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. + /// + [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 ---- + + /// Opens the palette in a window the size the application's is, and runs one body against it. + private Task OnThePaletteAsync(Action 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"); + + /// Enough hosts that the arrow keys have somewhere to go. + 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); + } +}