From c3277b52c98989ef9c41338540ae5a4d6b0848e0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 16 Jul 2026 02:17:37 -0400 Subject: [PATCH] =?UTF-8?q?review(track0):=20async=20ContextMenu=20OnClick?= =?UTF-8?q?=20+=20keyboard-=E2=8B=AF=20anchoring=20+=20focus-return;=20dis?= =?UTF-8?q?covery-driven=20DriverTypeNames=20guard;=20CSV=20round-trip=20c?= =?UTF-8?q?omment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-0 reviewer findings (no Critical/High): - T0-1 M: ContextMenuItem.OnClick Actionβ†’Func (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 --- .../Pages/Dev/ContextMenuDemo.razor | 6 +- .../Components/Shared/ContextMenu.razor | 136 +++++++++++------- .../Components/Shared/ContextMenu.razor.css | 14 ++ .../Components/Shared/ContextMenuItem.cs | 9 +- .../Csv/CsvRoundTripTests.cs | 9 +- .../DriverTypeNamesGuardTests.cs | 76 +++++++--- 6 files changed, 169 insertions(+), 81 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Dev/ContextMenuDemo.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Dev/ContextMenuDemo.razor index db5cd1da..77791f2a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Dev/ContextMenuDemo.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Dev/ContextMenuDemo.razor @@ -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; + } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor index ea8b00fc..40a613d7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor @@ -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 @@ } -@if (ShowEllipsis) -{ - -} - @if (_open) { - @* Transparent catch-all: any click that misses the menu closes it. *@ -
+ @* Transparent catch-all: any click β€” or a right-click β€” that misses the menu closes it. *@ +
+} - + +} + +@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 => + { + + }; + 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 */ } } } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor.css b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor.css index df025623..7c8c9c7d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor.css +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenu.razor.css @@ -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; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenuItem.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenuItem.cs index a513f7f0..d790c2a0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenuItem.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/ContextMenuItem.cs @@ -11,8 +11,13 @@ public sealed class ContextMenuItem /// Text shown for the row. public string Label { get; init; } = ""; - /// Invoked when the row is activated (click / Enter). - public Action? OnClick { get; init; } + /// + /// 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 async void; the menu awaits this and re-renders. + /// A synchronous handler returns . + /// + public Func? OnClick { get; init; } /// Optional leading icon β€” an emoji or single glyph. public string? Icon { get; init; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Csv/CsvRoundTripTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Csv/CsvRoundTripTests.cs index f2521798..e6604976 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Csv/CsvRoundTripTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Csv/CsvRoundTripTests.cs @@ -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 diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverTypeNamesGuardTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverTypeNamesGuardTests.cs index d5b12aaf..64c86d23 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverTypeNamesGuardTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverTypeNamesGuardTests.cs @@ -8,34 +8,72 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests; /// /// Guards against drift from the driver factories that are the -/// authority for these strings. The test builds the real registered-factory set the way -/// production does β€” it drives each driver assembly's *DriverFactoryExtensions.Register -/// entry point into a live , exactly as the Host's -/// DriverFactoryBootstrap.Register does β€” then asserts bidirectional parity with the -/// constants. If a factory renames its DriverTypeName (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 reflection-driven, not a hand-copied list: +/// the test scans the driver assemblies deployed to its output directory for the +/// *DriverFactoryExtensions.Register(DriverFactoryRegistry, …) convention and invokes each into +/// a live β€” 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 DriverType string with no matching constant (or +/// a constant is added/removed without a matching factory), this fails, forcing the two back in sync. /// public sealed class DriverTypeNamesGuardTests { + // Driver-adjacent assemblies that ship no runtime factory (contracts/addressing/CLI helpers). + private static readonly string[] NonFactoryDriverAssemblySuffixes = + { ".Contracts", ".Addressing", ".Cli" }; + /// - /// 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 DriverFactoryBootstrap.Register on a driver-role node. + /// Build the authoritative registered-factory set by discovering every + /// *DriverFactoryExtensions.Register 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 Register call installs β€” so is passed for the + /// optional loggerFactory/other parameters. /// private static IReadOnlyCollection 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; }