PedroBern / react-native-collapsible-tab-view

A cross-platform Collapsible Tab View component for React Native
MIT License
831 stars 161 forks source link

When `headerHeight` is not set sometimes tab opacity remains at 0 #148

Closed alexpchin closed 2 years ago

alexpchin commented 3 years ago

Current behavior

When using dynamic tabs and you don't add headerHeight, then sometimes (not every time) the Tabs will appear empty because the opacity will still be 0. Please see video:

blank-tab.mov.zip

If you add any value for headerHeight, i.e. 1 the content will visibly adjust but the content will be visible.

Expected behaviour

You should not see the white screen.

andreialecu commented 3 years ago

Tested this in the Example app and was not able to reproduce it.

Please share a repro.

alexpchin commented 3 years ago

Hi, @andreialecu

I've been working to try to resolve this today... Basically, what I was seeing was that (only sometimes) the pager's opacity visibly remains at 0.

(I can see the outline of the content in the simulator inspector).

Screenshot 2021-02-27 at 15 55 03

After doing some logging, I found that it happens when I see:

initialHeaderHeight --> undefined
initialTabName --> undefined
headerHeight --> undefined
pagerOpacity.value --> 0
update --> false

In the code below (348):

// fade in the pager if the headerHeight is not defined
    useAnimatedReaction(
      () => {
        return (
          (initialHeaderHeight === undefined || initialTabName !== undefined) &&
          headerHeight !== undefined &&
          pagerOpacity.value === 0
        )
      },
      (update) => {
        if (update) {
          pagerOpacity.value = withTiming(1)
        }
      },
      [headerHeight]
    )

Followed shortly after by something like:

initialHeaderHeight --> undefined
initialTabName --> undefined
headerHeight --> undefined
pagerOpacity.value --> 0.00617283975268588

Which I think means there is a race condition between the logic there and in Container line 214

pagerOpacity.value = withTiming(1)

I think line 214 can just be removed? Which will remove the race condition and fix the bug?

alexpchin commented 3 years ago

Hi @andreialecu There seems to be a scenario where headerHeight is undefined but pager.opacity is not 0 so the pager stays at 0 opacity due to the false return in useAnimatedReaction? Does withTiming(1) start at 0 and conclude at 1? Should this just be 1?

alexpchin commented 3 years ago

Hi @PedroBern @andreialecu I'm still seeing this problem in my app when headerHeight is not set. I don't actually think it's to do with dynamic tabs as I can replicate the behaviour without dynamic tabs.

If I have no HeaderComponent at all, it doesn't happen. Only if the HeaderComponent is set but no headerHeight is provided.

I've tried to replicate it in an example app but I'm really struggling to replicate it outside my app.

samih7 commented 3 years ago

We have the exact same issue in our application, without dynamic tabs. Seems to occur on Android most often.

alexpchin commented 3 years ago

@samih7 I've tried to replicate the behaviour in a repo but I can't seem to?

https://github.com/alexpchin/rnctv-react-native-bare-test/pull/1

PedroBern commented 3 years ago

I can't reproduce it. But maybe having a single layout event callback for taking the height of the header/tabbar/container help, instead of having 3. I did it here https://github.com/PedroBern/react-native-collapsible-segmented-view/blob/9a2e5006572d09ae0582454919edb4af7583949b/src/SegmentedView.tsx#L82 a refactoring should not be hard. Does someone would like to give it a try?

alexpchin commented 3 years ago

@PedroBern I've tried to reproduce it... In my app, it's really quite obvious. It's making me 🤯 trying to work out why I can't replicate it in a separate repo.

If no headerHeight is provided, then:

Container 92 would mean headerHeight is undefined.

const [headerHeight, setHeaderHeight] = React.useState<number | undefined>(
  !HeaderComponent ? 0 : initialHeaderHeight
)

Until the height of the header is calculated:

Container 322.

const getHeaderHeight = React.useCallback(
      (event: LayoutChangeEvent) => {
        const height = event.nativeEvent.layout.height
        if (headerHeight !== height) {
          setHeaderHeight(height)
        }
      },
      [headerHeight]
    )

This should trigger the page to fade in with:

Container 349.

useAnimatedReaction(
      () => {
        return (
          (initialHeaderHeight === undefined || initialTabName !== undefined) &&
          headerHeight !== undefined &&
          pagerOpacity.value === 0
        )
      },
      (update) => {
        console.log('update --->', update)
        if (update) {
          pagerOpacity.value = withTiming(1)
        }
      },
      [headerHeight]
    )

However, without headerHeight, sometimes update will always be false because pagerOpacity.value === 0

I believe this is because pagerOpacity.value is set on Container 213.

useAnimatedReaction(() => {
    return afterRender.value === 1;
  }, trigger => {
    if (trigger) {
      afterRender.value = 0;
      tabNamesArray.forEach(name => {
        'worklet';

        scrollToImpl(refMap[name], 0, scrollY.value[index.value] - contentInset, false);
      });
      pagerOpacity.value = withTiming(1);
    }
  }, [tabNamesArray, refMap, afterRender, contentInset]);

As per https://github.com/PedroBern/react-native-collapsible-tab-view/pull/149/commits/9da382079716f2d23a9ea33b90238e3344598f27 if I comment out that line then it seems to fix my problem but @andreialecu says that it breaks the dynamic tabs example?

https://github.com/PedroBern/react-native-collapsible-tab-view/pull/149#issuecomment-787176663

atitpatel commented 2 years ago

This is happening with our app as well. I can even click the list items but the opacity is 0 and the use can only see a white screen. I am at the latest version of the library. Is there a solution which I can use for this.

andreialecu commented 2 years ago

This should be resolved in v5, try installing it via yarn add react-native-collapsible-tab-view@rc