review(track0): async ContextMenu OnClick + keyboard-⋯ anchoring + focus-return; discovery-driven DriverTypeNames guard; CSV round-trip comment
Wave-0 reviewer findings (no Critical/High): - T0-1 M: ContextMenuItem.OnClick Action→Func<Task> (kills async-void at menu call sites Wave A/B/C need); keyboard-activated ⋯ now anchors below the button instead of viewport (0,0); focus returns to trigger on close; backdrop closes on native right-click too. - T0-3 M: guard test now discovers *DriverFactoryExtensions.Register by convention from the deployed driver assemblies instead of a hand-copied list, so a future driver lacking a constant is actually caught. - T0-2 L: corrected the round-trip 'sole non-round-trippable shape' comment (any table whose final row is a lone empty field). Claude-Session: https://claude.ai/code/session_01LVneM3eh1UtJxEisFXgmox
This commit is contained in:
Joseph Doherty
@@ -31,5 +31,9 @@
|
||||
new() { Label = "Delete", Icon = "🗑️", Disabled = true, OnClick = () => Set("Delete") },
|
||||
};
|
||||
|
||||
private void Set(string action) => _lastAction = action;
|
||||
private Task Set(string action)
|
||||
{
|
||||
_lastAction = action;
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,10 +4,12 @@
|
||||
• ChildContent — wrapped as a right-clickable surface (browser menu suppressed).
|
||||
• ShowEllipsis — a "⋯" button, the keyboard/touch fallback for the right-click.
|
||||
|
||||
Both triggers open the same menu anchored at the pointer. Outside-click closes
|
||||
via a transparent full-viewport backdrop (the standard Blazor dropdown pattern —
|
||||
no JS alert/confirm). Keyboard: Arrow up/down move selection, Enter activates,
|
||||
Esc closes. *@
|
||||
Mouse/touch open the menu anchored at the pointer (fixed positioning). Keyboard
|
||||
activation of the "⋯" button carries no pointer coordinates, so the menu instead
|
||||
anchors just below the button (absolute positioning inside the trigger wrap).
|
||||
Outside-click (or a right-click elsewhere) closes via a transparent full-viewport
|
||||
backdrop — no JS alert/confirm. Keyboard: Arrow up/down move selection, Enter/Space
|
||||
activate, Esc closes; focus returns to the "⋯" trigger on close. *@
|
||||
|
||||
@if (ChildContent is not null)
|
||||
{
|
||||
@@ -16,44 +18,29 @@
|
||||
</span>
|
||||
}
|
||||
|
||||
@if (ShowEllipsis)
|
||||
{
|
||||
<button type="button" class="ctx-ellipsis" title="More actions"
|
||||
aria-haspopup="true" aria-expanded="@_open" @onclick="OpenAt">
|
||||
⋯@* ⋯ MIDLINE HORIZONTAL ELLIPSIS *@
|
||||
</button>
|
||||
}
|
||||
|
||||
@if (_open)
|
||||
{
|
||||
@* Transparent catch-all: any click that misses the menu closes it. *@
|
||||
<div class="ctx-backdrop" @onclick="Close"></div>
|
||||
@* Transparent catch-all: any click — or a right-click — that misses the menu closes it. *@
|
||||
<div class="ctx-backdrop" @onclick="Close" @oncontextmenu="Close" @oncontextmenu:preventDefault="true"></div>
|
||||
}
|
||||
|
||||
<div class="ctx-menu" style="top:@(_y.ToString(Culture))px; left:@(_x.ToString(Culture))px"
|
||||
role="menu" tabindex="0" @ref="_menuRef" @onkeydown="OnKeyDown">
|
||||
@for (var i = 0; i < Items.Count; i++)
|
||||
@if (ShowEllipsis)
|
||||
{
|
||||
<span class="ctx-ellipsis-wrap">
|
||||
<button type="button" class="ctx-ellipsis" title="More actions"
|
||||
aria-haspopup="true" aria-expanded="@_open" @ref="_ellipsisRef" @onclick="OpenAt">
|
||||
⋯@* ⋯ MIDLINE HORIZONTAL ELLIPSIS *@
|
||||
</button>
|
||||
@if (_open && _anchorToEllipsis)
|
||||
{
|
||||
var item = Items[i];
|
||||
if (item.IsSeparator)
|
||||
{
|
||||
<div class="ctx-separator" role="separator"></div>
|
||||
}
|
||||
else
|
||||
{
|
||||
var index = i;
|
||||
<button type="button" role="menuitem"
|
||||
class="ctx-item @(index == _activeIndex ? "active" : "")"
|
||||
disabled="@item.Disabled"
|
||||
@onclick="() => Activate(item)">
|
||||
@if (!string.IsNullOrEmpty(item.Icon))
|
||||
{
|
||||
<span class="ctx-icon">@item.Icon</span>
|
||||
}
|
||||
<span class="ctx-label">@item.Label</span>
|
||||
</button>
|
||||
}
|
||||
@RenderMenu("ctx-menu ctx-menu--anchored", null)
|
||||
}
|
||||
</div>
|
||||
</span>
|
||||
}
|
||||
|
||||
@if (_open && !_anchorToEllipsis)
|
||||
{
|
||||
@RenderMenu("ctx-menu", $"top:{_y.ToString(Culture)}px; left:{_x.ToString(Culture)}px")
|
||||
}
|
||||
|
||||
@code {
|
||||
@@ -72,35 +59,44 @@
|
||||
private bool _open;
|
||||
private double _x;
|
||||
private double _y;
|
||||
private bool _anchorToEllipsis;
|
||||
private int _activeIndex = -1;
|
||||
private ElementReference _menuRef;
|
||||
private bool _focusPending;
|
||||
private ElementReference _ellipsisRef;
|
||||
private bool _focusMenuPending;
|
||||
private bool _focusEllipsisPending;
|
||||
|
||||
// Both triggers carry a MouseEventArgs, so the menu anchors at the pointer
|
||||
// (right-click point, or the click point on the "⋯" button) with no JS.
|
||||
// Mouse right-click / touch-tap carry real pointer coordinates → anchor the menu there
|
||||
// (fixed). Keyboard activation of the "⋯" button dispatches a click with clientX/Y == 0
|
||||
// (no pointer) → anchor the menu below the button instead of at viewport origin (0,0).
|
||||
private void OpenAt(MouseEventArgs e)
|
||||
{
|
||||
_anchorToEllipsis = e.ClientX == 0 && e.ClientY == 0;
|
||||
_x = e.ClientX;
|
||||
_y = e.ClientY;
|
||||
_activeIndex = FirstEnabledIndex();
|
||||
_open = true;
|
||||
_focusPending = true;
|
||||
_focusMenuPending = true;
|
||||
}
|
||||
|
||||
private void Close()
|
||||
{
|
||||
if (!_open) return;
|
||||
var returnFocus = _anchorToEllipsis; // keyboard-opened via the ⋯ button — restore its focus
|
||||
_open = false;
|
||||
_activeIndex = -1;
|
||||
_anchorToEllipsis = false;
|
||||
if (returnFocus && ShowEllipsis) _focusEllipsisPending = true;
|
||||
}
|
||||
|
||||
private void Activate(ContextMenuItem item)
|
||||
private async Task Activate(ContextMenuItem item)
|
||||
{
|
||||
if (item.Disabled || item.IsSeparator) return;
|
||||
Close();
|
||||
item.OnClick?.Invoke();
|
||||
if (item.OnClick is not null) await item.OnClick.Invoke();
|
||||
}
|
||||
|
||||
private void OnKeyDown(KeyboardEventArgs e)
|
||||
private async Task OnKeyDown(KeyboardEventArgs e)
|
||||
{
|
||||
switch (e.Key)
|
||||
{
|
||||
@@ -117,7 +113,7 @@
|
||||
case " ":
|
||||
if (_activeIndex >= 0 && _activeIndex < Items.Count)
|
||||
{
|
||||
Activate(Items[_activeIndex]);
|
||||
await Activate(Items[_activeIndex]);
|
||||
}
|
||||
break;
|
||||
}
|
||||
@@ -150,19 +146,49 @@
|
||||
return -1;
|
||||
}
|
||||
|
||||
// The menu body, shared between the pointer-anchored (fixed) and ellipsis-anchored
|
||||
// (absolute) placements so the item loop lives in exactly one place.
|
||||
private RenderFragment RenderMenu(string cssClass, string? style) => __builder =>
|
||||
{
|
||||
<div class="@cssClass" style="@style" role="menu" tabindex="0" @ref="_menuRef" @onkeydown="OnKeyDown">
|
||||
@for (var i = 0; i < Items.Count; i++)
|
||||
{
|
||||
var item = Items[i];
|
||||
if (item.IsSeparator)
|
||||
{
|
||||
<div class="ctx-separator" role="separator"></div>
|
||||
}
|
||||
else
|
||||
{
|
||||
var index = i;
|
||||
<button type="button" role="menuitem"
|
||||
class="ctx-item @(index == _activeIndex ? "active" : "")"
|
||||
disabled="@item.Disabled"
|
||||
@onclick="() => Activate(item)">
|
||||
@if (!string.IsNullOrEmpty(item.Icon))
|
||||
{
|
||||
<span class="ctx-icon">@item.Icon</span>
|
||||
}
|
||||
<span class="ctx-label">@item.Label</span>
|
||||
</button>
|
||||
}
|
||||
}
|
||||
</div>
|
||||
};
|
||||
|
||||
protected override async Task OnAfterRenderAsync(bool firstRender)
|
||||
{
|
||||
if (_focusPending)
|
||||
if (_focusMenuPending)
|
||||
{
|
||||
_focusPending = false;
|
||||
try
|
||||
{
|
||||
await _menuRef.FocusAsync();
|
||||
}
|
||||
catch
|
||||
{
|
||||
// The menu may have closed before the focus round-trip completed.
|
||||
}
|
||||
_focusMenuPending = false;
|
||||
try { await _menuRef.FocusAsync(); }
|
||||
catch { /* the menu may have closed before the focus round-trip completed */ }
|
||||
}
|
||||
if (_focusEllipsisPending)
|
||||
{
|
||||
_focusEllipsisPending = false;
|
||||
try { await _ellipsisRef.FocusAsync(); }
|
||||
catch { /* the trigger may have unmounted */ }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,6 +5,12 @@
|
||||
display: inline;
|
||||
}
|
||||
|
||||
/* Positioned wrap so a keyboard-opened menu can anchor below the "⋯" button. */
|
||||
.ctx-ellipsis-wrap {
|
||||
position: relative;
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
/* The "⋯" fallback trigger — a compact, quiet icon button. */
|
||||
.ctx-ellipsis {
|
||||
display: inline-flex;
|
||||
@@ -50,6 +56,14 @@
|
||||
outline: none;
|
||||
}
|
||||
|
||||
/* Keyboard-opened placement: anchored just below the "⋯" trigger instead of the pointer. */
|
||||
.ctx-menu--anchored {
|
||||
position: absolute;
|
||||
top: 100%;
|
||||
left: 0;
|
||||
margin-top: 0.25rem;
|
||||
}
|
||||
|
||||
.ctx-item {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
|
||||
@@ -11,8 +11,13 @@ public sealed class ContextMenuItem
|
||||
/// <summary>Text shown for the row.</summary>
|
||||
public string Label { get; init; } = "";
|
||||
|
||||
/// <summary>Invoked when the row is activated (click / Enter).</summary>
|
||||
public Action? OnClick { get; init; }
|
||||
/// <summary>
|
||||
/// Invoked when the row is activated (click / Enter). Async so per-node menu
|
||||
/// actions (delete, redeploy, rename) can await their service calls without an
|
||||
/// exception-swallowing <c>async void</c>; the menu awaits this and re-renders.
|
||||
/// A synchronous handler returns <see cref="Task.CompletedTask"/>.
|
||||
/// </summary>
|
||||
public Func<Task>? OnClick { get; init; }
|
||||
|
||||
/// <summary>Optional leading icon — an emoji or single glyph.</summary>
|
||||
public string? Icon { get; init; }
|
||||
|
||||
@@ -16,10 +16,11 @@ public sealed class CsvRoundTripTests
|
||||
// Each case is a jagged table of fields.
|
||||
yield return new object[] { new[] { new[] { "a", "b", "c" } } };
|
||||
yield return new object[] { new[] { new[] { "a", "b" }, new[] { "c", "d" } } };
|
||||
// NOTE: a single row of a single empty field ([[""]]) is deliberately excluded — it serialises to
|
||||
// the empty string, which is indistinguishable from an empty document and parses back to zero rows.
|
||||
// That one degenerate case is the sole non-round-trippable shape; every multi-field / multi-row
|
||||
// empty case below (",," and blank interior lines) does round-trip exactly.
|
||||
// NOTE: a table whose FINAL row is a single empty field ([[""]], or e.g. [["a"],[""]]) is deliberately
|
||||
// excluded — that trailing lone-empty-field row serialises to nothing after the preceding row's
|
||||
// terminator, so the parser's no-phantom-trailing-row rule drops it on the way back. This is the only
|
||||
// non-round-trippable shape; a single-empty-field row anywhere but last (see the interior [""] case
|
||||
// below), ",,", and blank interior lines all round-trip exactly.
|
||||
yield return new object[] { new[] { new[] { "", "", "" } } };
|
||||
yield return new object[] { new[] { new[] { "a" }, new[] { "" }, new[] { "b" } } };
|
||||
yield return new object[] { new[] { new[] { "a,b", "c" } } }; // embedded delimiter
|
||||
|
||||
@@ -8,34 +8,72 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Guards <see cref="DriverTypeNames"/> against drift from the driver factories that are the
|
||||
/// authority for these strings. The test builds the <b>real</b> registered-factory set the way
|
||||
/// production does — it drives each driver assembly's <c>*DriverFactoryExtensions.Register</c>
|
||||
/// entry point into a live <see cref="DriverFactoryRegistry"/>, exactly as the Host's
|
||||
/// <c>DriverFactoryBootstrap.Register</c> does — then asserts bidirectional parity with the
|
||||
/// constants. If a factory renames its <c>DriverTypeName</c> (or a constant is added/removed
|
||||
/// without a matching factory), this fails, forcing the constant and the factory back in sync.
|
||||
/// authority for these strings. Factory discovery is <b>reflection-driven, not a hand-copied list</b>:
|
||||
/// the test scans the driver assemblies deployed to its output directory for the
|
||||
/// <c>*DriverFactoryExtensions.Register(DriverFactoryRegistry, …)</c> convention and invokes each into
|
||||
/// a live <see cref="DriverFactoryRegistry"/> — the same registration path production takes — then
|
||||
/// asserts bidirectional parity with the constants. Because discovery is by convention, a brand-new
|
||||
/// driver assembly referenced by this project (and thus copied to bin) is picked up automatically: if
|
||||
/// it registers a <c>DriverType</c> string with no matching <see cref="DriverTypeNames"/> constant (or
|
||||
/// a constant is added/removed without a matching factory), this fails, forcing the two back in sync.
|
||||
/// </summary>
|
||||
public sealed class DriverTypeNamesGuardTests
|
||||
{
|
||||
// Driver-adjacent assemblies that ship no runtime factory (contracts/addressing/CLI helpers).
|
||||
private static readonly string[] NonFactoryDriverAssemblySuffixes =
|
||||
{ ".Contracts", ".Addressing", ".Cli" };
|
||||
|
||||
/// <summary>
|
||||
/// Build the authoritative registered-factory set by invoking every driver assembly's
|
||||
/// registration extension against a fresh registry — the same set of calls, in the same
|
||||
/// spirit, as <c>DriverFactoryBootstrap.Register</c> on a driver-role node.
|
||||
/// Build the authoritative registered-factory set by discovering every
|
||||
/// <c>*DriverFactoryExtensions.Register</c> in the deployed driver assemblies (by convention) and
|
||||
/// invoking it against a fresh registry. The factory func is never invoked — we only need the
|
||||
/// type-name key each <c>Register</c> call installs — so <see langword="null"/> is passed for the
|
||||
/// optional <c>loggerFactory</c>/other parameters.
|
||||
/// </summary>
|
||||
private static IReadOnlyCollection<string> RegisteredDriverTypeNames()
|
||||
{
|
||||
var registry = new DriverFactoryRegistry();
|
||||
var invoked = 0;
|
||||
|
||||
// loggerFactory is optional on every extension; the factory func is never invoked here,
|
||||
// so null is fine — we only need the type-name key each Register call installs.
|
||||
Driver.AbCip.AbCipDriverFactoryExtensions.Register(registry);
|
||||
Driver.AbLegacy.AbLegacyDriverFactoryExtensions.Register(registry, loggerFactory: null);
|
||||
Driver.FOCAS.FocasDriverFactoryExtensions.Register(registry);
|
||||
Driver.Galaxy.GalaxyDriverFactoryExtensions.Register(registry, loggerFactory: null);
|
||||
Driver.Modbus.ModbusDriverFactoryExtensions.Register(registry, loggerFactory: null);
|
||||
Driver.OpcUaClient.OpcUaClientDriverFactoryExtensions.Register(registry, loggerFactory: null);
|
||||
Driver.S7.S7DriverFactoryExtensions.Register(registry);
|
||||
Driver.TwinCAT.TwinCATDriverFactoryExtensions.Register(registry);
|
||||
var binDir = Path.GetDirectoryName(typeof(DriverTypeNamesGuardTests).Assembly.Location)!;
|
||||
foreach (var dll in Directory.GetFiles(binDir, "ZB.MOM.WW.OtOpcUa.Driver.*.dll"))
|
||||
{
|
||||
var name = Path.GetFileNameWithoutExtension(dll);
|
||||
if (NonFactoryDriverAssemblySuffixes.Any(s => name.EndsWith(s, StringComparison.Ordinal)))
|
||||
continue;
|
||||
|
||||
Assembly asm;
|
||||
try { asm = Assembly.LoadFrom(dll); }
|
||||
catch { continue; }
|
||||
|
||||
Type[] types;
|
||||
try { types = asm.GetTypes(); }
|
||||
catch (ReflectionTypeLoadException ex) { types = ex.Types.Where(t => t is not null).ToArray()!; }
|
||||
|
||||
foreach (var type in types)
|
||||
{
|
||||
if (type is null || !type.IsClass || type.Name is not { } n
|
||||
|| !n.EndsWith("DriverFactoryExtensions", StringComparison.Ordinal))
|
||||
continue;
|
||||
|
||||
var register = type.GetMethod("Register", BindingFlags.Public | BindingFlags.Static);
|
||||
var ps = register?.GetParameters();
|
||||
if (register is null || ps is null || ps.Length == 0
|
||||
|| ps[0].ParameterType != typeof(DriverFactoryRegistry))
|
||||
continue;
|
||||
|
||||
var args = new object?[ps.Length];
|
||||
args[0] = registry; // remaining params (optional loggerFactory, etc.) default to null
|
||||
register.Invoke(null, args);
|
||||
invoked++;
|
||||
}
|
||||
}
|
||||
|
||||
// Discovery must find *something*; a zero here means the bin scan/convention broke, not that the
|
||||
// fleet has no drivers — fail loudly rather than pass a vacuously-empty parity check.
|
||||
invoked.ShouldBeGreaterThan(0,
|
||||
"no *DriverFactoryExtensions.Register was discovered in the output directory — the driver " +
|
||||
"assemblies did not deploy to bin, or the naming convention changed");
|
||||
|
||||
return registry.RegisteredTypes;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user