Skip to content

Fix rn 0.50 repeated calls to onMeasure.#2261

Merged
guyca merged 2 commits into
wix:masterfrom
chaitanya-bhagavan:master
Dec 13, 2017
Merged

Fix rn 0.50 repeated calls to onMeasure.#2261
guyca merged 2 commits into
wix:masterfrom
chaitanya-bhagavan:master

Conversation

@chaitanya-bhagavan
Copy link
Copy Markdown
Contributor

RN 0.50 introduced a new feature to dynamically measure ReactRootView. Since RNN uses RelativeLayout for Screen and BottomTabsLayout child views added to the layout may have additional rules. When the child views are measured they are measured with respect to the LayoutParams set during Screen creation and the Parent MeasureSpec. In RNN, ContentView extends ReactRootView and it calls onMeasure using the widthMeasureSpec and heightMeasureSpec passed to it. If these measureSpecs are EXACTLY then ReactRootView will measure itself to fit the exact size requested by the parent.

If ContentView was added with LayoutParams.MATCH_PARENT for height and/or width with additional rules eg. BELOW or ABOVE the effective height of this View may not be compatible with MATCH_PARENT. RelativeLayout does two passes over its children, in the first pass each child is measured as per their LayoutParams, in the second pass LayoutParam rules are applied and each child is measured. The problem occurs here, if ReactRootView was asked to measure itself with MeasureSpec.EXACTLY but the measuredHeight was less than expected it will trigger a requestLayout() causing the entire layout to be remeasured.

The fix is to call super.onMeasure() in ContentView with MeasureSpec.AT_MOST.

Copy link
Copy Markdown

@enahum enahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this PR fixes the issue with RN 0.50 on android where the view was flickering all the time

@guyca guyca self-assigned this Dec 4, 2017
@enahum
Copy link
Copy Markdown

enahum commented Dec 4, 2017

is there anything else needed for this PR to get merged?

jpgarcia added a commit to underscopeio/react-native-navigation that referenced this pull request Dec 5, 2017
@bitcrumb
Copy link
Copy Markdown
Contributor

bitcrumb commented Dec 5, 2017

Anything that is holding back this PR? If not, would like to see this merged as soon as possible :)
Thanks!

@CherishIt
Copy link
Copy Markdown

Also waiting for this PR being merged to carry on with my work, thanks.

@fforres
Copy link
Copy Markdown

fforres commented Dec 6, 2017

Friendly Ping @guyca and/or @DanielZlotin ? :)

@ChristianAyala
Copy link
Copy Markdown

I can also confirm that this fixes the flickering issue in our app. Would love to see this merged in soon!

@koansang
Copy link
Copy Markdown

When do you merge this PR? @guyca

@guyca guyca merged commit 0ea0a7f into wix:master Dec 13, 2017
@ManAnRuck
Copy link
Copy Markdown

it works! thanks 🥇

@Flavien
Copy link
Copy Markdown

Flavien commented Dec 13, 2017

Is that on NPM yet?

@ManAnRuck
Copy link
Copy Markdown

@Flavien yes, I use yarn and with yarn upgrade-interactive I updated my project and yarn start --reset-cache it fixed this issue.

@jasonmerino
Copy link
Copy Markdown
Contributor

Looks like this fix was released with version 1.1.307. 🙌🏼

@ariona
Copy link
Copy Markdown

ariona commented Dec 14, 2017

This fix the flickering view issue on my app 🥂

@skovhus
Copy link
Copy Markdown

skovhus commented Dec 15, 2017

Thanks @chaitanya0bhagvan ! 👍

Is this backwards compatible? Does the new version of react-native-navigationwork with react-native < 0.50?

I really miss a change log here or at least which versions that depends on which react-native versions.

Related #1551

@chaitanya-bhagavan
Copy link
Copy Markdown
Contributor Author

@skovhus This is backward compatible change. As of now the docs and examples have not been updated to reflect RN 0.50 change where index.android.js and index.ios.js have been replaced with index.js. Let me know if you need any help with the RN upgrade.

chilinh added a commit to chilinh/react-native-navigation that referenced this pull request Dec 18, 2017
* r_master:
  fixed toggle tabs bug (wix#2369)
  Add paddingTop to android ToolBar. (wix#2266)
  [Android] TopTabs font family (wix#2342)
  Ensure correct spelling of commandType (wix#2246)
  Add unregisterForPreviewingWithContext (wix#2239)
  Fix rn 0.50 repeated calls to onMeasure. (wix#2261)
  [Android] Support for transparent statusbar with current screen rendered behind it (wix#2274)
  Fix NPE when activity is relaunched on changing font size of the phone (wix#2319)
  Missed comma (wix#2305)
  Add commandType param to props which indicates if a screen is displayed using push or modal
  Find textual buttons in TitleBar by text instead of content description
  Add a toString implementation to StyleParams Color (wix#2313)
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
thanhzusu pushed a commit to thanhzusu/react-native-navigation that referenced this pull request Dec 21, 2017
* master:
  Now allowing the custom nav bar to take up the whole space on iOS (wix#2306)
  Fix formatting of code block in installation-ios (wix#2356)
  Added optional fixedWidth property to the Android side menu (wix#2380)
  await startApp
  on activity resumed (wix#2370)
  fixed toggle tabs bug (wix#2369)
  Add paddingTop to android ToolBar. (wix#2266)
  [Android] TopTabs font family (wix#2342)
  Ensure correct spelling of commandType (wix#2246)
  Add unregisterForPreviewingWithContext (wix#2239)
  Fix rn 0.50 repeated calls to onMeasure. (wix#2261)
  [Android] Support for transparent statusbar with current screen rendered behind it (wix#2274)
  Fix NPE when activity is relaunched on changing font size of the phone (wix#2319)
  Missed comma (wix#2305)
  Add commandType param to props which indicates if a screen is displayed using push or modal
  Find textual buttons in TitleBar by text instead of content description
  Add a toString implementation to StyleParams Color (wix#2313)
pedro added a commit to lugg/react-native-navigation that referenced this pull request Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.