Share one ModalOverlay component across every modal-task panel #137
Loading…
Reference in a new issue
No description provided.
Delete branch "task-193-shared-overlay"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 193. Two new components in
Overlays/:OverlayBackdrop— the one dimming/blurring/fading scrim every overlay uses. Always mounted, driven byVisible/Inert(kept separate:Drawer's own scrim has to stay visually dimmed but unclickable while a panel elsewhere holds the focus trap, without fading itself out). Fade is a plain CSStransition, soprefers-reduced-motionis handled in CSS alone, no C# timing needed.ModalOverlay— the shell every modal-task panel floats inside: nestsOverlayBackdrop, owns the fade/slide animation, the close button and its focus,inertwhile closing, andOverlayFocusTrap's enter/exit. It does not own the close decision —OnDismissRequestedasks the host, which alone knows whether a dirty check or delete confirmation has to answer first.ChildContentisRenderFragment<bool>; itscontextistruewhile closing, so a host gates its own content the same wayPlaceFormPanelalready had to for PR #117's duplicate-id fix — nowGroupFormPanelandGroupMembersPanelget the same gate too.PlaceFormPanel,GroupFormPanel,GroupMembersPanelall adopt it; each panel's own backdrop/animation/close-button markup, CSS andIAsyncDisposableboilerplate is deleted, not left beside it.Drawer's own narrow-viewport scrim usesOverlayBackdropdirectly — always mounted now instead of@if (IsOpen), so it fades both ways.Adoption is proved by
FindComponents<ModalOverlay>().ShouldHaveSingleItem()andrenderedPanel.Nodes.OfType<IElement>().Count().ShouldBe(2)in each panel's own test class — the first catches a bespoke reimplementation, the second catches a panel keeping its own bespoke backdrop alongsideModalOverlay(found wanting in review: presence alone doesn't prove exclusivity). Both mutation-tested independently: hand-rolled markup instead ofModalOverlayreddens the first; a stray<div class="…-backdrop">next to a genuineModalOverlayreddens the second, with the first still green.ADR-0116, allocated. Partially supersedes ADR-0102 for the three paragraphs whose implementation-location claims moved; the rest of ADR-0102 stands. Rebased onto main to pick up #136 (task 195's colour work) — no conflicts beyond the generated ADR index, resolved mechanically.
Review round 1 correction: the original PR body claimed
HomeAndDrawerSharedGroupStateTests's ownWaitForAssertion(() => Click())retried the click — it doesn't; the reviewer instrumented it and found it fires exactly once. What actually helps isWaitForAssertionsettling Blazor's pending render queue first. The real mechanism, found by the same review:OverlayFocusTrap.ExitAsyncupdates its own state synchronously, but Blazor calls a removed component'sDisposeAsync— and so this call — asynchronously, after the render that removed it. For a render or two after any modal-task panel closes, the wholeDraweris brieflyinert, not only the panel that closed — already true ofPlaceFormPanelalone before this PR, never exercised until this PR gaveGroupFormPanelthe same delay. Not fixed here: a same-tick fix attempted in review (releasing the trap fromModalOverlay.CloseAsyncitself, beforeOnClose) reproducibly left a newly opened panel's ownModalOverlaydisposed again within the same render pass — a worse, less-understood failure than the one it replaced, and clear evidence the deterministic fix needs more room than this PR should spend on it under review. Documented precisely in ADR-0116's own Consequences and in the test's own comment; the test still usesWaitForAssertionto work around the symptom.A follow-up ticket for the
OverlayFocusTraprelease-timing gap needs raising — I don't have a Vikunja task-creation tool in this environment (checked; onlyget_task/list_tasks/list_comments/add_commentavailable), so this is a blocker for whoever does: title something like "OverlayFocusTrap release lags Blazor's own DisposeAsync scheduling, leaving Drawer briefly inert after any modal-task panel closes" — full mechanism and the failed same-tick fix attempt are in ADR-0116's Consequences section and this PR's own commit1abc741.Full suite (post-merge, E2E's 6 skips folded in): 1808 total / 1798 passing / 10 skipped.
dotnet format --verify-no-changesclean, pinned SDK.Verdict: changes needed
Reviewed at head
4fa8d17. CI run #583, status success, 4m50s — long enough, and gated correctly enough (e2eneedsbuild), that I take this as the isolatede2ejob having actually run and passed, not justbuild; no per-job breakdown is retrievable to confirm harder than that. Locally reproduced: cleanReleasebuild,dotnet format --verify-no-changesclean, full suite 1779/1769/10 exactly matching the PR body, and the coverage ratchet holds (Infrastructure back to 99%, WebUI up to 90.1%).The flaky-test framing, interrogated by testing rather than reading. Confirmed by history that
GroupFormPanel.CloseAsynconmainhad no delay at all (await OnClose.InvokeAsync(), nothing else) — this PR is what gives it one, so "exposed by the new shared closing delay" is doing real work, not softening a regression. But two things in the fix itself don't hold up:WaitForAssertion(() => Find(...).Click())call: it invokes the click exactly once, every time, across 40 runs —Click()doesn't throw on a no-op, so there's nothing forWaitForAssertionto retry. What actually makes this pass is thatWaitForAssertionsettles pending renders before that one attempt runs, which happens to giveModalOverlay's disposal time to complete first. Reverting only this line (keeping the panel-removal wait) reproduces a deterministic failure, not a flaky one —#place-form-namenever found, every one of 25 runs, because the click lands while<aside>is stillinert. Please reword the comment to say what's actually happening; "retried" will send the next person who touches this looking for a retry loop that isn't there.OverlayFocusTrap.ExitAsyncupdates_openCallers/firesChangedsynchronously, before its own JS call — so the lag isn't in that class. It's that Blazor calls a removed component'sDisposeAsync(and henceExitAsync) asynchronously, after the render that removed it, not as part of it. That means the whole drawer — not just the closing panel — is brieflyinertand unclickable after any modal-task panel closes, and this was already true ofPlaceFormPanelalone before this PR; it's just never been exercised by a "close one panel, immediately act outside it" sequence until this PR gaveGroupFormPanelthe same delay. Worth a line in the ADR or a code comment naming it, since it's a real, if narrow, structural property ofOverlayFocusTraprather than something scoped to the test that happened to catch it here.Adoption test proves presence, not exclusivity. Mutation-tested both directions: pointing a panel at hand-rolled markup reddens
FindComponents<ModalOverlay>().ShouldHaveSingleItem()as claimed. But I also added a bespoke<div class="...-backdrop">back in alongside<ModalOverlay>inGroupFormPanel.razorand the test still passed — it countsModalOverlayinstances, not the absence of anything else. The PR body's "not a CSS/text pattern a reimplementation could coincidentally match" oversells this one specific way. Not blocking on its own, but the claim should be narrowed or the test's own doc comment should say what it doesn't check, the way this project already does elsewhere (ADR-0066's "best-effort" framing) rather than implying a completeness it doesn't have.Everything else checks out.
OverlayBackdrop.razor.css:backdrop-filter: blur(var(--backdrop-blur))with--backdrop-blur: 4pxnewly declared,background-colordims,transition: opacityapplies on both the add and remove of.overlay-backdrop-visibleso the fade genuinely runs both ways, andprefers-reduced-motionsetstransition: none(removed outright, not shortened) — same pattern onModalOverlay.razor.css's own keyframes.PlaceDetailPanelisn't in the diff at all and still calls noOverlayFocusTrap/ModalOverlay/OverlayBackdropmethod — ADR-0099's scope holds.Drawer's scrim carriesHideAboveWideViewport="true", same condition as before, so desktop stays undimmed and the map stays reachable.PlaceFormPanel.RequestCloseAsyncis still the one thing the close button, backdrop (viaOnDismissRequested) and Escape (viaHandlePanelKeyDownAsync) all route through, so the dirty-check question is still asked once, the same way, from all three. ADR-0116/ADR-0102 annotate each other correctly in both directions, scoped to exactly the three relocated paragraphs — the rest of ADR-0102 is left standing, honestly.Verdict: mergeable
Reviewed at head
66fdd7a(superseding my "changes needed" against4fa8d17;1abc741is the pre-merge parent of this head, not orphaned, but I re-reviewed the final merged tree regardless). CI run #588, status success, 7m4s, commit SHA matches exactly. Locally reproduced: cleanReleasebuild,dotnet format --verify-no-changesclean, full suite 1808/1798/10 exactly matching the PR's own count, coverage ratchet holds.1. Race explanation — true, checked against my own instrumentation.
OverlayFocusTrap.ExitAsyncdoes update_openCallers/fireChangedsynchronously before its own JS call, confirmed by re-reading the class; the lag is genuinely Blazor calling a removed component'sDisposeAsyncasynchronously, after the render that removed it. Stated as a live entry under## Consequencesin ADR-0116, not a footnote, with its own bold headline.2. The same-tick fix claim — true, I tried it myself, twice. Released the trap from
ModalOverlay.CloseAsyncin two placements (immediately after_closing = true, and immediately beforeOnClose.InvokeAsync()). Both times: the drawer's owninertgenuinely clears (confirmed directly —GetAttribute("inert")returnsnullafter the click), so that half of the claim is real. But the test still fails deterministically (25/25), and differently:.modal-overlay-panelcount is 0, not 1 — the newly-opened panel never survives, not merely a still-inert control. That is the "worse and less-understood failure" ADR-0116 describes, not a restatement of the original symptom. Ticket-worthy as described; not fixable as a one-line change in this PR, matching the record's own conclusion.3. Exclusivity assertion — sound, and I mutation-tested it independently. Added my own stray
<div class="…-backdrop">next to<ModalOverlay>inGroupFormPanel.razor:Nodes.OfType<IElement>().Count()went from 2 to 3, test reddened with exactly that message, restored and green again. The count is coupled toModalOverlay's own root shape, which reads as brittle in isolation — but it's paired withFindComponents<ModalOverlay>(), not standing alone: the component check catches "wrong implementation", the count catches "right implementation plus something extra", and neither alone would have caught what the other does. That pairing is the right mechanism, not a workaround.4. Nothing regressed. Diffed 4fa8d17..66fdd7a directly: only the ADR, four test files, and one unrelated line in
PlaceFormPanel.HandleSavedAsync(a colour-fallback literal, merged in from #136, nothing to do with this PR) changed. Backdrop CSS,PlaceDetailPanel,Drawer's desktop scrim condition andPlaceFormPanel.RequestCloseAsync's routing are byte-identical to what I already confirmed clean in the first round.ADR index. 117 files, 117 rows, both 0116 and 0117 present;
AdrIndexTests(12/12) passes mechanically rather than by my own count alone.