React Native v0.43 ViewPagerAndroid work not well when detached then attach

[liuzhibopp]

Description

Integration With Existing Apps
on 0.42 ViewPagerAndroid work well when detached then attach , after upgrading to React Native v0.43 when i slide one page to other page ，no smooth scroll and stop in middle ,i compared two version of react native.

Reproduction Steps and Sample Code

0.42 when i slide viewpager following codes will work and 0.43 there no updateLayout for ViewPagerAndroid :
java.lang.Thread.State: RUNNABLE at android.support.v4.view.ViewPager.onLayout(ViewPager.java:1705) at android.view.View.layout(View.java:16980) at android.view.ViewGroup.layout(ViewGroup.java:5550) at com.facebook.react.uimanager.NativeViewHierarchyManager.updateLayout(NativeViewHierarchyManager.java:199) at com.facebook.react.uimanager.NativeViewHierarchyManager.updateLayout(NativeViewHierarchyManager.java:184) at com.facebook.react.uimanager.UIViewOperationQueue$UpdateLayoutOperation.execute(UIViewOperationQueue.java:124) at com.facebook.react.uimanager.UIViewOperationQueue$2.run(UIViewOperationQueue.java:784) at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:831) - locked <0x1e9d> (a java.lang.Object) at com.facebook.react.uimanager.UIViewOperationQueue.access$1500(UIViewOperationQueue.java:45) at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:870) at com.facebook.react.uimanager.GuardedChoreographerFrameCallback.doFrame(GuardedChoreographerFrameCallback.java:32) at com.facebook.react.uimanager.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:131) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:925) at android.view.Choreographer.doCallbacks(Choreographer.java:718) at android.view.Choreographer.doFrame(Choreographer.java:650) at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:913) at android.os.Handler.handleCallback(Handler.java:739) at android.os.Handler.dispatchMessage(Handler.java:95) at android.os.Looper.loop(Looper.java:148) at android.app.ActivityThread.main(ActivityThread.java:5660) at java.lang.reflect.Method.invoke(Method.java:-1) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

Solution

I don‘t know whether there is a relationship with the following changes ：

• Only call onLayout when layout has actually changed (15429e3)

how to fix ?
thanks in advance!

[henrytao-me]

Hi @liuzhibopp ,

I ended up writing this custom component to wrap ViewPagerAndroid. It works on my side. Please check.

ViewPagerAndroidContainer

import React, { PureComponent } from 'react'
import { View } from 'react-native'

class ViewPagerAndroidContainer extends PureComponent {

  state = {
    width: 0,
    height: 0
  }

  render() {
    return (
      <View style={[this.props.style]} onLayout={this._onLayoutChange}>
        <View style={{ width: this.state.width, height: this.state.height }}>{this.props.children}</View>
      </View>
    )
  }

  _onLayoutChange = (e) => {
    const { width, height } = e.nativeEvent.layout
    this.setState({ width: width, height: height })
  }
}

export default ViewPagerAndroidContainer

Usage

<ViewPagerAndroidContainer style={{ flex: 1 }}>
  <ViewPagerAndroid ref='viewPager' style={{ flex: 1 }} initialPage={selectedIndex} onPageSelected={this._onPageSelected}>
    ...
  </ViewPagerAndroid>
</ViewPagerAndroidContainer>

[liuzhibopp]

hi @henrytao-me,
Sorry for so long to reply to you，I test just now and it does not work for me. I try to extended ViewPager modifies the mFirstlayout property by reflection，it works for me.
I hope to have a better way for the issue，Thanks for your answer.

[satya164]

This issue seems to provide some insight to the bug expo/ex-navigation#415 (comment)

[sibelius]

I can confirm this

[bood]

Met similar issue when adding pages to ViewPagerAndroid dynamically
Problem does seems to point to 15429e3, not sure if the commit is still necessary after Yoga fixes @astreet ?
layout of ReactViewPager is no longer invoked after the change in this case. However, newly added ReactViewGroup children of ReactViewPager requires the layout to be called to be properly layout-ed.

A temp working fix is to by-pass the optimization for ReactViewPager in dispatchUpdates (by comparing mViewClassName). You can try it out too.

I'll open a separate issue if needed.

[astreet]

Yeah, we're not handling layout correctly here: the view should be given the ability to lay itself out (and its children) if it so requests. However, we're able to bypass a lot of extra work since the majority of views don't use this ability.

My first thought it to try to solve this by giving the view manager the ability to request the UIManager to layout the corresponding view in the next dispatchUpdates (e.g. exposing a new command on UIManager)

[Jackoder]

This is my temp working fix:

• Hack ReactViewPager to modify mFirstLayout member

import android.support.v4.view.ViewPager;

import com.facebook.react.bridge.ReactContext;
import com.facebook.react.views.viewpager.ReactViewPager;

import java.lang.reflect.Field;

public class FixReactViewPager extends ReactViewPager {

    boolean hasLayout;

    public FixReactViewPager(ReactContext reactContext) {
        super(reactContext);
    }

    @Override
    protected void onLayout(boolean changed, int l, int t, int r, int b) {
        super.onLayout(changed, l, t, r, b);
        hasLayout = true;
    }

    @Override
    protected void onAttachedToWindow() {
        super.onAttachedToWindow();
        if (hasLayout) {
            Class<ViewPager> clazz = ViewPager.class;
            try {
                Field field = clazz.getDeclaredField("mFirstLayout");
                field.setAccessible(true);
                field.set(this, false);
                field.setAccessible(false);
            } catch (NoSuchFieldException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            }
        }
    }
}

• Inject Manager with the same name AndroidViewPager

