grahammendick / navigation

Scene-Based Navigation for React and React Native
https://grahammendick.github.io/navigation/
Apache License 2.0
571 stars 40 forks source link

Refreshing inactive tabs from another tab #784

Closed janwende closed 2 months ago

janwende commented 3 months ago

Hey,

First of all, thank you for this great library! Everything is working well for us, but we've encountered an issue after implementing theming in our React Native app. We are looking for a way to trigger a re-render of an inactive tab when the theme changes.

Let's say we change the theme on tab C; the theme updates correctly. However, if we then navigate to tab A, we see a brief flash because the old theme is still being rendered on tab A. The new theme only applies after the tab is selected, causing this flicker.

Initially, we used our own implementation of dynamic styles and recently switched to Unistyles, hoping it might resolve the issue, but the flash persists.

Is there a way to refresh an inactive tab to apply the new theme immediately?

I came across a comment from you in another issue which suggests that it might be possible to update an inactive tab (see below).

We've attempted to use Zustand and React Context to address this, but it doesn't seem to work when the tab is inactive. Is there something we could utilize within the NavigationHandler to achieve this?

Any help would be greatly appreciated. Thank you!


Your comment in the other issue:

          I'm confused. Do you want to update one tab from another tab? You can use React Context or a 3rd party state library for that. If that's not what you mean then please can you explain what you want to do in more detail, thanks

Originally posted by @grahammendick in https://github.com/grahammendick/navigation/issues/733#issuecomment-1817604868

grahammendick commented 3 months ago

Hey, thanks for getting in touch. It's great to hear you're enjoying the Navigation router!

The Navigation router freezes (using React Suspense) inactive tabs so that state updates aren't applied. There's no point React Native rendering content that isn't visible to the user. When the user presses a tab the Navigation router changes the content change immediately on the native side. It also sends an event to the JavaScript that unfreezes the tab. If the tab is a bit slow to render and there's a drastic change in content (like a theme change) then this can cause a flicker.

I've created a branch with a possible fix. Instead of updating the tab content immediately on the native side, it waits for the React render before updating the content. Could you try this fix out and let me know what you think, please? You can clone the repo, switch to that branch, run npm install and npm run package. Then copy the 'navigation-react-native' folder from build/npm into your 'node_modules'.

Please report back how it looks on both Android and iOS.

This fix is only an experiment at the moment. But If it works for you then I can think about whether it can be made production-ready.

janwende commented 3 months ago

Hey,

Thank you for your quick response and for providing a potential fix.

We have implemented the changes in our app as you suggested. We cloned the repository, switched to the branch with the fix, ran npm install and npm run package, and copied the ‘navigation-react-native’ folder into our ‘node_modules’. We also created a new Dev-Client to test the changes.

Unfortunately, the flashing issue still persists after these adjustments. So far, we have only been able to test this on iOS. We plan to test it on Android as well in the coming days and will report back with our findings.

Thank you again for your assistance. We appreciate your efforts in helping us resolve this issue.

grahammendick commented 3 months ago

Thanks for testing. I was hoping the fix would work on iOS but suspected it would only work on Android. So I look forward to your Android results.

I'll keep thinking about a solution that works on both platforms. Are you using useColorScheme to change the theme?

grahammendick commented 3 months ago

