Generalize #1849 menu coordination to all context menus (#1849) - #2286
Merged
Conversation
…alpass#1849) Add ContextMenuCoordinator so only one context menu is open at a time; re-activating the same trigger toggles it closed. Applied to priority, status, date, recurrence, reminder, task, ICS event and batch menus, removing duplicated per-component state. Generalizes the callumalpass#1849 fix, which previously covered date menus only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Repeatedly activating the priority indicator on a task card, or opening the task context menu repeatedly, created another menu each time instead of closing or reusing the one already open. Both of these were observed and reproduced (see "Reproduction" below); the other context menus were not observed to stack.
The reason the two differ is not that the other menus are safer — it is that no wrapper coordinated menu visibility at all, except
DateContextMenu, which has had its own fix since #1849 (4.8.0). Because every other wrapper shares the exact same pattern, the fix is applied to all of them rather than patching only the two observed cases: the remaining menus are one refactor away from the same bug, and the behaviour should not depend on which component happens to be clicked.Reproduction
What's in it
ContextMenuCoordinatorowns menu visibility: it hides the active menu before showing a new one and treats a repeated activation of the same trigger as a toggle.showCoordinatedMenu()/showCoordinatedMenuAtElement().DateContextMenu— that component's existing behaviour is unchanged, it now comes from the shared coordinator — and the element-anchored positioning copies shared by five wrappers.role="button"set byprepareInteractiveControl,data-tn-action,data-type), so clicking an inner icon maps to the same control.Testing
tests/unit/components/ContextMenuCoordinator.test.ts(14 cases: toggle on the same trigger, nested elements, replacement across triggers, reopening after an external hide, keyboard-event anchoring).DateContextMenu.issue-1849coverage passes unchanged, confirming the refactor preserves the behaviour that shipped in 4.8.0.npm test: 4159 passed, 0 failed.npm run typecheckandnpm run lintclean.Manual verification
Notes
onHide/hide/showAtMouseEvent/showAtPositionsurface, so it works forContextMenuinstances and for plainMenuinstances — the latter matters forTaskContextMenu, one of the two reproduced cases, since it can be populated onto a caller-suppliedMenu.new Menu()are untouched and can be migrated with the same one-line call if desired.docs/releases/unreleased.md.Refs #1849