Fix Viewpager on Android when using native navigation. #14867

ruiaraujo · 2017-07-06T13:52:50Z

See the "broken" video attached to really understand the problem easily.

On Android after navigating to any other screen using wix navigation library, the native viewpager would lose the settling page behaviour which is quite annoying for the users.

This is caused by the onAttachedToWindow that resets mFirstLayout to true inside ViewPager. By request another layout pass, everything works as expected.

Working video is the application with patched RN.

broken.mp4
working.mp4

javache · 2017-07-06T18:23:55Z

cc @achen1

ruiaraujo · 2017-07-17T18:40:24Z

@javache Any chance of getting this reviewing this?

andrerfneves · 2017-08-11T17:07:16Z

@ruiaraujo this doesn't seem to fix my issue while using React Native Navigation. The issue is the same though. The viewpager loses the layout.

facebook-github-bot · 2017-09-18T17:02:34Z

@ruiaraujo I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

pull-bot · 2017-09-21T12:30:33Z

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

satya164 · 2017-10-08T17:06:38Z

@andrerfneves how did you test this?

facebook-github-bot · 2017-10-21T17:03:55Z

@ruiaraujo I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale · 2017-12-20T17:43:40Z

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.

danilobuerger · 2017-12-28T17:07:46Z

@satya164 Test Plan:

Checkout https://github.com/danilobuerger/react-native-viewpagerandroid-test
Run yarn
Run react-native run-android
Follow Instructions provided in App
Provide patched react-native aar
Run react-native run-android
Follow Instructions provided in App, notice how issue doesn't happen anymore.

bood · 2018-01-04T03:02:12Z

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'
  }
}

danilobuerger · 2018-01-04T08:38:48Z

@bood Why does this PR need to fix your unrelated problem? This fix is about:

Fix Viewpager on Android when using native navigation.

bood · 2018-01-04T09:59:03Z

That's embarrassing if it is not related at all.
I was asked to paste it here as #13463 seems to be related to the my issue, and this PR is intended to fix the issue? no?

danilobuerger · 2018-01-04T10:01:59Z

My understanding of that issue is that it only talks about losing smooth scrolling after detach and reattach.

danilobuerger · 2018-01-04T10:03:39Z

@satya164 Do you need anything else to get this merged?

satya164 · 2018-01-04T10:05:06Z

@danilobuerger I'll test it when I get time. holidays just got over

ruiaraujo · 2018-01-04T10:38:15Z

@danilobuerger Thanks for following through, I have changed topic quite dramatically so it was hard for me to provide a test plan easily.

danilobuerger · 2018-01-18T10:06:58Z

@satya164 I am frustrated. It has been 3 weeks since I made the test plan for you. When I posted:

A shame that nobody seems to care to get this merged.

You immediately jumped in and objected:

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.

A few days later you had your test plan. You have everything you need. Was it just a trick? Did you just want me to get off your back? Do you really not care? If so, please tell me if there is somebody else that is willing to take a look at this and get it merged.

satya164 · 2018-01-18T16:22:59Z

@danilobuerger that's a shitty way to treat maintainers. listen, I contribute to React Native in my free time. and I haven't gotten the time to test the PR. even PRs on my own repos haven't been reviewed. it's been only 14 days. chill out, and maybe have some empathy for other people.

danilobuerger · 2018-01-18T17:11:24Z

@satya164 it has been 21 days, not 14 days. Why do you think it was 14 days ago when I submitted the test plan?

I do have empathy, that's why I am asking if you know of anybody else who can get it merged as you clearly don't have time to do it. And personally, I don't think my attitude towards you is "shitty", I just want to get things done.

Rather what you are doing has a really chilling effect on new contributors. You are scaring them away, they are putting hard work into the PR, they are really trying and then they just get the cold shoulder. Why should anybody new contribute, if they get the feeling that things won't get merged?

andrerfneves · 2018-01-18T17:23:48Z

@danilobuerger @satya164 is but a contributor to the repo. He takes his free time to try and move the technology forward. He's not paid for it. It can take 1 day, 14 days, 21 days, or 6 months, it doesn't matter. If you'd like to complain about the RN open source repository as a whole, including how PRs are getting merged (or not), complain to Facebook directly.

I understand your frustration, dealt with a lot of this myself in the past. But it's the nature of open source software, especially really large packages/ecosystems like RN.

brunolemos · 2018-01-21T01:44:15Z

@danilobuerger I understand your frustration because I also have an open PR on this repo that's just a single line change and it's been open for more than 40 days. I also agree that this push away new contributions. But I understand how open source works so I try not to be hard on maintainers.

What I recommend for you is what I do on these situations: use a fork or, better, use the awesome patch-package to apply your fixes without a fork.

danilobuerger · 2018-01-24T22:50:50Z

@brunolemos Thanks for the tip. However, how would you use patch-package in that case? I guess you would have to always recompile from source since you can't use the aar?

brunolemos · 2018-01-24T23:36:13Z

If you can find this file inside node_modules, it will work. Just edit it with this PR changes and follow the instructions in the patch package readme.

danilobuerger · 2018-01-24T23:50:24Z

@brunolemos I just tried it out and as I thought, it doesn't work as the build process uses the aar and not the source.

brunolemos · 2018-01-25T00:13:14Z

Never tried this but if you edit the aar patch package might be able to patch it.

https://stackoverflow.com/a/38861263/2228575

satya164 · 2018-02-06T14:31:56Z

cc @hramos tested and works fine. can we get this merged?

hramos · 2018-02-08T01:14:34Z

@ruiaraujo please rebase and I can look into getting this imported. Thanks!

ruiaraujo · 2018-02-08T08:28:48Z

@hramos done!

