[Android] Call handleUpdateLayout even if the content didn't change #11222

Kerumen · 2016-11-30T12:57:23Z

This PR fixes #11096.

I don't know enough the ReactAndroid's source code so I don't know if this is correct but I hope it is.

In a recent commit (d4b8ae7), the dispatchUpdates method now returns a boolean to dispatch or not the onLayout event. This works well but if the content is unchanged, the line nativeViewHierarchyOptimizer.handleUpdateLayout(this); is never called. I don't know if it was intended but it was this which introduces my issue. I called this again even if the content didn't change. This was the behaviour before 0.38 so I guess I didn't break anything.

Test plan (required)

I tested my pretty big app with this fix and every screen is ok.

facebook-github-bot · 2016-11-30T12:57:31Z

By analyzing the blame information on this pull request, we identified @astreet and @emilsjolander to be potential reviewers.

astreet · 2016-11-30T15:02:50Z

Why do we need to dispatch handleUpdateLayout when the layout hasn't changed? As in, what did it break for you? I have a feeling there's a better fix we can do.

Kerumen · 2016-11-30T15:16:54Z

I invite you to have a look at the issue I linked (#11096). I explain what worked before and broke (basically a height change that moves my button). And I created a GitHub repo with my bug.

I don't doubt there is a better fix, I'm not an expert in the React Native's java source code.

astreet · 2016-11-30T15:47:55Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java

@@ -295,6 +295,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
          newRight == mAbsoluteRight &&


Thanks for the repro: I just looked through the code here and this check is just not correct. It should all be handled by hasNewLayout above. As such, the correct fix is to just combine lines 301-304 into lines 289-292 and delete this entire if-block.

Noted! I updated the PR :)

Fixes facebook#11096

astreet · 2016-11-30T18:37:08Z

Awesome, thanks!

facebook-github-bot · 2016-11-30T18:37:28Z

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

fred2028 · 2016-12-01T17:40:25Z

Thanks! This fixes something I was investigating

Summary: This PR fixes #11096. I don't know enough the ReactAndroid's source code so I don't know if this is correct but I hope it is. In a recent commit (d4b8ae7), the `dispatchUpdates` method now returns a boolean to dispatch or not the `onLayout` event. This works well but if the content is unchanged, the line `nativeViewHierarchyOptimizer.handleUpdateLayout(this);` is never called. I don't know if it was intended but it was this which introduces my issue. I called this again even if the content didn't change. This was the behaviour before 0.38 so I guess I didn't break anything. **Test plan (required)** I tested my pretty big app with this fix and every screen is ok. Closes #11222 Differential Revision: D4252101 Pulled By: astreet fbshipit-source-id: 551559234631ac37245a854d81ba568f0ddb02dd

