fix: [Web] Focus restoration mechanism on back navigation (#76921)#79834
fix: [Web] Focus restoration mechanism on back navigation (#76921)#79834mavrickdeveloper wants to merge 18 commits intoExpensify:mainfrom
Conversation
Note for code-reviewersThis PR fixes Keyboard navigation focus loss during back navigation on Web. Three components work together:
The timing issue When a user clicks a button that triggers navigation, the events unfold in a specific order: at T+0ms the user clicks and activeElement is the button; at T+1ms the click handler calls Navigation.navigate(); sometime later React processes the state change and useNavigationState fires; and by then, the screen transition has begun and activeElement has already moved to body. By the time useNavigationState fires, focus has already moved. The hook tells you navigation happened, not what was focused before it happened. Reactive vs Proactive
My Solution Capture-phase DOM events (pointerdown, keydown with {capture: true}) run at T+0ms, before any click handlers execute, before navigation triggers, before focus moves. This is the only mechanism that fires early enough to capture the correct element. React hooks are designed to respond to state changes. But we need to capture state before the change occurs. This requires stepping outside React's reactive model entirely. Changes
|
mavrickdeveloper
changed the title
mavrickdeveloper
changed the title
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/libs/NavigationFocusManager.ts
Outdated
| // Use startsWith for robustness against minor content changes | ||
| const candidateText = (candidate.textContent ?? '').slice(0, 100).trim(); | ||
| if (identifier.textContentPreview && candidateText.startsWith(identifier.textContentPreview.slice(0, 20))) { | ||
| score += 30; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The magic number 20 in slice(0, 20) is used in the element matching logic without clear documentation or being defined as a named constant. This reduces code maintainability.
Suggested fix:
Define a constant at the top of the file:
const TEXT_CONTENT_PREFIX_LENGTH = 20; // Chars to match for fuzzy text comparisonThen use it in the code:
if (identifier.textContentPreview && candidateText.startsWith(identifier.textContentPreview.slice(0, TEXT_CONTENT_PREFIX_LENGTH))) {
score += 30;
}
```\n\n---\n\nPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.There was a problem hiding this comment.
Will address
There was a problem hiding this comment.
@mavrickdeveloper ✅ This is a valid review comment, all magic numbers should be extracted to CONST and named accordingly for what their use is.
- all score related ones (
5,10,15,30,40,50) - both slice related (
20,100)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50e2a4d6b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/components/ConfirmModal.tsx
Outdated
| const computeInitialFocus = (() => { | ||
| const platform = getPlatform(); | ||
|
|
||
| // Skip for mouse/touch opens or non-web platforms | ||
| if (!wasOpenedViaKeyboardRef.current || platform !== CONST.PLATFORM.WEB) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
On the first render when isVisible flips to true, wasOpenedViaKeyboardRef.current is still undefined because it’s only set in the useLayoutEffect below. That means computeInitialFocus is computed as false and never re-evaluated (no state update), so a ConfirmModal opened via keyboard won’t auto-focus its first button on web. This is a regression for keyboard users because the focus trap never gets the intended initial target. Consider deferring the keyboard check to the initialFocus function itself or forcing a re-render after capturing the flag.
Useful? React with 👍 / 👎.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Accessibility project issue 👍
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-01-22.at.19.54.10.mov |
|
🟡 Medium: Verbose Logging in Production Files:
The file contains extensive
Element attributes exposed in logs - @mavrickdeveloper Let's remove these if they are not used for debugging anymore, or if they are really needed for some reason they must be wrapped by an |
| const computeInitialFocus = (): HTMLElement | false => { | ||
| const platform = getPlatform(); | ||
|
|
||
| // Check ref LAZILY - this runs when FocusTrap activates (after useLayoutEffect) | ||
| if (!wasOpenedViaKeyboardRef.current || platform !== CONST.PLATFORM.WEB) { | ||
| return false; | ||
| } | ||
|
|
||
| // CRITICAL: Scope query to this modal's container | ||
| // This prevents focusing buttons from OTHER open modals | ||
| // in nested scenarios (e.g., ThreeDotsMenu → PopoverMenu → ConfirmModal) | ||
| const container = modalContainerRef.current as unknown as HTMLElement; | ||
| if (!container) { | ||
| // Fallback: If container ref not set, use last dialog (legacy behavior) | ||
| Log.warn('[ConfirmModal] modalContainerRef is null, falling back to last dialog'); | ||
| const dialogs = document.querySelectorAll('[role="dialog"]'); | ||
| const lastDialog = dialogs[dialogs.length - 1]; | ||
| const firstButton = lastDialog?.querySelector('button'); | ||
| return firstButton instanceof HTMLElement ? firstButton : false; | ||
| } | ||
|
|
||
| const firstButton = container.querySelector('button'); | ||
|
|
||
| Log.info('[ConfirmModal] initialFocus activated via keyboard', false, { | ||
| foundButton: !!firstButton, | ||
| buttonText: firstButton?.textContent?.slice(0, 30), | ||
| }); | ||
|
|
||
| return firstButton instanceof HTMLElement ? firstButton : false; | ||
| }; |
There was a problem hiding this comment.
🟡 Medium: Complex Function in ConfirmModal
The computeInitialFocus function is 48 lines with multiple responsibilities. Why it matters:
- Mixes platform detection, container lookup, fallback logic, and logging
- Hard to unit test individual concerns
- The
as unknown as HTMLElementcast suggests type issues (⚠️ also type casting is forbidden)
Suggested Refactor:
/**
* Find the first focusable button in this modal's container.
* Scopes the query to avoid focusing buttons from other open modals.
*/
const findFirstFocusableButton = (): HTMLElement | false => {
// Don't rely on ref casting - query the DOM directly
// The modalContainerRef's testID allows us to find it reliably
const container = document.querySelector('[data-testid="confirm-modal-container"]');
if (!container) {
// Fallback: Find last dialog (this modal)
const dialogs = document.querySelectorAll('[role="dialog"]');
const lastDialog = dialogs[dialogs.length - 1];
if (!lastDialog) {
return false;
}
const button = lastDialog.querySelector('button');
return button instanceof HTMLElement ? button : false;
}
const button = container.querySelector('button');
return button instanceof HTMLElement ? button : false;
};
// src/libs/focusUtils/computeInitialFocus/index.web.ts
/**
* Compute initialFocus for Modal's FocusTrap.
* Returns the first button for keyboard opens, false otherwise.
*/
const computeInitialFocus = (): HTMLElement | false => {
if (!wasOpenedViaKeyboardRef.current) {
return false;
}
return findFirstFocusableButton();
};
// src/libs/focusUtils/computeInitialFocus/index.ts
/**
* Compute initialFocus for Modal's FocusTrap on native.
* Native platforms handle focus differently, so this is a no-op.
*
* @returns false - Native platforms don't use HTML-based focus traps
*/
function computeInitialFocus(): false {
return false;
}
export default computeInitialFocus;✅ No platform checks - Extract platform dependent function to platform-specific files
✅ No type casting - Uses document.querySelector directly instead of trying to cast the React Native ref
✅ Relies on testID - The modalContainerRef has testID="confirm-modal-container" which we can query
✅ Type-safe fallback - Falls back to [role="dialog"] query which is already in the DOM
✅ instanceof checks - Uses proper type guards instead of casts
This works because:
- On web, the View with
testID="confirm-modal-container"renders as<div data-testid="confirm-modal-container"> - We don't need to access the ref at all - just query the DOM directly
- TypeScript is happy because we're using proper DOM APIs throughout
The ref was only needed for scoping, which we now achieve via the testID selector.
Caution
Before pushing the refactored code please ensure that the logic still works as the previous logic (using ref) to avoid regressions if pushing refactor without verification.
| if (wasFocused && !isNowFocused) { | ||
| // Screen is losing focus (forward navigation) - capture the focused element | ||
| NavigationFocusManager.captureForRoute(route.key); | ||
| } |
There was a problem hiding this comment.
🔴 Critical: Missing null check could cause runtime error
Why it matters: If route is undefined (edge case during rapid navigation), route.key access throws:
TypeError: Cannot read properties of undefined (reading 'key')| if (wasFocused && !isNowFocused) { | |
| // Screen is losing focus (forward navigation) - capture the focused element | |
| NavigationFocusManager.captureForRoute(route.key); | |
| } | |
| if (wasFocused && !isNowFocused && route?.key) { | |
| // Screen is losing focus (forward navigation) - capture the focused element | |
| NavigationFocusManager.captureForRoute(route.key); | |
| } |
| function initialize(): void { | ||
| if (isInitialized || typeof document === 'undefined') { | ||
| return; | ||
| } | ||
|
|
||
| // Capture phase runs BEFORE the event reaches target handlers | ||
| // This ensures we capture the focused element before any navigation logic | ||
| document.addEventListener('pointerdown', handleInteraction, {capture: true}); | ||
| document.addEventListener('keydown', handleKeyDown, {capture: true}); | ||
|
|
||
| isInitialized = true; | ||
| } |
There was a problem hiding this comment.
🟡 Medium: Potential memory leak in event listeners
Why it matters: If initialize() is called multiple times (e.g., during hot reload), and destroy() isn't called in between, listeners accumulate.
Bug Scenario:
- User navigates, triggers capture
- Hot reload occurs
useEffectinApp.tsxruns againisInitializedistrue(module state persists), but effect cleanup randestroy()- Listeners are gone but not re-added
Fix:
| function initialize(): void { | |
| if (isInitialized || typeof document === 'undefined') { | |
| return; | |
| } | |
| // Capture phase runs BEFORE the event reaches target handlers | |
| // This ensures we capture the focused element before any navigation logic | |
| document.addEventListener('pointerdown', handleInteraction, {capture: true}); | |
| document.addEventListener('keydown', handleKeyDown, {capture: true}); | |
| isInitialized = true; | |
| } | |
| function initialize(): void { | |
| if (typeof document === 'undefined') { | |
| return; | |
| } | |
| // Always clean up first to handle hot reload | |
| if (isInitialized) { | |
| destroy(); | |
| } | |
| // Capture phase runs BEFORE the event reaches target handlers | |
| // This ensures we capture the focused element before any navigation logic | |
| document.addEventListener('pointerdown', handleInteraction, {capture: true}); | |
| document.addEventListener('keydown', handleKeyDown, {capture: true}); | |
| isInitialized = true; | |
| } |
| onModalHide={() => { | ||
| // Restore focus to captured anchor (web only) | ||
| // This improves accessibility by returning focus to the trigger element | ||
| if (getPlatform() === CONST.PLATFORM.WEB && capturedAnchorRef.current && document.body.contains(capturedAnchorRef.current)) { | ||
| capturedAnchorRef.current.focus(); | ||
| Log.info('[ConfirmModal] Restored focus to captured anchor', false, { | ||
| anchorLabel: capturedAnchorRef.current.getAttribute('aria-label'), | ||
| }); | ||
| } | ||
| // Reset the ref AFTER focus restoration (not in useLayoutEffect) | ||
| capturedAnchorRef.current = null; | ||
| onModalHide(); | ||
| }} |
There was a problem hiding this comment.
🟡 Medium: Platform-specific code without native fallbacks
Why it matters: Using document.body.contains() and .focus() directly assumes web platform - while gated by platform check, this pattern should use platform-specific files as per our code guidelines.
Suggested Pattern: To separate concerns, extract to a utility function:
// libs/focusUtils/index.ts (native)
export const restoreFocus = () => {}; // no-op
// libs/focusUtils/index.web.ts
export const restoreFocus = (element: HTMLElement | null) => {
if (element && document.body.contains(element)) {
element.focus();
}
};
| const activeElement = document?.activeElement; | ||
| if (activeElement instanceof HTMLElement && (activeElement.tagName === 'INPUT' || activeElement.tagName === 'TEXTAREA')) { | ||
| blurActiveElement(); | ||
| } |
There was a problem hiding this comment.
🟡 Medium: Duplicated blur-only-inputs logic
Files:
src/components/MoneyRequestConfirmationList.tsx(lines 1042-1047)src/pages/tasks/NewTaskPage.tsx(lines 68-73)
Both have identical logic:
const activeElement = document?.activeElement;
if (activeElement instanceof HTMLElement && (activeElement.tagName === 'INPUT' || activeElement.tagName === 'TEXTAREA')) {
blurActiveElement();
}Why it matters: Duplicated logic should be extracted to maintain consistency.
Suggested Fix:
- additionally use existing
CONSTvariables instead of in-code strings ✅
// src/libs/blurActiveElement/index.ts
export function blurActiveInputElement(): void {
const activeElement = document?.activeElement;
if (activeElement instanceof HTMLElement &&
(activeElement.tagName === CONST.ELEMENT_NAME.INPUT || activeElement.tagName === CONST.ELEMENT_NAME.TEXTAREA)) {
blurActiveElement();
}
}
| * Find an element in the current DOM that matches the stored identifier. | ||
| * Uses a scoring system to find the best match. | ||
| */ | ||
| function findMatchingElement(identifier: ElementIdentifier): HTMLElement | null { |
There was a problem hiding this comment.
🟡 Medium: Impure function accessing global state
This is an impure (queries DOM). Why it matters: For testability, DOM queries should be injectable.
Suggested Pattern:
function findMatchingElement(
identifier: ElementIdentifier,
queryFn: (selector: string) => NodeListOf<HTMLElement> = (s) => document.querySelectorAll(s)
): HTMLElement | null {
const candidates = queryFn(identifier.tagName);
// ...
}
|
|
||
| // Capture focus when screen loses focus (navigating away) and restore when returning | ||
| // useLayoutEffect runs synchronously, minimizing the timing window | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
🟡 Medium: useLayoutEffect with heavy DOM operations
Why it matters: useLayoutEffect blocks painting. Complex logic here can cause jank.
Suggested optimization:
useLayoutEffect(() => {
// Keep synchronous checks minimal
const wasFocused = prevIsFocused.current;
prevIsFocused.current = isFocused;
if (wasFocused && !isFocused) {
NavigationFocusManager.captureForRoute(route.key);
}
}, [isFocused, route.key]);
// Move restoration logic to useEffect (can be async)
useEffect(() => {
if (!prevIsFocused.current && isFocused && NavigationFocusManager.hasStoredFocus(route.key)) {
// Focus restoration logic here
}
}, [isFocused, route.key, isActive]);
src/components/PopoverMenu.tsx
Outdated
| // CRITICAL: Scope query to this menu's container | ||
| // This prevents focusing menuitems from OTHER open modals | ||
| // in nested scenarios (e.g., ThreeDotsMenu → PopoverMenu → ConfirmModal) | ||
| const container = menuContainerRef.current as unknown as HTMLElement; |
There was a problem hiding this comment.
🟡 Medium: Same forbidden type casting
See comment https://github.com/Expensify/App/pull/79834/files#r2719350670 for how to address the forbidden type casting issue here.
🔴 Failing Manual Tests
Video proofcat-new.mov
Video proofcat-settings.mov
Videos proof27wf-edit.mov28wf-sub.mov29wf-add.mov
Videos proof30rul.mov31dis.mov☝️ It happened maybe once or twice during the entire manual testing that the focus actually returned back to the workspace sidebar menu item - @Expensify/design what's your take on this, should we actually stick to the tests on this ✅ All other tests are passing. Tip Regarding merging with focus that's not persistent (always works the same) I think we're fine with having multiple follow-up PRs since this keyboard focusing feature does not actually block users from using the app - are only accessibility related issues which don't require reverts as the regressions won't be deploy blockers because this initial PR does improve accessibility overall. @mavrickdeveloper Will be awaiting for you to address all code review / testing issues before doing another pass before merge 👍 Take your time - goal here is to minimize regressions as much as possible while having clean code 🟢 |
I think for focus we should always go back to the element that opened the RHP. So the video looks right to me. If a user is in a flow going through elements like this
Then I'd expect them to go back to element 3 not element 1 or something else. This leaves the user in the process they were in and can continue to open element 4, 5, so on. Does that make sense? |
|
Appreciate the thorough review @ikevin127 , will address your code review comments. But before diving into the failing tests (25-35), I believe what your videos show is actually the intended behavior of my PR & per the original issue. From #76921: "focus should return to the element that triggered the navigation" So for eg: test 26 (Categories - Settings), focus returning to the More button is correct ,that's the element that triggered the navigation, not the workspace sidebar menu item. Also as @dubielzyk-expensify described #79834 (comment) Thoughts ? |
…x matches in element scoring Swapped condition order so exact matches now correctly score higher than prefix-only matches. Also replaced all magic numbers with named constants following the MATCH_RANK pattern from filterArrayByMatch.ts.
mavrickdeveloper
force-pushed
the
fix/76921-focus-restoration-clean
branch
from
97b4aea to
170a84c
Compare
Reason: improves keyboard focus determinism and cleanup safety while preserving mouse/touch behavior by isolating platform-specific focus helpers, deduplicating input blur logic, and closing a provenance-only cleanup gap. - extract ConfirmModal focus restore logic into web/native helpers - add shared blurActiveInputElement helper for input/textarea-only blur behavior - scaffold NavigationFocusManager metadata/provenance for keyboard-safe routing - fix cleanupRemovedRoutes to clear provenance-only route state - expand focus tests across NavigationFocusManager, FocusTrap, ConfirmModal, and keyboard-intent integration
|
@mavrickdeveloper Please consider the CONTRIBUTING: Submit your pull request for final review rule quoted below as I noticed you already force-pushed two times:
Any update on the status / completion % of the PR ? |
- PopoverMenu: compute initial focus from actionable button/menuitem candidates and skip disabled, inert, and non-focusable rows - NavigationFocusManager: add shared listener registry with ownership checks to prevent stale/duplicate listeners across hot reload - Add regression tests for popover keyboard selection/Enter behavior and NavigationFocusManager module-replacement scenarios
|
@ikevin127 Thanks for the thorough feedback, really appreciate it , I’ve addressed the code review points raised so |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…ration-clean # Conflicts: # src/components/MenuItem.tsx
…nu focus coverage Cover lifecycle hooks, keyboard tracking, and focus restoration paths flagged by Codecov as uncovered in PR Expensify#79834.
…o native resolution
|
@mavrickdeveloper How are we looking here - what's left to do ? |
re-requested review , please check |
| onModalHide={() => { | ||
| // Focus the anchor button after modal closes but before navigation triggers | ||
| // This ensures NavigationFocusManager can capture it for focus restoration on back navigation | ||
| (dropdownAnchor.current as unknown as HTMLElement)?.focus?.(); |
There was a problem hiding this comment.
🔴 Critical Issue: Forbidden Type Casting (as unknown as HTMLElement)
Caution
Type casting is forbidden per Expensify's STYLE.md. This PR introduces a double cast in ButtonWithDropdownMenu.
Per the STYLE.md Refs section, when you need DOM methods on a RN ref, declare the ref as a union type and use proper narrowing:
// ✅ FIX — union type ref + proper narrowing
const dropdownAnchor = useRef<View | HTMLDivElement>(null);
// In onModalHide:
onModalHide={() => {
if (dropdownAnchor.current && 'focus' in dropdownAnchor.current) {
(dropdownAnchor.current as HTMLDivElement).focus();
}
}}Bug if not addressed: The double cast silences TypeScript entirely. If dropdownAnchor.current is not an HTMLElement on native platforms, calling .focus?.() would fail silently or crash.
| @@ -0,0 +1,18 @@ | |||
| import blurActiveElement from '@libs/Accessibility/blurActiveElement'; | |||
There was a problem hiding this comment.
🔴 Critical Issue: Missing types.ts for Platform-Specific Modules
Important
Per STYLE.md: "In modules with platform-specific implementations, create types.ts to define shared types."
Files affected:
src/components/ConfirmModal/focusRestore/index.ts+index.web.tssrc/libs/Accessibility/blurActiveInputElement/index.ts+index.native.ts
Both modules define InitialFocusParams separately in each file. They should share a common types.ts:
// src/components/ConfirmModal/focusRestore/types.ts
type InitialFocusParams = {
isOpenedViaKeyboard: boolean;
containerElementRef: unknown;
};
type FocusRestoreModule = {
getInitialFocusTarget: (params: InitialFocusParams) => HTMLElement | false;
restoreCapturedAnchorFocus: (capturedAnchorElement: HTMLElement | null) => void;
shouldTryKeyboardInitialFocus: (isOpenedViaKeyboard: boolean) => boolean;
isWebPlatform: (platform: string) => boolean;
};
export type {InitialFocusParams, FocusRestoreModule};Bug if not addressed: Each platform file can drift independently, breaking the cross-platform contract.
|
|
||
| type InteractionTrigger = 'enterOrSpace' | 'escape' | 'pointer' | 'unknown'; | ||
|
|
||
| type RetrievalMode = 'keyboardSafe' | 'legacy'; |
There was a problem hiding this comment.
🟠 High Severity Issue: Dead/Unused Exported Code in NavigationFocusManager
The following types and functions are defined and exported but never imported anywhere else in src/:
| Symbol | Lines | Status |
|---|---|---|
RetrievalMode |
80 | Dead type |
getRetrievalModeForRoute |
809–815 | Exported, never called |
getRouteFocusMetadata |
817–819 | Exported, never called |
Warning
The methods are only used in 1 testing file which is also irrelevant since there's no point in testing methods that are not actually used in the app.
// ❌ Dead code — remove or mark as intended for future use
type RetrievalMode = 'keyboardSafe' | 'legacy';
function getRetrievalModeForRoute(routeKey: string): RetrievalMode { ... }
function getRouteFocusMetadata(routeKey: string): RouteFocusMetadata | null { ... }Why this matters: Dead exported code increases bundle size and creates false impressions about the public API surface.
| * This prevents attempting to focus elements that have been hidden or disabled | ||
| * since they were captured. | ||
| */ | ||
| function isElementFocusable(element: Element | null): boolean { |
There was a problem hiding this comment.
🟡 NAB: Duplicated Focus Validation Logic
isElementFocusable()inFocusTrapForScreen/index.web.tsxisFocusableActionablePopoverCandidate()inPopoverMenu.tsx
both check disabled, inert, aria-disabled, and dimensions. These should be extracted into a shared utility.
// ✅ Extract to src/libs/focusUtils.ts (or similar)
function isElementFocusable(element: Element | null): boolean { ... }
function isFocusableActionable(element: Element): element is HTMLElement { ... }
There was a problem hiding this comment.
🟠 High Severity Issue: NavigationFocusManager Is Web-Only but Imported Unconditionally
NavigationFocusManager.ts relies heavily on document, HTMLElement, and DOM APIs. It's imported in:
App.tsx(all platforms)ComposerWithSuggestions.tsx(all platforms)ButtonWithDropdownMenu/index.tsx(all platforms)ThreeDotsMenu/index.tsx(all platforms)ConfirmModal.tsx(all platforms)
While initialize() guards with typeof document === 'undefined', every call site still imports the module and calls methods like wasRecentKeyboardInteraction() on native platforms (returning false by default). This works but wastes bundle space.
Consider creating a platform-specific split:
src/libs/NavigationFocusManager/
index.ts ← no-op stubs (native)
index.web.ts ← full implementation
types.ts ← shared type contract
| @@ -639,5 +722,6 @@ export default React.memo( | |||
| prevProps.withoutOverlay === nextProps.withoutOverlay && | |||
| prevProps.shouldSetModalVisibility === nextProps.shouldSetModalVisibility, | |||
| ); | |||
There was a problem hiding this comment.
🟡 Medium Severity Issue: Missing wasOpenedViaKeyboard in React.memo Comparison
The newly added wasOpenedViaKeyboard prop is not included in the custom React.memo comparison function at the bottom of the file.
Bug if not addressed: If wasOpenedViaKeyboard changes but all other compared props remain the same, the component won't re-render, causing stale initial focus behavior.
| ); | |
| prevProps.wasOpenedViaKeyboard === nextProps.wasOpenedViaKeyboard, | |
| ); |
There was a problem hiding this comment.
🟢 NAB: Test File Names Don't Follow Existing Convention
Several test files use mixed naming patterns:
ButtonWithDropdownMenuFocusCoverageTest.tsx(PascalCase + "Test" suffix) ✅blurActiveInputElementTest.ts(camelCase + "Test" suffix) ❌
Unify to:
BlurActiveInputElementTest.ts
|
@mavrickdeveloper Dropped a few comments that need addressed:
Let me know when you're done addressing them and I'll take another look 🙌 |
|
@ikevin127 thanks for the fast review , will address |
mavrickdeveloper
force-pushed
the
fix/76921-focus-restoration-clean
branch
from
14feebe to
826d7ae
Compare
4 participants
Explanation of Change
This PR fixes the Web-only issue where keyboard navigation focus was lost across
48 scenarios/pagesafter navigating back in the app, causing screen readers to announce incorrect elements.adherence to WCAG 2.4.3 - Focus Order (Level A) , WCAG 3.2.3 Consistent Navigation (AA) , WCAG 2.1.2 No Keyboard Trap (A) & , WCAG 2.1.1 Keyboard (A)
Keyboard focus order , should not conflict with pre-existing custom (mouse/touch) focus behavior
Notes to code reviewers: fix: [Web] Focus restoration mechanism on back navigation (#76921) #79834 (comment)
Root Cause:
PR Disable initialFocus and setReturnFocus in focusTrap for screen #49240 set
setReturnFocus: falseinFocusTrapForScreento fix Issue [$250] mWeb – Chat – Blue frame appears around + button when open conversation link URL in a new tab #46109 (blue frame appearing on new tab). This was an over-correction that broke focus restoration during in-app navigation.Fundamental race-condition issues causing cross-cutting focus stealing ( focus management touches many files by nature)
Solution: Implement a custom
NavigationFocusManagerthat:Fixed Issues
$ #76921
PROPOSAL: #76921 (comment)
Prerequisite:
The user is logged in
Using Windows + Chrome, open expensify dev
Navigate using TAB only in the scenarios below
Navigate to the 'Back' button using TAB, press enter to activate
Observe the focus behavior , the correct behavior is the back button (in RHP for eg) should restore the keyboard focus to the triggering element
Tests
Primary Test (Issue #76921):
Note : Tests should only be executed using keyboard navigation (TAB) , (See attached video below for demonstration)
Additional scope : Other pages to test on:
On Settings - About - Keyboard Shortcuts
On Settings - Save the world - I know a teacher
On Settings - Save the world - I am a teacher
On Settings - Save the world - Intro your school principal
On Settings - Preferences - Language
On Settings - Preferences - Priority mode
On Settings - Security
On Settings - Security - Validate your account
On Settings - Security - Close account
On Settings - Security - Two-factor authentication
On Settings - Profile - Display name
On Settings - Profile - Contact methods
On Settings - Profile - Pronouns
On Settings - Profile - Share Code
On Settings - Profile - Legal Name
On Settings - Profile - DOB
On Settings - Profile - Phone number
On Settings - Profile - Address
On Settings - Profile - Country
On Settings - Profile - Timezone
On Workspaces - Duplicate Workspaces
On Workspaces - Delete Workspace
On Workspaces - Overview - Workspace Name
On Workspaces - Overview - Expensify Policy
On Workspace - Categories - Add Category
On Workspace - Categories - Settings
On Workspace - Workflows - Edit Approval Workflow
On Workspace - Workflows - Expenses From
On Workspace - Workflows - Approver
On Workspace - Rules - Cash Expense Default
On Workspaces - Distance Rates - Rate Details
On Workspaces - Expensify Card - Add bank account
On Workspaces - Expensify Card - Bank info
On Workspaces - Expensify Card - Confirm currency and country
On Workspace - Company Card - Add Cards
On Workspace - Create Workspace - Confirm Workspace
On Workspace - Create Workspace - Invite new members
On Workspace - Create Workspace - Default Currency
On Create Report - Restricted
On Create Report - Add payment card
On Create Report - Change payment currency
On Track Distance
On Track Distance - Choose Recipient
On Send Invoice
On Wallet - Add bank account
On Create Expense flow
On Paid Expense details flow
On Reports flow
On Chat flow
Regression Test (Issue #46109):
Offline tests
N/A - Focus restoration is a client-side UI behavior that doesn't depend on network state.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
2026-01-13.17-13-53.mp4