pull-bot · 2018-02-08T08:29:25Z

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

facebook-github-bot · 2018-02-08T08:29:48Z

Something went wrong executing that command, @mkonicek could you take a look?

satya164 · 2018-03-05T13:32:49Z

@hramos friendly ping!

facebook-github-bot

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: See the "broken" video attached to really understand the problem easily. On Android after navigating to any other screen using wix navigation library, the native viewpager would lose the settling page behaviour which is quite annoying for the users. This is caused by the onAttachedToWindow that resets mFirstLayout to true inside ViewPager. By request another layout pass, everything works as expected. Working video is the application with patched RN. [broken.mp4](https://github.com/facebook/react-native/files/1128028/broken.mp4.zip) [working.mp4](https://github.com/facebook/react-native/files/1128032/working.mp4.zip) Closes facebook/react-native#14867 Differential Revision: D7154981 Pulled By: hramos fbshipit-source-id: 2b3570800a5320ed2c12c488748d9e1358936c84

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 6, 2017

chirag04 requested a review from javache July 6, 2017 14:46

javache removed their request for review July 6, 2017 18:24

ruiaraujo mentioned this pull request Sep 1, 2017

Fix ViewPager being stuck in some scenarios #15376

Closed

javache requested a review from aaronechiu October 11, 2017 10:43

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 20, 2017

danilobuerger mentioned this pull request Dec 24, 2017

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

Closed

stale bot closed this Dec 27, 2017

satya164 reopened this Dec 28, 2017

stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 28, 2017

satya164 self-assigned this Dec 28, 2017

satya164 approved these changes Feb 7, 2018

View reviewed changes

Fix Viewpager on Android when using native navigation.

33f825d

ruiaraujo force-pushed the viewpagerFixes branch from e32824a to 33f825d Compare February 8, 2018 08:28

facebook-github-bot added the Failed Commands label Feb 8, 2018

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 5, 2018

facebook-github-bot approved these changes Mar 5, 2018

View reviewed changes

facebook-github-bot closed this in a1295e1 Mar 5, 2018

ruiaraujo deleted the viewpagerFixes branch March 5, 2018 19:53

ghoshabhi mentioned this pull request Mar 6, 2018

[Help] [Android only] Tab Switching freezes after a new screen is pushed from the tabbed view satya164/react-native-tab-view#474

Closed

jenswellmeyer mentioned this pull request Mar 6, 2018

Toggling tabs not working after poping back in Android ptomasroos/react-native-scrollable-tab-view#839

Closed

danilobuerger mentioned this pull request Mar 28, 2018

ViewPagerAndroid: measure and layout after adding a view to the container #18609

Closed

madhu314 mentioned this pull request Apr 7, 2018

Bug when another component opened and then backed to this one madhu314/react-native-tabbed-view-pager-android#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Viewpager on Android when using native navigation. #14867

Fix Viewpager on Android when using native navigation. #14867

ruiaraujo commented Jul 6, 2017

javache commented Jul 6, 2017

ruiaraujo commented Jul 17, 2017

andrerfneves commented Aug 11, 2017

facebook-github-bot commented Sep 18, 2017

pull-bot commented Sep 21, 2017

satya164 commented Oct 8, 2017

facebook-github-bot commented Oct 21, 2017

stale bot commented Dec 20, 2017

danilobuerger commented Dec 28, 2017

bood commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

bood commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

satya164 commented Jan 4, 2018

ruiaraujo commented Jan 4, 2018

danilobuerger commented Jan 18, 2018

satya164 commented Jan 18, 2018

danilobuerger commented Jan 18, 2018

andrerfneves commented Jan 18, 2018 •

edited

Loading

brunolemos commented Jan 21, 2018 •

edited

Loading

danilobuerger commented Jan 24, 2018

brunolemos commented Jan 24, 2018

danilobuerger commented Jan 24, 2018

brunolemos commented Jan 25, 2018

satya164 commented Feb 6, 2018

hramos commented Feb 8, 2018

ruiaraujo commented Feb 8, 2018

pull-bot commented Feb 8, 2018

facebook-github-bot commented Feb 8, 2018

satya164 commented Mar 5, 2018

facebook-github-bot left a comment

Fix Viewpager on Android when using native navigation. #14867

Fix Viewpager on Android when using native navigation. #14867

Conversation

ruiaraujo commented Jul 6, 2017

javache commented Jul 6, 2017

ruiaraujo commented Jul 17, 2017

andrerfneves commented Aug 11, 2017

facebook-github-bot commented Sep 18, 2017

pull-bot commented Sep 21, 2017

satya164 commented Oct 8, 2017

facebook-github-bot commented Oct 21, 2017

stale bot commented Dec 20, 2017

danilobuerger commented Dec 28, 2017

bood commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

bood commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

danilobuerger commented Jan 4, 2018

satya164 commented Jan 4, 2018

ruiaraujo commented Jan 4, 2018

danilobuerger commented Jan 18, 2018

satya164 commented Jan 18, 2018

danilobuerger commented Jan 18, 2018

andrerfneves commented Jan 18, 2018 • edited Loading

brunolemos commented Jan 21, 2018 • edited Loading

danilobuerger commented Jan 24, 2018

brunolemos commented Jan 24, 2018

danilobuerger commented Jan 24, 2018

brunolemos commented Jan 25, 2018

satya164 commented Feb 6, 2018

hramos commented Feb 8, 2018

ruiaraujo commented Feb 8, 2018

pull-bot commented Feb 8, 2018

facebook-github-bot commented Feb 8, 2018

satya164 commented Mar 5, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

andrerfneves commented Jan 18, 2018 •

edited

Loading

brunolemos commented Jan 21, 2018 •

edited

Loading