Summary: Fixes [#7202 Android Redundant onLayout event](#7202) Closes #7250 Differential Revision: D4104066 Pulled By: mkonicek fbshipit-source-id: 383efdb4b4881aa7d7e508d61c9c01165bcf7bb6

mkonicek · 2016-12-05T00:09:58Z

Thanks for the fix! The bug affected RN versions 0.38 and 0.39.

Your fix fb23000 will be part of the 0.40 release, and was cherry picked into 0.39, and I just now cherry-picked it into 0.38 too for people who are choosing to use 0.38 over 0.39.

Summary: This PR fixes #11096. I don't know enough the ReactAndroid's source code so I don't know if this is correct but I hope it is. In a recent commit (d4b8ae7), the `dispatchUpdates` method now returns a boolean to dispatch or not the `onLayout` event. This works well but if the content is unchanged, the line `nativeViewHierarchyOptimizer.handleUpdateLayout(this);` is never called. I don't know if it was intended but it was this which introduces my issue. I called this again even if the content didn't change. This was the behaviour before 0.38 so I guess I didn't break anything. **Test plan (required)** I tested my pretty big app with this fix and every screen is ok. Closes #11222 Differential Revision: D4252101 Pulled By: astreet fbshipit-source-id: 551559234631ac37245a854d81ba568f0ddb02dd

Summary: This PR fixes facebook#11096. I don't know enough the ReactAndroid's source code so I don't know if this is correct but I hope it is. In a recent commit (facebook@d4b8ae7), the `dispatchUpdates` method now returns a boolean to dispatch or not the `onLayout` event. This works well but if the content is unchanged, the line `nativeViewHierarchyOptimizer.handleUpdateLayout(this);` is never called. I don't know if it was intended but it was this which introduces my issue. I called this again even if the content didn't change. This was the behaviour before 0.38 so I guess I didn't break anything. **Test plan (required)** I tested my pretty big app with this fix and every screen is ok. Closes facebook#11222 Differential Revision: D4252101 Pulled By: astreet fbshipit-source-id: 551559234631ac37245a854d81ba568f0ddb02dd

mschipperheyn · 2016-12-14T19:23:46Z

I'm having a number of issues that seem to be related to this commit. Please reference the discussion here: #11441

Summary: This PR fixes facebook#11096. I don't know enough the ReactAndroid's source code so I don't know if this is correct but I hope it is. In a recent commit (facebook@d4b8ae7), the `dispatchUpdates` method now returns a boolean to dispatch or not the `onLayout` event. This works well but if the content is unchanged, the line `nativeViewHierarchyOptimizer.handleUpdateLayout(this);` is never called. I don't know if it was intended but it was this which introduces my issue. I called this again even if the content didn't change. This was the behaviour before 0.38 so I guess I didn't break anything. **Test plan (required)** I tested my pretty big app with this fix and every screen is ok. Closes facebook#11222 Differential Revision: D4252101 Pulled By: astreet fbshipit-source-id: 551559234631ac37245a854d81ba568f0ddb02dd

ramilushev · 2017-01-12T14:26:53Z

I am also having an issue. See here: #11809

astreet · 2017-01-24T20:40:56Z

I'm having a number of issues that seem to be related to this commit. Please reference the discussion here: #11441

That problem seems to have been on iOS, this is an Android-only change.

sahrens · 2017-01-24T21:25:44Z

Did you experiment with calling nativeViewHierarchyOptimizer.handleUpdateLayout(this); just as often, but returning false if the layout coordinates didn't change? I think that could provide the onLayout optimization we want, but hopefully without breaking anything like the original diff did.

sahrens · 2017-01-24T23:51:46Z

I'm putting up a diff internally that filters out redundant onLayout calls without affecting handleUpdateLayout so hopefully that will get us some solid perf wins without breaking anything :)

ramilushev · 2017-02-08T08:53:26Z

@sahrens Did that internal diff make it to production?

mkonicek · 2017-02-22T16:51:12Z

@ramilushev Not yet but @astreet picked up the work and the diff should be in master soon.

Summary: Developers are complaining about horrible lag (#11809) caused by PR #11222. The issue was that hasNewLayout in yoga is actually a dirty bit and indicates that either you OR one of your children has a new layout. We still need to manually check whether the component's layout actually is different from before. Reviewed By: sahrens Differential Revision: D4597545 fbshipit-source-id: 27d4605afd00badfdcdacae740ee2e477adee929

Summary: Developers are complaining about horrible lag (facebook#11809) caused by PR facebook#11222. The issue was that hasNewLayout in yoga is actually a dirty bit and indicates that either you OR one of your children has a new layout. We still need to manually check whether the component's layout actually is different from before. Reviewed By: sahrens Differential Revision: D4597545 fbshipit-source-id: 27d4605afd00badfdcdacae740ee2e477adee929

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 Nov 30, 2016

astreet suggested changes Nov 30, 2016

View reviewed changes

[Android] Call handleUpdateLayout even if the content didn't change

c07c320

Fixes facebook#11096

Kerumen force-pushed the master branch from b3cf96b to c07c320 Compare November 30, 2016 15:57

facebook-github-bot closed this in fb23000 Nov 30, 2016

mkonicek mentioned this pull request Dec 5, 2016

Android shouldn't dispatch onLayout if frame didn't change #7250

Closed

astreet mentioned this pull request Feb 20, 2017

[Android] Extreme lag after upgrade to 0.39.2 and 0.40.0 #11809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Call handleUpdateLayout even if the content didn't change #11222

[Android] Call handleUpdateLayout even if the content didn't change #11222

Kerumen commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016

astreet commented Nov 30, 2016

Kerumen commented Nov 30, 2016 •

edited

Loading

astreet Nov 30, 2016

Kerumen Nov 30, 2016

astreet commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016

fred2028 commented Dec 1, 2016

mkonicek commented Dec 5, 2016 •

edited

Loading

mschipperheyn commented Dec 14, 2016

ramilushev commented Jan 12, 2017 •

edited

Loading

astreet commented Jan 24, 2017

sahrens commented Jan 24, 2017 •

edited

Loading

sahrens commented Jan 24, 2017 •

edited

Loading

ramilushev commented Feb 8, 2017

mkonicek commented Feb 22, 2017 •

edited

Loading

		@@ -295,6 +295,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
		newRight == mAbsoluteRight &&

[Android] Call handleUpdateLayout even if the content didn't change #11222

[Android] Call handleUpdateLayout even if the content didn't change #11222

Conversation

Kerumen commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016

astreet commented Nov 30, 2016

Kerumen commented Nov 30, 2016 • edited Loading

astreet Nov 30, 2016

Choose a reason for hiding this comment

Kerumen Nov 30, 2016

Choose a reason for hiding this comment

astreet commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016

fred2028 commented Dec 1, 2016

mkonicek commented Dec 5, 2016 • edited Loading

mschipperheyn commented Dec 14, 2016

ramilushev commented Jan 12, 2017 • edited Loading

astreet commented Jan 24, 2017

sahrens commented Jan 24, 2017 • edited Loading

sahrens commented Jan 24, 2017 • edited Loading

ramilushev commented Feb 8, 2017

mkonicek commented Feb 22, 2017 • edited Loading

Kerumen commented Nov 30, 2016 •

edited

Loading

mkonicek commented Dec 5, 2016 •

edited

Loading

ramilushev commented Jan 12, 2017 •

edited

Loading

sahrens commented Jan 24, 2017 •

edited

Loading

sahrens commented Jan 24, 2017 •

edited

Loading

mkonicek commented Feb 22, 2017 •

edited

Loading