From a33a2f44569005c7dfccca2c11b6f635718416a5 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Tue, 8 Sep 2026 10:00:17 -0400
Subject: [PATCH] =?UTF-8?q?fix(central-ui):=20InstancePicker=20=E2=80=94?=
 =?UTF-8?q?=20keyboard=20activation=20selects/toggles=20through=20TreeView?=
 =?UTF-8?q?=20selection;=20aria-label/InputId;=20aria-controls=20only=20wh?=
 =?UTF-8?q?ile=20open?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

ArrowDown moved focus into the tree, but the picker passed no Selectable /
SelectedKeyChanged, so Enter/Space on a treeitem only moved the roving tabindex —
the pick lived on the label span's @onclick, i.e. mouse only. The tree now runs in
Selectable mode and BOTH gestures arrive at one handler (OnTreeKeyActivatedAsync)
through TreeView's SelectedKeyChanged: an `i:{id}` key selects that instance, an
`a:{id}` key toggles the branch. The label's own @onclick is gone — the click
already bubbles into TreeView's content span, so a second handler there would fire
the callback twice. SelectedKey stays unbound (a combobox shows its pick in the
field, not as a row highlight, and binding it would arm TreeView's
selected-key-not-in-tree reset on every render while the pick is filtered out);
SelectedCssClass is emptied to match.

a11y: the field gains an accessible name via a new AriaLabel parameter (default
"Instance") and an optional InputId so a page can point a <label for> at it;
aria-controls is rendered only while the panel exists, instead of always pointing
at an id that is absent when closed.

Also drops the unused @using of Commons.Entities.Instances from the .razor.

Tests: keyboard activation on an instance row and on an area row, a
one-selection-per-gesture guard across mouse and keyboard, and the labelling
assertions. CentralUI suite 1331 passed / 0 failed; build 0 warnings.

Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
---
 .../Components/Shared/InstancePicker.razor    |  26 +++--
 .../Components/Shared/InstancePicker.razor.cs |  54 ++++++++-
 .../Shared/InstancePickerTests.cs             | 106 ++++++++++++++++++
 3 files changed, 175 insertions(+), 11 deletions(-)

diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor
index 15f9ac8d..c2d66c62 100644
--- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor
+++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor
@@ -2,7 +2,9 @@
     Instance Picker — a combobox whose dropdown is the site's area tree (design
     2026-09-08). The field shows the selected instance; the panel hosts the shared
     TreeView over InstanceTreeBuilder's forest with every area collapsed. Clicking an
-    area row toggles it, clicking an instance row selects it and closes the panel.
+    area row toggles it, activating an instance row selects it and closes the panel —
+    and "activating" is one path for both input modes, because the tree runs in
+    Selectable mode and the mouse click reaches it through TreeView's own content span.
     Typing filters the forest to the matching instances and their ancestors; Enter picks
     the first visible match, ArrowDown hands keyboard navigation to the tree and Escape
     closes without changing the selection.
@@ -10,14 +12,13 @@
     an outside click, the same pattern TreeView uses for its context menu.
 *@
 @namespace ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared
-@using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances
 
 <div class="ip-wrap position-relative" data-test="@($"{DataTestPrefix}-picker")">
     @* ip-field lifts the field above the open panel's backdrop — see the .razor.css note. *@
     <div class="input-group input-group-sm ip-field">
         <input type="text" class="form-control" role="combobox" autocomplete="off"
-               id="@_inputId" aria-autocomplete="list" aria-haspopup="tree"
-               aria-expanded="@(_open ? "true" : "false")" aria-controls="@_panelId"
+               id="@_inputId" aria-autocomplete="list" aria-haspopup="tree" aria-label="@AriaLabel"
+               aria-expanded="@(_open ? "true" : "false")" aria-controls="@(_open ? _panelId : null)"
                placeholder="@Placeholder" disabled="@Disabled"
                data-test="@($"{DataTestPrefix}-input")"
                value="@_text" @onfocus="Open" @oninput="OnInput" @onkeydown="OnInputKeyDown" />
