Fix keyboard focus not returning to FAB and composer + after Back - #97678
Fix keyboard focus not returning to FAB and composer + after Back#97678Krishna2323 wants to merge 2 commits into
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@MelvinBot review |
Review — LGTM with one minor nitThe approach is correct and the diff is clean. Root cause is accurately identified: the create-menu triggers Verified the wiring holds together:
Nit (low severity, non-blocking)In In practice this is benign: Testing noteThe three new unit tests are well-targeted (registration-before-open ordering, native no-op path, deactivate-on-hide). They mock I couldn't validate the end-to-end keyboard focus-return behavior automatically — the web test tooling here can't dispatch raw Tab/Enter/Arrow keys or read |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@MelvinBot review |
Re-review — nit resolved, LGTM
Nice side benefit: registration and deactivation now both key off Test coverage followed the move correctly: the old No new concerns. Same caveat as before: I can't auto-verify the end-to-end keyboard focus-return in CI (the web tooling can't dispatch raw Tab/Enter/Arrow keys or read |
|
@mkhutornyi 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e43a7105
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // launcher later. | ||
| const launcher = resolvePopoverLauncherElement(actionButtonRef); | ||
| if (launcher) { | ||
| markActivePopoverLauncherDeactivated(launcher); |
There was a problem hiding this comment.
Keep the composer launcher alive through file picking
When the user chooses Add attachment, triggerAttachmentPicker only calls onAttachmentPicked after the OS file picker returns, and that handler navigates to REPORT_ADD_ATTACHMENT; marking the composer + launcher deactivated as soon as the menu hides means LauncherStack prunes it after the 1s clear delay. If the user takes more than a second to pick a file, the forward navigation has no launcher to capture, so Back from the attachment preview will not restore focus to the composer + despite this flow being covered by the fix. Re-register or otherwise preserve the launcher when the picker returns before navigating.
Useful? React with 👍 / 👎.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product.
Explanation of Change
Fixed Issues
$ #96972
PROPOSAL:
Tests
Flow 1 — FAB Create Menu (Track Distance)
Flow 2 — Chat Composer Create Menu
Regression Checks
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web_chrome_mac.mp4
web_chrome.mp4