Also, do you see the flicker when you peek back on iOS - when you gesture back to move to the previous scene? (You can peek back on Android, too, if you're using the latest release)

janwende commented 3 months ago

We are currently using UniStyles for theming, and overall, we’re quite happy with it. Within UniStyles, we are leveraging UniStylesRuntime.setTheme() and const { styles, theme } = useStyles(stylesheet) instead of useColorScheme. Although we are generally satisfied with UniStyles, we are not strictly bound to it and could consider switching if necessary. However, our current preference is to continue using UniStyles in our project.

Regarding your question about peeking back on iOS: We actually tested this earlier when we had the theme change at a deeper navigation level. We noticed that when we slowly swipe back from the edge or navigate back, there is no flicker, and the previous page updates to the correct theme immediately. Or at least, it happened so fast that it wasn’t visible.

grahammendick commented 3 months ago

Could you pull the latest from the fix branch and retest on iOS, please? I've done a similar fix on iOS that I did for Android. So, instead of updating the tab content immediately on the native side, it waits for the React render before updating the content.

janwende commented 3 months ago

Hey,

First of all, I just want to say a huge thank you for the effort you’ve put into helping us. I’m super excited to report that your fix works perfectly! We can now switch between tabs seamlessly without any flashing. There’s no noticeable delay when switching tabs either. It looks flawless!

There’s just one tab which occasionally still shows a short (but much shorter) flash, but I think this is on us. We’ll investigate this further, but most of the time this tab also renders without flashing.

I’m incredibly happy that we’re using your library, and your support has been absolutely amazing. Thanks so much for everything!

We’ll definitely test this on Android too and get back to you with our findings. Unfortunately, we won’t be able to do this before the weekend.

grahammendick commented 3 months ago

Ok, that's great to hear! Thanks for the kind words, too 🙏

Please keep me informed about how the testing goes. Including anything you find out about the shorter flash.

I'm not sure if this is how the tabs should behave by default. My worry is that blocking the iOS tab on the native side is bad for accessibility - or maybe it's hiding some subtle feedback indicator that users expect. I wonder if I should make this behaviour opt-in in some way. So that you could turn it on after a theme change, for example. Then 99% of the time the Navigation router changes the tabs instantly on the native side. What do you think?

janwende commented 3 months ago

I wanted to let you know that I have also tested the fix on Android, and the flashing issue is resolved there as well. The tabs switch seamlessly without any flashing, just like on iOS.

Regarding your question about how to implement this, I agree that it might not be ideal to make this the default behavior for all tabs. Making it opt-in sounds like a good approach. Maybe just pass a prop to the TabBar, enabling the behavior when needed, such as after a theme change.

grahammendick commented 3 months ago

Thanks for the update. I'm really glad the fix is working on both platforms.

Just to set expectations, turning this idea into a production-ready solution won't be easy. There are top and bottom tabs to consider on iOS and Android and on the old and new architecture. In the meantime, if it's a blocker for you, you could use patch-package to set enabled to false on the Freeze component in the TabBarItem.

https://github.com/grahammendick/navigation/blob/2924de7ba3f2bb95f754fab9529516f76fe720c6/NavigationReactNative/src/TabBarItem.tsx#L34

grahammendick commented 3 months ago

I've made a start on making the fix production-ready and I'd appreciate if you could give it a retest. I'm still working in the same branch so pull the latest, please.

I've added a contentSync prop to the TabBar component. You need to set that to true when the user changes the theme. Then you reset it back to false when the tab changes.

const [contentSync, setContentSync] = useState(false);

<Button onPress={() => setContentSync(true)>Change Theme</Button> 

<TabBar contentSync={contentSync} onChangeTab={() => setContentSync(false)}>

I'm interested to hear if it still works on both iOS and Android, of course. But I'd also like to know if you're happy with the new prop and how it behaves?

grahammendick commented 3 months ago

I'm thinking of renaming the prop to preventFouc. Fouc is a term from the web (flash of unstyled content) but I think it works well here.

RichardLindhout commented 3 months ago

Nice potential change! FYI I'm seeing this too with the safe area component where the tab loads for the first time it flashes.

janwende commented 3 months ago

I have now integrated the latest changes and tested them. The contentSync feature works great. I implemented it using a state from Zustand, ensuring it only activates when necessary. Thank you for this!

I also think your suggestion to rename the prop to preventFouc is a great idea. It clearly communicates its purpose, even more so than contentSync.

Thank you again for your support and for continually improving this library!

grahammendick commented 3 months ago

Thanks for testing. That's great news! It gives me confidence that I'm moving in the right direction

grahammendick commented 3 months ago

@janwende Ok, I'm happy with the implementation on the old architecture. I've renamed the prop to preventFouc as we both agree that's clearer. Please would you give it another test and check it still works for you?

const [preventFouc, setPreventFouc] = useState(false);

<Button onPress={() => setPreventFouc(true)>Change Theme</Button> 

<TabBar preventFouc={preventFouc} onChangeTab={() => setPreventFouc(false)}>

I'll work on copying the changes over to the new architecture next. Things are always harder on the new architecture - that's why I try to solve all the problems on the old first.

grahammendick commented 3 months ago

@janwende I've finished the work from my side. Once you give me the green light I'll merge it in

@afkcodes you should check this out because I recall you had this problem

afkcodes commented 3 months ago

yes i was using a patch that you helped me with.

grahammendick commented 3 months ago

@janwende if you're too busy to retest this at the moment at least let me know you've seen this please 🙏

janwende commented 3 months ago

Hey, Sorry for the delay. I had already planned to test the change and wanted to get it done, but unfortunately, I haven't had the time yet. I’m hopeful I can test it either today or tomorrow. Thank you for your patience and the changes you've done! :)

grahammendick commented 3 months ago

@janwende no problem at all. Whenever you're free is cool. Just wanted to check you'd seen it. Thanks for letting me know

grahammendick commented 2 months ago

@janwende can you look at this soon, please?

janwende commented 2 months ago

Hey, I finally had the chance to test the changes, and I’m happy to report that everything works perfectly! Thank you so much for your efforts and patience. Your support has been fantastic, and I really appreciate all the hard work you’ve put into this.

grahammendick commented 2 months ago

Oh, that's great to hear. Thank you for raising this and for all your help with the testing. I'll thank you again in the release notes

grahammendick commented 2 months ago

@janwende I just released navigation-react-native v9.19.0 and thanked you in the release notes