import com.facebook.react.uimanager.ThemedReactContext;
import com.facebook.react.views.viewpager.ReactViewPager;
import com.facebook.react.views.viewpager.ReactViewPagerManager;

public class FixReactViewPagerManager extends ReactViewPagerManager {

    public String getName() {
        return "AndroidViewPager";
    }

    @Override
    protected ReactViewPager createViewInstance(ThemedReactContext reactContext) {
        return new FixReactViewPager(reactContext);
    }
}

[itinance]

The workaround from @henrytao-me didn't work for us, but we could fix the issue on RN 0.45.1 with the following workaround: for those cases, when we want to replace the already existing ViewPager with new children, we enforce a re-render while passing a id-property:

const id= ...something to create an id that reflacts the state of the pages and will change every time when pages will change

        <IndicatorViewPager
          id={id}
          ref={(c) => this.setViewPagerRef(c, id)}
          style={{width: DEVICE_WIDTH, height: slideHeight}}
          pagerStyle={{width: DEVICE_WIDTH, height: slideHeight}}
          initialPage={0}
          indicator={this.renderDotIndicator(slideCount)}
          onPageSelected={this.handlePageSelected}
          >
          {slides}
        </IndicatorViewPager>

[cbjs]

+1

[INCHMAN1900]

@henrytao-me you answer works for me, but there is more work to do.
If you nest viewPagerAndroid in a TabNavigator, you need to set swipeEnabled and animationEnabled to true, at least one of them. If they're both false , then viewPagerAndroid will work very slowly, and get stuck between the pages.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

[liuzhibopp]

New work not using RN for a long time, so forget to care this issue，I've already put forward a method that modifies the # mFirstlayout property by reflection to slove the issue ，like @Jackoder ，I hope this problem has been solved in an elegant way.

[Noitidart]

I copied pasted the docs example of ViewPagerAndroid but it doesnt work for me. It doesnt render its children it seems, its mind boggling:

            <View style={styles.main}>
                <ViewPagerAndroid style={styles.viewPager} initialPage={0}>
                    <View style={styles.pageStyle}>
                        <Text>First page</Text>
                    </View>
                    <View style={styles.pageStyle}>
                        <Text>Second page</Text>
                    </View>
                </ViewPagerAndroid>
            </View>

    main: {
        flex: 1
    },
    viewPager: {
        height: 100,
        backgroundColor: 'green'
    },
    pageStyle: {
        alignItems: 'center',
        padding: 20,
        width: 100,
        height: 100,
        backgroundColor: 'green',
        flex: 1
    }

Anyone have any ideas? It just shows a 100 height green box.

[Noitidart]

Wow @henrytao-me thanks for ViewPagerAndroidContainer it makes it work. So weird. However peekEnabled is not working, do you know how to get that to work?

[danilobuerger]

There is a fix here: #14867
A shame that nobody seems to care to get this merged.

[satya164]

@danilobuerger

You can see that I asked for a test plan, but the author didn't respond. I can't merge things without having a way to test it if it actually works and doesn't break anything. I don't see how nobody cares to get this merged.

[danilobuerger]

@satya164 So if the original author disappeared, we just wait for a test plan forever?

[satya164]

Someone else can send a PR. What do you want me to do? Merge without testing?

[danilobuerger]

@satya164 I didn't ask you to do anything. But since you are responding, and since you submitted a similar PR, wouldn't it make sense for you to write a test? If it's just about having a project where you can see that the PR actually works, I can supply you with one.

[danilobuerger]

For anybody else running into this issue: I have created a fork here: https://github.com/feastr/react-native and will be maintaining this (and porting to new versions whenever released) until the issue is fixed upstream. To use, just replace the react-native dependency in your package.json with:

"dependencies": {
  "react-native": "feastr/react-native#0.51-stable"
}

[satya164]

I don't need a written test, just a way to test if the change actually works. If you have one, please send a PR with a minimal example where I can check if it's actually fixed.

I could write a test, but I don't have a lot of free time. And since someone sent a PR, they probably already have a test plan.

[danilobuerger]

@satya164 I will use the already existing PR #14867 and comment a test plan there.

[satya164]

@danilobuerger sounds good

[bood]

@satya164 @danilobuerger But the fix does not seems to solve the problem of dynamic adding a page to ViewPager, code sample below:

import React, { Component } from 'react';
import { View, Text, ViewPagerAndroid } from 'react-native';

export default class DebugViewPager extends Component {
  constructor(props) {
    super(props);
    this.state = {
      npage: 2,
    };
  }
  render() {
    let pages = []
    for (var i = 1; i <= this.state.npage; i++) {
      let bg = (i % 2 == 0) ? 'blue' : 'yellow';
      pages.push(
        <View style={[styles.pageStyle], [{backgroundColor: bg}]} key={i}>
          <Text>Page {i}</Text>
        </View>
      )
    }
    viewPager = (<ViewPagerAndroid
             style={styles.viewPager}
             initialPage={0}>
      {pages}
    </ViewPagerAndroid>);
    return (
      <View style={styles.viewPager}>
        {viewPager}
        <Text style={styles.add} onPress={()=>{
            this.setState({npage: this.state.npage + 1})
        }}>
          Add Page
        </Text>
      </View>
    )
  }
}

var styles = {
  viewPager: {
    flex: 1,
    backgroundColor: 'white',
  },
  pageStyle: {
    alignItems: 'center',
    padding: 20,
  },
  add: {
    height: 100,
    width: 100,
    backgroundColor: 'red'
  }
}

[satya164]

Thanks for the comment. Can you copy/paste the comment on the linked PR too?

[bood]

Done

[hramos]

#14867 merged.