Charts - Closing expanded chart with Esc shows a blue frame and waits for key release

[luacmartins]

Problem

Closing an expanded chart with Esc shows a blue frame. Escape dismissal also differs from the normal attachment viewer: attachments close on keydown, while the chart modal waits until keyup.

Reported by @situchan during review of #97698. The keydown/keyup inconsistency was explicitly identified as pre-existing and deferred to a separate issue.

Action Performed

1. Open a chat containing an inline chart.
2. Click the expand button to open the full-screen chart modal.
3. Press and hold Esc, then release it.
4. Observe when the modal closes and the blue frame shown on dismissal.
5. Open a normal image attachment and repeat the same Escape interaction to compare.

Expected Result

• The expanded chart closes immediately on Esc keydown, matching the normal attachment viewer.
• Closing the chart with Esc does not introduce the blue-frame artifact shown in the recording.

Actual Result

• The chart modal closes only when Esc is released (keyup).
• A blue frame appears when closing the modal with Esc.
• Normal attachments close immediately on Esc keydown.

Context

• Original report: feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas #97698 (comment)
• Decision to handle the Escape timing inconsistency separately: feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas #97698 (comment)
• Version and staging/production reproducibility were not specified in the report.

Screenshots/Videos

Screen.Recording.2026-09-15.at.10.37.09.PM.mov

Issue Owner

Current Issue Owner: @abbasifaizan70

[abbasifaizan70]

@luacmartins, can you assign me this issue?

[luacmartins]

Done

[melvin-bot]

@luacmartins — to hire @abbasifaizan70 on Upwork, add the External label so a job is created and the offer is sent automatically.

[melvin-bot]

Current assignee @situchan is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

[MelvinBot]

Proposal

What is the root cause of that problem?

Both symptoms come from one thing: the chart modal has no Escape handler of its own, so it falls through to the generic Modal fallback — which is bound to keyup.

The expanded chart and the attachment viewer render the same Modal component with the same type. The single line of difference is that the attachment viewer explicitly subscribes Escape on keydown:

src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/index.tsx:206 — useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, ...), which binds through react-native-key-command's document keydown-capture listener.

The chart modal has no such subscription, so nothing handles its keydown and it is only dismissed by the generic fallback at src/components/Modal/ReanimatedModal/index.tsx:98:

document.body.addEventListener('keyup', handleEscape, {capture: true});

That keydown/keyup gap is also what makes the blue frame visible. focus-trap deactivates on Escape keydown (escapeDeactivates defaults to true), so focus — and the App's own keyboard focus indicator, box-shadow: inset 0 0 0 1px #5AB0FF from web/index.html:84-87 — lands on an element while the modal is still open, and stays on screen until you release the key. The frame is App CSS on :focus-visible, not the browser's default outline.

This is pre-existing and generic to any Modal without a keydown subscriber, as situchan said. PR #97698 did not introduce it.

What changes do you think we should make in order to solve the problem?

1. Close on keydown (fixes the key-release delay). In VictoryChartExpandModal.tsx, subscribe Escape with useKeyboardShortcut exactly as the attachment viewer does, calling onClose.

⚠️ This must be gated on isActive: isVisible. VictoryChartExpandable.tsx:70 deliberately never resets shouldRenderModal, so the modal stays mounted for the rest of the session after the first expand. An ungated priority-0 handler would swallow Escape app-wide for every chart ever expanded.

2. Suppress the focus ring (fixes the blue frame). Follow the canonical blur-on-click pattern from MenuItemRoot.tsx:52-59 on the click-zoom pressable at VictoryChartExpandedContent/index.tsx:83-92: drop DOM focus on mouse activation only, so Tab users keep the ring (WCAG 2.4.7).

Do not swap in PressableWithoutFocus here — its pressAndBlur calls onPress() with no arguments, and useClickZoomPan needs nativeEvent.offsetX/offsetY to centre the zoom, so click-point-centred zoom would silently regress.

Also note styles.noOutline is the fix a reviewer reaches for first, and it likely will not work: react-native-web applies it as an atomic class (specificity 0,1,0), which loses to :focus-visible:not([data-programmatic-focus="true"]) (0,2,0).