@@ -30,14 +31,25 @@
     {
         <div class="ip-backdrop" data-test="@($"{DataTestPrefix}-backdrop")" @onclick="Close"></div>
         <div class="ip-panel border rounded bg-body shadow-sm" id="@_panelId" data-test="@($"{DataTestPrefix}-panel")">
+            @* Selectable is what makes the tree's own Enter/Space activation reach us: it
+               routes BOTH a click on TreeView's content span and a key press on the
+               treeitem through SelectedKeyChanged, so mouse and keyboard share one path
+               (the row label below therefore carries no @onclick of its own — a second
+               handler there would fire twice on a click, since the click bubbles into the
+               content span). SelectedKey is deliberately left unbound: a combobox shows
+               its pick in the field, not as a row highlight, and binding it would also arm
+               TreeView's "selected key no longer in the tree" reset — which fires on every
+               render while the pick is filtered out, and this component would never clear
+               it. SelectedCssClass is emptied to match; with no SelectedKey nothing is ever
+               marked selected, so it is belt-and-braces. *@
             <TreeView @ref="_tree" TItem="InstanceTreeNode" Items="_roots"
                       ChildrenSelector="n => n.Children" HasChildrenSelector="n => n.HasChildren"
-                      KeySelector="n => n.Key" IndentPx="16">
+                      KeySelector="n => n.Key" IndentPx="16"
+                      Selectable="true" SelectedCssClass="" SelectedKeyChanged="OnTreeKeyActivatedAsync">
                 <NodeContent Context="node">
                     <span class="tv-glyph"><i class="bi @(node.IsInstance ? "bi-box" : "bi-diagram-3")"></i></span>
                     <span class="tv-label @(node.IsInstance ? "ip-instance" : "ip-area")" title="@node.Label"
-                          data-test="@($"{DataTestPrefix}-row")" data-instance-id="@(node.Instance?.Id)"
-                          @onclick="() => OnRowClickAsync(node)">@RowLabel(node)</span>
+                          data-test="@($"{DataTestPrefix}-row")" data-instance-id="@(node.Instance?.Id)">@RowLabel(node)</span>
                 </NodeContent>
                 <EmptyContent><span class="text-muted fst-italic small px-2">@(_hasFilter ? "No instances match." : "No instances at this site.")</span></EmptyContent>
             </TreeView>
diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor.cs
index 198d9cff..f9303110 100644
--- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/InstancePicker.razor.cs
@@ -27,6 +27,13 @@ namespace ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared;
 /// </para>
 ///
 /// <para>
+/// Inside the tree, <c>Enter</c>/<c>Space</c> on the focused row does exactly what a click on
+/// it does — picks an instance, toggles an area — because the tree runs in
+/// <c>Selectable</c> mode and both gestures arrive at
+/// <see cref="OnTreeKeyActivatedAsync"/> through TreeView's single selection callback.
+/// </para>
+///
+/// <para>
 /// A parent that swaps <see cref="Areas"/>/<see cref="Instances"/> (a site change)
 /// closes the panel and re-derives the field text from the current selection, clearing
 /// it when the selected instance is not in the new list. A parent that changes
@@ -86,6 +93,22 @@ public partial class InstancePicker
     [Parameter]
     public string Placeholder { get; set; } = "Search or browse instances…";
 
+    /// <summary>
+    /// The combobox's accessible name, rendered as <c>aria-label</c> on the field. A page
+    /// that renders its own visible <c>&lt;label for&gt;</c> should point it at
+    /// <see cref="InputId"/>; the label wins for a screen reader either way, so this stays
+    /// as the fallback name rather than being suppressed.
+    /// </summary>
+    [Parameter]
+    public string AriaLabel { get; set; } = "Instance";
+
+    /// <summary>
+    /// DOM id for the text field, so a page can address it from a <c>&lt;label for&gt;</c>.
+    /// Unset means the per-instance generated id — two pickers on one page never collide.
+    /// </summary>
+    [Parameter]
+    public string? InputId { get; set; }
+
     private readonly string _idSuffix = Guid.NewGuid().ToString("N")[..8];
 
     private bool _open;
@@ -126,7 +149,7 @@ public partial class InstancePicker
     private int _lastSelectedId;
     private bool _parametersSeen;
 
-    private string _inputId => $"ip-{_idSuffix}-input";
+    private string _inputId => InputId is { Length: > 0 } id ? id : $"ip-{_idSuffix}-input";
 
     private string _panelId => $"ip-{_idSuffix}-panel";
 
@@ -309,11 +332,34 @@ public partial class InstancePicker
         return null;
     }
 
