Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/App/Backlog.Desktop.UI/Shell/Home.razor
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@
tabindex="-1"
@ref="_surfaceElement"
@onkeydown="OnSurfaceKeyDown"
data-escape-closes
aria-label="System tools">
<ToolsPane OnClose="CloseSurface" />
</main>
Expand All @@ -583,6 +584,7 @@
tabindex="-1"
@ref="_surfaceElement"
@onkeydown="OnSurfaceKeyDown"
data-escape-closes
aria-label="Dashboard">
<DashboardPane OnClose="CloseSurface"
RepositoryAliases="@State.SelectedRepositoryAliases"
Expand All @@ -597,6 +599,7 @@
tabindex="-1"
@ref="_surfaceElement"
@onkeydown="OnSurfaceKeyDown"
data-escape-closes
aria-label="Sessions">
<SessionsPane OnClose="CloseSurface"
RepositoryScope="@State.SelectedRepositoryAliases"
Expand All @@ -615,6 +618,7 @@
tabindex="-1"
@ref="_surfaceElement"
@onkeydown="OnSurfaceKeyDown"
data-escape-closes
aria-label="Pull requests">
<PullRequestsPane OnClose="CloseSurface"
RepositoryScope="@State.SelectedRepositoryAliases"
Expand Down Expand Up @@ -787,6 +791,16 @@
{
// Without the interop the separator still resizes by keyboard.
}

try
{
await JS.InvokeVoidAsync("backlogTakeoverEscape.register", _selfReference);
}
catch (Exception)
{
// Without it Escape still closes a surface from inside the surface;
// only the press after focus fell to <body> goes unheard.
}
}

// The Inbox loads the first time its pane is on screen rather than when
Expand Down Expand Up @@ -1277,6 +1291,21 @@
if (e.Key == "Escape") CloseSurface();
}

/// <summary>The same Escape, heard at the document because focus is on
/// <c>&lt;body&gt;</c> — where a focused control that went disabled, such as a
/// Refresh button while its read runs, leaves it — and so never reached the
/// surface's own handler. The script checks a closing surface is open; this
/// checks again, because the render may have moved on since the key.</summary>
[JSInvokable]
public Task CloseSurfaceOnEscapeAsync()
{
if (WorkspaceVisible || RoadmapVisible) return Task.CompletedTask;

CloseSurface();
StateHasChanged();
return Task.CompletedTask;
}

private async Task AskAiAsync()
{
if (EffectiveAiScope is not { } scope || !_aiSources.TryGetValue(scope, out var source)) return;
Expand Down Expand Up @@ -1992,6 +2021,7 @@
// before rendering the incoming one, so it does not — stated here so
// that a change to that ordering is read as breaking this.
await JS.InvokeVoidAsync("backlogPaneResizer.dispose");
await JS.InvokeVoidAsync("backlogTakeoverEscape.unregister");
}
catch (JSException)
{
Expand Down
32 changes: 32 additions & 0 deletions src/App/Backlog.Desktop.UI/wwwroot/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,38 @@ window.backlogDiagrams.renderDevbookAtlas = async (element, id, graph, dotnet) =
};


// Escape on a takeover whose focus has fallen to <body>. Each surface's <main>
// closes on Escape through its own @onkeydown, which hears only the keys pressed
// inside it. A focused control that becomes disabled — a Refresh button while
// its read runs — drops focus to <body>, outside the Blazor root, and the next
// Escape went nowhere. This catches exactly that press and nothing else:
//
// - only with focus on <body>, so a key pressed inside the surface stays the
// surface's own handler's, and one pressed in the header stays the header's;
// - only when a surface that closes on Escape is open — the Roadmap carries no
// `data-escape-closes`, because Escape means something else there;
// - never when something already took the key (defaultPrevented) or a modal is
// open, since Escape belongs to the dialog first.
window.backlogTakeoverEscape = (() => {
let owner = null;

document.addEventListener('keydown', (event) => {
if (event.key !== 'Escape' || event.defaultPrevented || !owner) return;
if (event.target !== document.body && event.target !== document.documentElement) return;
if (!document.querySelector('main[data-escape-closes]')) return;
if (document.querySelector('[aria-modal="true"], dialog[open]')) return;

event.preventDefault();
owner.invokeMethodAsync('CloseSurfaceOnEscapeAsync');
});

// Keyless, like the shell's resizer registration: there is one shell.
return {
register: (dotnet) => { owner = dotnet; },
unregister: () => { owner = null; }
};
})();