One thing to confirm before picking the step-2 fix: which element actually receives the ring. I could not verify this at runtime — the test session rendered a blank page, and reproducing an inline chart needs AI-generated content. Watch the recording and note where the frame sits, or log document.activeElement on Escape keydown:

• Around the chart inside the modal → the click-zoom pressable (step 2 above). Most likely; it is the only focusable element PR feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas #97698 added inside this modal, and situchan would have clicked the chart while reviewing click-to-zoom.
• Around the composer at the bottom of the report → ComposerWithSuggestions.tsx:856-891 refocuses the composer whenever any modal closes. That is a global behaviour and should be its own issue, not part of this fix.

Full event sequence and evidence

Expanded chart (broken):

1. keydown — react-native-key-command's document capture listener fires. The ESCAPE stack holds only the app-level EscapeHandler, which early-returns because any open Modal sets MODAL.willAlertModalBecomeVisible = true (BaseModal.tsx:163). Nothing closes.
2. Same keydown, bubble phase — focus-trap's checkEscapeKey deactivates the trap; FocusTrapForModal/index.web.tsx:62-63 runs its focus restore. The blue :focus-visible ring is painted — with the modal still on screen.
3. keyup — ReanimatedModal's document.body capture listener finally fires → onBackButtonPress → closeTop (BaseModal.tsx:337-338, whose comment documents this as the Escape path) → setIsExpanded(false).
4. Modal animates out, revealing the ring applied back in step 2 — the "blue frame on dismissal".

Attachment viewer (correct): the keydown handler at AttachmentModalBaseContent/index.tsx:206 sits at index 0 of the ESCAPE stack (useKeyboardShortcut defaults priority = 0), so it runs on the very first keydown and calls Navigation.dismissModal(). By the time keyup arrives, onBackButtonPressHandler() returns false because isVisibleState is already false, so the keyup path is a no-op.

A second keyup path exists. react-native-web's own ModalContent.js also binds document.addEventListener('keyup', closeOnEscape), wired to onRequestClose at ReanimatedModal/index.tsx:236. In practice the App's document.body capture listener wins and calls stopImmediatePropagation(). This matters for the alternative below.

Repo already documents the ordering at ReanimatedModal/index.tsx:138-143: "the focus trap may have already returned focus to the launcher ... (visible on Escape, where focus-trap deactivates before we close)".

Not verified: whether Chromium honours FocusOptions.focusVisible, which restoreFocusWithModality.ts:16 relies on. My understanding is it is Firefox-only; if so, every programmatic focus restore in Chrome can show this frame, which would be the systemic cause behind this and the earlier blue-frame reports.

Ruled out: shouldHandleNavigationBack (added by PR #97698) only feeds useSyncModalWithHistory for browser Back — TOGGLE_MODAL_WITH_HISTORY leaves the focused route key unchanged, so diffNavigationState returns noop and no focus path runs. PR #97698's MultiGestureCanvas/useTapGestures changes are a zoom clamp only, with no focus, DOM, or a11y effect.

What alternative solutions did you explore? (Optional)

Change the global fallback at ReanimatedModal/index.tsx:98 from keyup to keydown. This would fix every modal at once and is arguably the more correct fix, but it is riskier: react-native-web's own keyup Escape path would remain as a straggler, and for modals that already have a keydown subscriber both handlers would run inside the same event before React re-renders, so closeTop could pop and close an unrelated modal underneath. Worth a follow-up issue with proper regression testing, not this one.

Pass shouldReturnFocus={false} to the chart's Modal so no focus restore happens. Cheap and local, but it loses focus return for keyboard users.

---

Possibly related (not a duplicate): [$500] Add "expand" button overlay to charts for full-screen mode — the parent feature issue for this modal.

---

Next Steps for Contributor+ team:
To accept: @MelvinBot implement [this](https://github.com/Expensify/App/issues/101373) to create a draft PR.
To refine: @MelvinBot <your feedback>
To reject: Explain why you are rejecting Melvin's proposal.

---

view run · no recording available

[melvin-bot]

@abbasifaizan70 Eep! 4 days overdue now. Issues have feelings too...