-    private Task OnRowClickAsync(InstanceTreeNode node)
+    /// <summary>
+    /// The single activation path for a tree row, reached from TreeView's
+    /// <c>SelectedKeyChanged</c> — which a mouse click on the row and an
+    /// <c>Enter</c>/<c>Space</c> on the focused treeitem both run through, so the two input
+    /// modes can never diverge. An instance key picks that instance; an area key toggles the
+    /// branch (there is nothing to select on an area, and a keyboard user who cannot open a
+    /// branch cannot reach the instances inside it).
+    ///
+    /// <para>
+    /// Keys are the ones <see cref="InstanceTreeBuilder"/> mints: <c>i:{instanceId}</c> and
+    /// <c>a:{areaId}</c>. Anything else — including the null TreeView raises when it clears a
+    /// selection — is ignored rather than guessed at.
+    /// </para>
+    /// </summary>
+    private Task OnTreeKeyActivatedAsync(object? key)
     {
-        if (node.Instance is { } instance) return SelectAsync(instance);
+        if (Disabled || key?.ToString() is not { Length: > 0 } keyStr) return Task.CompletedTask;
+
+        if (keyStr.StartsWith("i:", StringComparison.Ordinal))
+        {
+            return int.TryParse(keyStr.AsSpan(2), out var instanceId)
+                && Instances?.FirstOrDefault(i => i.Id == instanceId) is { } instance
+                    ? SelectAsync(instance)
+                    : Task.CompletedTask;
+        }
+
+        if (keyStr.StartsWith("a:", StringComparison.Ordinal)) _tree?.ToggleExpanded(keyStr);
 
-        _tree?.ToggleExpanded(node.Key);
         return Task.CompletedTask;
     }
 
diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/InstancePickerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/InstancePickerTests.cs
index 07dec031..d67a77df 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/InstancePickerTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/InstancePickerTests.cs
@@ -65,6 +65,10 @@ public class InstancePickerTests : BunitContext
     private static string[] Rows(IRenderedComponent<InstancePicker> cut) =>
         cut.FindAll("[data-test='t-row']").Select(r => r.TextContent.Trim()).ToArray();
 
+    /// <summary>The nth treeitem in document order - i.e. the nth row the operator can see.</summary>
+    private static AngleSharp.Dom.IElement TreeItem(IRenderedComponent<InstancePicker> cut, int index) =>
+        cut.FindAll("[role='treeitem']")[index];
+
     [Fact]
     public void Closed_ShowsNothingButTheField()
     {
@@ -450,4 +454,106 @@ public class InstancePickerTests : BunitContext
         Assert.Single(cut.FindAll("[data-test='t-panel']"));
         Assert.Equal(new[] { "Zulu" }, Rows(cut));
     }
