fix(ui): Escape closes exactly one popup, topmost first (#535) - #790
Merged
Merged
Conversation
Escape handling had grown one ad-hoc `document` keydown listener per hand-rolled overlay, none of which knew about the others or about react-bootstrap's own listener. With two layers on screen a single press either closed the wrong one or closed both — EntityDetailModal, for instance, swallowed Escape with `stopImmediatePropagation` whenever no nested *IP* panel was open, so the role help / evidence explainer / add-evidence modals opened from inside it took the whole panel down with them. Replaces those listeners with a shared coordinator (`useEscapeLayer`): - one capture-phase listener dispatches Escape to the topmost registered layer, ranked by painted stacking order (z-index path, then document order) with registration order as the tiebreak; - it yields when an SGDS modal is stacked above the layer, so react-bootstrap closes that modal itself, and stops propagation when it does handle the key so no modal below closes in the same press; - an open info popover wins outright: every OverlayTrigger here is click + rootClose, which dismisses on Escape *keyup*, so swallowing the keydown leaves the modal underneath alone. Also closes gaps the audit turned up: the cluster side panel only responded to Escape in fullscreen, and the geo-source popovers and the node-colour explainer did not respond at all. The page/dialog fullscreen handlers lose their hand-rolled "close this modal first" cascades — the stack now orders them. Every SGDS Modal was inventoried: all close on Escape via `onHide` except the four that block dismissal on purpose (upload in progress, merge in progress, snapshot save, pcap removal). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- topLayer() no longer lets a layer whose ref.current is temporarily null (e.g. an unmounted-then-remounting CSS-fullscreen pane) win the stacking tie-break outright; such layers are skipped instead. - Nested popups inside EntityDetailModal (role help, evidence explainer, add-evidence) now always raise above it via tp-nested-modal, instead of only when the parent was given an explicit zIndex prop — no real call site ever passed one, so the #535 bug this PR fixes could still reproduce. - NetworkControls' "Node colours" explainer now opens an SGDS Modal instead of a hand-rolled dialog behind a native title= tooltip, per CLAUDE.md's popup-affordance rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
Reverts the SGDS-Modal conversion from the previous commit so we can test the original useEscapeLayer-based Escape behavior directly, rather than swapping it for a different mechanism. The two coordinator correctness fixes (null-ref tie-break, EntityDetailModal nested-modal z-index) stay in place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
Undo the correctness patches from the previous two commits (useEscapeLayer.ts, EntityDetailModal + its child sections, and NetworkControls) so the branch matches the original PR #790 content exactly (aside from the dev sync merge). Reverted per request pending a closer look at whether useEscapeLayer duplicates Escape handling that SGDS Modal already provides by default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
Phase 1 of replacing hand-rolled Escape overlays with real SGDS components (see plan). This overlay was already modal-shaped (centered card, backdrop-click-to-close already implemented) so it converts directly: drop the hand-rolled <div> + useEscapeLayer registration in favor of <Modal>, which gets correct Escape-stacking for free from react-bootstrap's shared ModalManager. The unrelated ArrowLeft/ ArrowRight step-navigation keydown listener is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
Phase 2 of replacing hand-rolled Escape overlays with real SGDS components (see plan). EntityDetailModal was a <div> wearing Bootstrap CSS classes, invisible to react-bootstrap's shared ModalManager — that invisibility is the root cause #535 had to work around. Convert it to a real <Modal>: drop the hand-rolled backdrop/click-to-close, the naive non-ref-counted body-scroll-lock effect, and the useEscapeLayer registration, all superseded by Modal's own ModalManager-backed handling. Peer-navigation self-nesting (a second EntityDetailModal opened from inside the first) no longer needs a manual +10 z-index offset — two real Modals stack correctly via the shared ModalManager automatically. This removes the entire zIndex/raisedModal prop-threading chain through HostIdentitySection and AdjudicationPanel, and the .tp-nested-modal/-backdrop CSS classes that existed only to raise their inner Modals (role-help, evidence explainer, add/edit evidence) above the old hand-rolled parent. Also drop enforceFocus={false} from the two Modals that render an EntityDetailModal as a sibling (SnapshotDetailModal, SubnetDiagramModal) — that workaround existed only because the hand-rolled child was invisible to Bootstrap's focus trap; a real nested Modal doesn't need it. Needs manual verification that tabbing into the nested panel still works correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
Phase 3 of replacing hand-rolled Escape overlays with real SGDS
components (see plan). Investigated whether all 5 CSS-fullscreen
panes (.nd-css-fullscreen toggle + useEscapeLayer) could convert to a
real Modal fullscreen: NetworkGraph (Sigma.js, used by both diagram
panes) holds pan/zoom camera state only in the live Sigma instance
with no saved-viewport prop, so branch-swapping Card<->Modal would
unmount/remount it and reset the user's view on every fullscreen
toggle — a real regression. VolumeHeatmap (both heatmap panes) and
ClusterGraph (ReactFlow, ClusterGraph) have no such state at risk:
VolumeHeatmap only has a discrete zoom step + scroll position that
resets to "fit" anyway, and ClusterGraph already auto-refits on
nearly every data/groupBy change and already remounts its own
ReactFlow branch when groupBy toggles to/from 'country'.
Convert the 3 low-risk panes — NetworkDiagramPage's heatmap,
SnapshotDetailModal's heatmap (nested inside its own Modal.Body), and
NetworkClusterPage's cluster graph — to real <Modal fullscreen>,
dropping their useEscapeLayer registrations, refs, and the
.nd-css-fullscreen/-over-modal CSS classes they used. Also remove the
container={graphCardRef.current} hack on NetworkClusterPage's filter
modal (only needed so it portaled inside the old hand-rolled
fullscreen card) and the vestigial, always-dead .card:fullscreen CSS
in ClusterGraph.css (native :fullscreen was never triggered — nothing
called requestFullscreen — so the cluster graph canvas was clamped to
70vh even in "fullscreen" mode; replaced with real .modal-fullscreen
flex-fill rules that actually apply).
Left untouched, on the existing CSS-toggle + useEscapeLayer approach:
NetworkDiagramPage's and SnapshotDetailModal's diagram fullscreen
(both wrap NetworkGraph/Sigma) — this is a deliberate scope decision,
not an oversight; useEscapeLayer keeps these 2 consumers going
forward.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TWoEzqBqJDSu4mk6GAeQYe
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.
Closes #535.
What was wrong
Escape handling was one ad-hoc
documentkeydown listener per hand-rolled overlay, each unaware of the others and of react-bootstrap's own listener. With two layers on screen a press closed the wrong one, or two at once:EntityDetailModalswallowed Escape withstopImmediatePropagationunless a nested IP panel was open — so the role-help, evidence-explainer and add-evidence modals opened from inside it were closed together with the whole panel.ConversationPageclosed the conversation modal on every Escape, on top of the modal's ownonHide.What this does
New shared coordinator
useEscapeLayer(frontend/src/utils/useEscapeLayer.ts), one capture-phase listener for all hand-rolled popups:OverlayTriggers here are click +rootClose, which dismiss on Escape keyup, so swallowing the keydown dismisses the popover and spares the modal beneath it. A root-level guard inMainLayoutkeeps that rule working on pages with no layer of their own.Audit results
SGDS
Modals — all close on Escape viaonHide. Four block dismissal on purpose and are left as-is: upload in progress (keyboard={!isUploading}), merge in progress (keyboard={false}), snapshot save while busy, pcap removal while deleting.Hand-rolled popups — now on the shared stack:
EntityDetailModal(+ its nested self),ConversationTracerModal, the cluster side panel, both geo-source popovers, the node-colour-priority explainer, and the four CSS-fullscreen modes (diagram + heatmap on the analysis page, diagram + heatmap in the monitor dialog).Gaps this closes: the cluster side panel only answered Escape while fullscreen; the geo-source popovers and the node-colour explainer did not answer it at all (the latter also gained
role="dialog").Deliberately still not dismissible: the report-generation progress overlays on Analysis/Compare.
Testing
useEscapeLayer.test.tsx) covering topmost-only dispatch, unwinding a nested stack one press at a time, yielding to a modal above, not closing a modal below, the nested-stacking-context case, and the popover rule. Full suite: 641 passing.docker compose up -d --build):🤖 Generated with Claude Code