fix: device back button doesn't close FAB menu#82977
fix: device back button doesn't close FAB menu#82977arosiclair merged 7 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
jayeshmangwani and
joekaufmanexpensify
and removed request for
a team
|
@jayeshmangwani 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] |
![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: dd38317347
ℹ️ 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".
| if (!(window.history.state as WindowState)?.shouldGoBack) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This early return skips history.back() whenever a menu action has already navigated, but those actions typically use Navigation.navigate, which pushes a new browser entry by default (src/libs/Navigation/helpers/linkTo/index.ts:159-161). That means the synthetic shouldGoBack entry created when the modal opened is never popped, so the history stack keeps an extra duplicate route and users need an extra Back press (or appear to go back to the same page) after navigating from the FAB menu.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@daledah , I tested a few FAB navigation cases but couldn’t reproduce the issue with pressing back twice as GPT suggested above. Could you confirm on your side as well?
There was a problem hiding this comment.
@daledah Please check the comment #82977 (comment) above⬆️.
There was a problem hiding this comment.
This review was above, so I missed it. I'm checking again now.
There was a problem hiding this comment.
@daledah , I tested a few FAB navigation cases but couldn’t reproduce the issue with pressing back twice as GPT suggested above. Could you confirm on your side as well?
I've retested multiple times and couldn't reproduce the extra back-press issue
@daledah can you explain this change a bit more? It looks like this is a fix for #81239. What was the root problem there and how does this fix it?
@arosiclair
Rootcause: When shouldHandleNavigationBack is enabled, opening the FAB menu calls window.history.pushState({shouldGoBack: true}). When the modal closes, hideModal calls window.history.back() — which fires a popstate event asynchronously. If a menu item uses shouldCallAfterModalHide: true, its onSelected callback runs via onModalDidClose() synchronously right after history.back() is called, but before the popstate fires.
=> We should call onModalHide() first (so onModalDidClose() → menu callback → navigation runs synchronously), then defer history.back() via setTimeout(0) => the navigation callback has already pushed a new history state, so window.history.state.shouldGoBack is no longer true - and the history.back() call is skipped by the guard check.
There was a problem hiding this comment.
Rootcause: When shouldHandleNavigationBack is enabled, opening the FAB menu calls window.history.pushState({shouldGoBack: true}). When the modal closes, hideModal calls window.history.back()
So hideModal always calls window.history.back() even when we're not closing the menu using the back button? Is there a way to just prevent that in the non-back-button flow? Or would it be easier to implement shouldHandleNavigationBack without using window.history.pushState?
There was a problem hiding this comment.
So hideModal always calls window.history.back() even when we're not closing the menu using the back button?
history.back() is not always called. When the modal is closed via the browser back button, popstate fires first - which already pops the {shouldGoBack: true} guard entry from the history stack. By the time hideModal runs, window.history.state.shouldGoBack is already false, so it falls to the else branch and calls onModalHide() directly. No history.back() needed.
Or would it be easier to implement shouldHandleNavigationBack without using window.history.pushState?
The push state or pop state pattern is the only web API that allows intercepting the browser's back button before it navigates. So pushState is necessary for this approach.
There was a problem hiding this comment.
Ah okay I see. Thanks for explaining. This LGTM 👍
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
bump here @jayeshmangwani |
|
@daledah Sorry, I missed this new PR. I’m checking it now. |
| // callbacks (from onModalDidClose) have pushed their history entries. | ||
| // This prevents the popstate from undoing navigations triggered by | ||
| // menu item selection callbacks. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
@daledah , is there another way we can avoid adding a new setTimeout here?
There was a problem hiding this comment.
@jayeshmangwani There's no synchronous alternative here - popstate is inherently async by spec, so setTimeout(0) is the minimal and most predictable way to yield
There was a problem hiding this comment.
@arosiclair, we added zsetTimeout(0)` as a workaround to fix this issue #81239. Are we fine keeping it, or should we open the main issue to find a better solution?
There was a problem hiding this comment.
Sorry, @daledah , could you clarify what you want me to confirm here?
There was a problem hiding this comment.
Sorry @jayeshmangwani. I missed the comment above. I'm checking now
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.mov |
|
🚧 @arosiclair has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.3.40-0 🚀
|
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.3.40-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #70951
#81239
PROPOSAL: #70951 (comment)
Tests
Offline tests
Same as tests
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
Android: Native
android.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov
Screen.Recording.2026-02-20.at.00.36.10.mov