+
+    // -- Keyboard activation on the tree itself --------------------------------
+
+    [Fact]
+    public void KeyboardActivation_OnATreeRow_SelectsTheInstance()
+    {
+        int? picked = null;
+        var cut = RenderPicker(onChanged: id => picked = id);
+
+        Open(cut);
+        Press(cut, "ArrowDown");
+
+        // Areas render before root instances, so Zulu is the SECOND treeitem - the first
+        // one is the Line1 area (see KeyboardActivation_OnAnAreaRow_TogglesIt).
+        TreeItem(cut, 1).KeyDown(new KeyboardEventArgs { Key = "Enter" });
+
+        Assert.Equal(8, picked);
+        Assert.Empty(cut.FindAll("[data-test='t-panel']"));
+        Assert.Equal("Zulu", InputValue(cut));
+    }
+
+    [Fact]
+    public void KeyboardActivation_OnAnAreaRow_TogglesIt()
+    {
+        int? picked = null;
+        var cut = RenderPicker(onChanged: id => picked = id);
+
+        Open(cut);
+        Press(cut, "ArrowDown");
+
+        TreeItem(cut, 0).KeyDown(new KeyboardEventArgs { Key = "Enter" });
+
+        // An area activates as an expand, not as a pick: its children appear and the
+        // panel stays open with no callback raised.
+        Assert.Contains("Cell A", Rows(cut));
+        Assert.Single(cut.FindAll("[data-test='t-panel']"));
+        Assert.Null(picked);
+
+        // The same key on the same row closes it again - activation is a toggle.
+        TreeItem(cut, 0).KeyDown(new KeyboardEventArgs { Key = "Enter" });
+        Assert.DoesNotContain("Cell A", Rows(cut));
+    }
+
+    [Fact]
+    public void KeyboardActivation_AndMouseClick_RaiseExactlyOneSelection()
+    {
+        // The label click and Enter both run through TreeView's single selection path,
+        // so neither input mode may double-fire the callback.
+        var picks = new List<int>();
+        var cut = RenderPicker(onChanged: picks.Add);
+
+        Open(cut);
+        ClickRow(cut, "Zulu");
+        Assert.Equal(new[] { 8 }, picks);
+
+        Open(cut);
+        TreeItem(cut, 1).KeyDown(new KeyboardEventArgs { Key = "Enter" });
+        Assert.Equal(new[] { 8, 8 }, picks);
+    }
+
+    // -- Accessible name / labelling -------------------------------------------
+
+    [Fact]
+    public void Input_CarriesAnAccessibleName_ByDefaultAndFromTheParameter()
+    {
+        var cut = RenderPicker();
+        Assert.Equal("Instance", cut.Find("[data-test='t-input']").GetAttribute("aria-label"));
+
+        cut.Render(ps => ps.Add(p => p.AriaLabel, "Target instance"));
+        Assert.Equal("Target instance", cut.Find("[data-test='t-input']").GetAttribute("aria-label"));
+    }
+
+    [Fact]
+    public void InputId_DefaultsToAGeneratedId_AndHonoursTheParameter()
+    {
+        // A page that wants its own <label for> supplies the id; two pickers left to the
+        // generated one must still not collide.
+        var first = RenderPicker();
+        var second = RenderPicker();
+
+        var generated = first.Find("[data-test='t-input']").Id;
+        Assert.False(string.IsNullOrEmpty(generated));
+        Assert.NotEqual(generated, second.Find("[data-test='t-input']").Id);
+
+        first.Render(ps => ps.Add(p => p.InputId, "flow-instance"));
+        Assert.Equal("flow-instance", first.Find("[data-test='t-input']").Id);
+    }
+
+    [Fact]
+    public void AriaControls_IsRenderedOnlyWhileThePanelIsOpen()
+    {
+        var cut = RenderPicker();
+
+        // Closed: the panel element does not exist, so a dangling aria-controls would
+        // point screen readers at nothing.
+        Assert.False(cut.Find("[data-test='t-input']").HasAttribute("aria-controls"));
+
+        Open(cut);
+        Assert.Equal(
+            cut.Find("[data-test='t-panel']").Id,
+            cut.Find("[data-test='t-input']").GetAttribute("aria-controls"));
+    }
 }