New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] perf: native-stack (v2) #37891
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
e178a2d
to
73870c2
Compare
@cubuspl42 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] |
@mountiny I think this PR is ready for review 👀 And from my testing experience - the second version feels much more stable than initial implementation 💪 |
@kirillzyusko for easier QA could you please list the test scenarios from the regressions in the PR body? QA will then have easier life once this hits staging |
@cubuspl42 Createing a new build, will you be able to test this one tomorrow? |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.ts
Outdated
Show resolved
Hide resolved
A tough one to review meaningfully. I left one minor comment. |
@kirillzyusko I would also appreciate some summary of the testing steps! Especially if you redacted them for clarity. |
@mountiny @cubuspl42 I update test steps 🙌 But you also can test any scenarios that you would like to test 👀 |
What I observe: on iOS Native it's right-to-left, on Android Native it's left-to-right. Is this expected? If so, the testing steps should specify it. |
@cubuspl42 I created a patch for that functionality on iOS, so you'll need to reinstall |
My bad! I did as you said, works as expected now in that aspect. |
Interestingly, on Android I'm able to go back using a gesture from both sides of the screen. Is it expected? native-stack-v2-android-compressed.mp4Actually, always takes two tries. Doesn't feel right. |
@cubuspl42 Yes. Actually this is system UI and yes, it can be triggered from any horizontal side of the app (i. e. left or right). And it's simply simulation of hardware/software button press (these buttons were widely adopted in Android < 10), but in Android 11 they decided to make navigation bar smaller and replace all buttons (back button/recent apps/home) with gestures. So when you perform gesture one time - it'll close a keyboard. When you do it a second time - it'll close a screen. So this is handled on OS level and it's not something that we can prevent (though we can override an action on the gesture and prevent screen from being hidden, but I wouldn't recommend to do that because many android users are using these system gestures to do a quick navigation in the app). |
I understand! Makes sense. |
Thanks for chugging along, @cubuspl42 what is your ETA on this one? |
I'm resuming the testing, ETA should be < 4h unless something bad is found |
You wrote "Not affected" in the Screenshots/Videos on non-Native platforms. Please don't do so in the future in cases like this. Your intention was not to affect these platforms, it's not clear from the change itself that they are not affected. On a related, but separate topic, the "Tests" sections should be written according to Expensify's "cross-platform 99% of the time" philosophy. Pragmatically speaking, you should note which steps are platform-specfic and you should cover all platforms. Even when it's something as simple as "Ensure that this and this and this interaction (still) works". |
Asking about this PR specifcially now: from which side should the search panel enter the screen on mobile web, by design and intention? |
On iOS Web, the automatic keyboard opening doesn't work for me. It's not clear if this is a regression or an unrelated known issue. Would you please clarify it? native-stack-v2-ios-web-compressed.mp4 |
They compare quite well in my opinion. This is Android on Since the animations are quite similar and this PR paths the way for all future work on platform stack navigation, do you think we can still merge this? (seems that some pages/logic in E/App is not quite 100% adapted to native-stack yet. iOS animations look a bit weird) Screen.Recording.2024-04-26.at.00.27.45.movRPReplay_Final1714084540.MP4 |
Hey 👋 I did testing of app side by side (iOS for now, will test Android later) and here is a list of all bugs that I discovered:
|
native-stack | js stack |
---|---|
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-26.at.14.30.28.mp4 |
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.14.31.38.mp4 |
It happens because you press on a "card" (chat). And when you release a finger, then navigation hapens to this particular screen. But if you simply press on this card then you will also see a white screen.
got fixed after pulling in main branch 🤷♂️
2. Double navigation after back gesture (software-mansion/react-native-screens#2118)
native-stack | js stack |
---|---|
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-26.at.14.26.40.mp4 |
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.14.28.57.mp4 |
It happens because you press on a "card" (chat). And when you release a finger, then navigation hapens to this particular screen.
3. Random white flash on back transition
Click me to see a bug
native-stack | js stack |
---|---|
random-white-flash.mov |
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.14.37.33.mp4 |
got fixed after pulling in main branch 🤷♂️
4. Avatar incorrect back transition
Click me to see a bug
native-stack | js stack |
---|---|
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-26.at.14.39.35.mp4 |
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.14.40.21.mp4 |
Fixed via patch modification + unmounting screen after transition completion + reverting screen options.
5. Random screen clearance on back transition (maybe related to white flash on back transition)
Click me to see a bug
native-stack | js stack |
---|---|
random-screen-clearance.mov |
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.14.48.24.mp4 |
got fixed after pulling in main branch 🤷♂️
6. Empty screen gets pushed
Click me to see a bug
native-stack | js stack |
---|---|
Screen.Recording.2024-04-26.at.14.55.27.mov |
Screen.Recording.2024-04-26.at.15.00.20.mov |
We show spinner initially, so technically it's not an empty screen (and the same problem now is present in js-stack too).
7. Submit expense/assign task/pay someone/track expense/etc. leads to white screen
got fixed after pulling in main branch 🤷♂️
Warning
The correct priority would be to fix issues 1, 2, 7 (to understand why white screen gets shown and how to fix it).
And here is Android part
|
App from market | With native stack |
---|---|
telegram-cloud-document-2-5294459555042838618.mp4 |
telegram-cloud-document-2-5294459555042838604.mp4 |
3. Keyboard handling is broken (sometimes) (opened issue - software-mansion/react-native-screens#2124 Expensify/react-native-live-markdown#346)
App from market | With native stack |
---|---|
I'll go through all issues and will try to fix them (or will create a separate issues in associated repositories in case if I can not fix them).
@kirillzyusko @chrispader I agree that the latest videos you have showed are quite similar for both ios and android so that would be good to go imho @kirillzyusko great testing! definitely lets try to catch as many of these as possible |
@mountiny sure! 😎 |
It would be perfect to submit issues (and hopefully fix them) in the |
@mountiny @WoLewicki I fixed all the issues that were fixable in Expensify codebase and for all external bugs I opened issues on GitHub in I think the next step would be to pause the work in the current PR and wait until all reported issues will be fixed and then continue a work in this PR. Let me know if you want me to look into opened issues or if you want SWM guys to look into it 😊 |
Thanks Kiril, that sounds great! @WoLewicki Will you be able to handle the reported issues in SWM? |
Sure! @kirillzyusko can you list all of them here? |
Could you check if software-mansion/react-native-screens#2134 makes it all work correctly? |
Hey @WoLewicki I am OOO this week, so can verify only on the next week. @chrispader maybe you can check in a meantime (if you have a capacity)? |
I'm also pretty busy this week (also because of App.js Conf), so i could work on it next week at the earliest. |
Details
Change for the mobile platforms to use the native-stack navigator:
Native Stack Navigator provides a way for your app to transition between screens where each new screen is placed on top of a stack.
With this we will get native screen transitions, which feel better and don't get blocked if the JS thread is blocked.
This was discussed here: #vip-newdot-performance
Warning
This is a second revision of the implementation. The first one caused a lot of issues, especially when it comes to keyboard appearance on iOS, transition events detection on Android. A successor of #29991
The main difference comparing to the first version of implementation is the fact that now we use native-stack everywhere (before on different stack navigators level we were using different navigators). For that we had to patch some libraries:
PRs from patches:
slide_from_left
transition software-mansion/react-native-screens#2057 <- addedslide_from_left
transition for Search/Workspace pagesInteractionManager
withnative-stack
react-navigation/react-navigation#11887 <- fixed an integration withInteractionManager
(used inuseSingleExecution
hook).Fixed Issues
$ #37923
$ #29948
PROPOSAL: #29948
These issues were spotted in the first revision of implementation, so it's also important to test them:
$ #37354
$ #37257
$ #37252
$ #37273
$ #37156
$ #37325
Tests
Offline tests
Not needed.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
telegram-cloud-document-2-5442872807086113148.mp4
Android: mWeb Chrome
telegram-cloud-document-2-5449560084050887588.mp4
iOS: Native
76cd68bb-6ef8-46d7-9609-cf0ad3964289.mp4
iOS: mWeb Safari
5aa0fca4-72a3-4441-a56f-57da3d095883.mp4
MacOS: Chrome / Safari
a8d30d7e-8aad-4ac0-affe-83ab19584109.mp4
MacOS: Desktop
f6fe3999-d45a-49a2-9e6a-cc5c8e5764d3.mp4