// An attached image is evidence for a bug report, not a photograph: it is
// committed to the repository and embedded in the issue, so it is held to a
// budget rather than sent at whatever size it arrived. Named because two
Expand Down
71 changes: 71 additions & 0 deletions tests/Backlog.Desktop.UI.UnitTests/HomeWorkspaceSurfaceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1667,6 +1667,77 @@ public async Task Opening_a_session_from_the_pull_requests_list_shows_the_sessio
});
}

/// <summary>
/// Escape closes a takeover even after focus has left it for <c>&lt;body&gt;</c>.
/// <para>
/// A focused Refresh button that goes disabled while its read runs drops focus
/// to the body, outside the surface's <c>@onkeydown</c>, and the surface stayed
/// open on Escape. The shell now registers with a document listener that hears
/// that press and calls back; bUnit has no document, so the callback is invoked
/// the way the script invokes it, and the surface marker the script looks for
/// is asserted beside it.
/// </para>
/// </summary>
[Theory]
[InlineData("tools-toggle-button", "tools-surface")]
[InlineData("dashboard-toggle-button", "dashboard-surface")]
[InlineData("sessions-toggle-button", "sessions-surface")]
[InlineData("pull-requests-toggle-button", "pull-requests-surface")]
public async Task Escape_heard_at_the_document_closes_the_takeover(string toggle, string surface)
{
using var harness = CreateHarness();
var component = Render(harness);

component.WaitForAssertion(() =>
Assert.Single(
harness.Context.JSInterop.Invocations["backlogTakeoverEscape.register"],
invocation => invocation.Arguments.Count == 1));

component.WaitForAssertion(() => Assert.NotEmpty(component.FindAll($"[data-testid='{toggle}']")));
component.Find($"[data-testid='{toggle}']").Click();
component.WaitForAssertion(() =>
Assert.True(component.Find($"[data-testid='{surface}']").HasAttribute("data-escape-closes")));

await component.InvokeAsync(component.Instance.CloseSurfaceOnEscapeAsync);

component.WaitForAssertion(() =>
{
Assert.Empty(component.FindAll($"[data-testid='{surface}']"));
Assert.NotEmpty(component.FindAll("[data-testid='workspace']"));
});
}

/// <summary>The Roadmap keeps Escape for its own grabbed bars and dialogs, so it
/// carries no marker for the document listener and the callback leaves it open.</summary>
[Fact]
public async Task Escape_heard_at_the_document_leaves_the_roadmap_open()
{
using var harness = CreateHarness();
var component = Render(harness);

OpenTheRoadmap(component);

Assert.False(component.Find("[data-testid='roadmap-surface']").HasAttribute("data-escape-closes"));

await component.InvokeAsync(component.Instance.CloseSurfaceOnEscapeAsync);

Assert.NotEmpty(component.FindAll("[data-testid='roadmap-band']"));
}

[Fact]
public async Task Disposing_the_shell_unregisters_its_escape_listener()
{
using var harness = CreateHarness();
var component = Render(harness);

component.WaitForAssertion(() =>
Assert.NotEmpty(harness.Context.JSInterop.Invocations["backlogTakeoverEscape.register"]));

await harness.Context.DisposeAsync();

Assert.Single(harness.Context.JSInterop.Invocations["backlogTakeoverEscape.unregister"]);
}

/// <summary>Presses the Pull requests segment and waits for the list to take the
/// screen.</summary>
private static void OpenPullRequests(IRenderedComponent<Home> component)
Expand Down
Loading