From 4076293aa1059005704576530d8fe948b85e6a6d Mon Sep 17 00:00:00 2001 From: David Vacca Date: Sun, 10 Jan 2021 19:44:28 -0800 Subject: [PATCH] Fix changes of View visibilities Summary: The purpose of this diff is to ensure that visibility changes are handled correctly when the value of "display" for a View changes from 'flex' to 'none'. RNTester is nesting several Views with different kind of visibilities. When the user tap on an item there's a state update that changes the visibility styles for some of these views. Fabric does not reflect the right changes of visibility on the screen. changelog: internal Reviewed By: shergin Differential Revision: D25841763 fbshipit-source-id: 769b97afb72939d346a4c6f2669ff938b35596bc --- .../java/com/facebook/react/fabric/jni/Binding.cpp | 11 +++++++---- .../react/fabric/mounting/MountingManager.java | 8 +++++++- .../mounting/mountitems/IntBufferBatchMountItem.java | 11 ++++++++--- ReactCommon/react/renderer/core/conversions.h | 11 +++++++++++ 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 75ec98c011b9d3..b6161d0db0938e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -102,7 +102,7 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) { } else if (mountItemType == CppMountItem::Type::UpdatePadding) { return 5; // tag, top, left, bottom, right } else if (mountItemType == CppMountItem::Type::UpdateLayout) { - return 6; // tag, x, y, w, h, layoutDirection + return 7; // tag, x, y, w, h, layoutDirection, DisplayType } else if (mountItemType == CppMountItem::Type::UpdateEventEmitter) { return 1; // tag } else { @@ -714,7 +714,7 @@ void Binding::schedulerDidFinishTransaction( int intBufferPosition = 0; int objBufferPosition = 0; int prevMountItemType = -1; - jint temp[6]; + jint temp[7]; for (int i = 0; i < cppCommonMountItems.size(); i++) { const auto &mountItem = cppCommonMountItems[i]; const auto &mountItemType = mountItem.type; @@ -879,6 +879,8 @@ void Binding::schedulerDidFinishTransaction( int h = round(frame.size.height * pointScaleFactor); int layoutDirection = toInt(mountItem.newChildShadowView.layoutMetrics.layoutDirection); + int displayType = + toInt(mountItem.newChildShadowView.layoutMetrics.displayType); temp[0] = mountItem.newChildShadowView.tag; temp[1] = x; @@ -886,8 +888,9 @@ void Binding::schedulerDidFinishTransaction( temp[3] = w; temp[4] = h; temp[5] = layoutDirection; - env->SetIntArrayRegion(intBufferArray, intBufferPosition, 6, temp); - intBufferPosition += 6; + temp[6] = displayType; + env->SetIntArrayRegion(intBufferArray, intBufferPosition, 7, temp); + intBufferPosition += 7; } } if (cppUpdateEventEmitterMountItems.size() > 0) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index fe00e5d35b9dfb..494e3c1f46b971 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -563,7 +563,7 @@ public void updateProps(int reactTag, @Nullable ReadableMap props) { } @UiThread - public void updateLayout(int reactTag, int x, int y, int width, int height) { + public void updateLayout(int reactTag, int x, int y, int width, int height, int displayType) { UiThreadUtil.assertOnUiThread(); ViewState viewState = getViewState(reactTag); @@ -589,6 +589,12 @@ public void updateLayout(int reactTag, int x, int y, int width, int height) { // TODO: T31905686 Check if the parent of the view has to layout the view, or the child has // to lay itself out. see NativeViewHierarchyManager.updateLayout viewToUpdate.layout(x, y, x + width, y + height); + + // displayType: 0 represents display: 'none' + int visibility = displayType == 0 ? View.INVISIBLE : View.VISIBLE; + if (viewToUpdate.getVisibility() != visibility) { + viewToUpdate.setVisibility(visibility); + } } @UiThread diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index efaafa11ead5a8..e2c226ede8a0b9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -157,11 +157,16 @@ public void execute(@NonNull MountingManager mountingManager) { } else if (type == INSTRUCTION_UPDATE_STATE) { mountingManager.updateState(mIntBuffer[i++], castToState(mObjBuffer[j++])); } else if (type == INSTRUCTION_UPDATE_LAYOUT) { - mountingManager.updateLayout( - mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++]); - + int reactTag = mIntBuffer[i++]; + int x = mIntBuffer[i++]; + int y = mIntBuffer[i++]; + int width = mIntBuffer[i++]; + int height = mIntBuffer[i++]; // The final buffer, layoutDirection, seems unused? i++; + int displayType = mIntBuffer[i++]; + mountingManager.updateLayout(reactTag, x, y, width, height, displayType); + } else if (type == INSTRUCTION_UPDATE_PADDING) { mountingManager.updatePadding( mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++]); diff --git a/ReactCommon/react/renderer/core/conversions.h b/ReactCommon/react/renderer/core/conversions.h index 9dc3daa994fa89..70dda9a9f5589c 100644 --- a/ReactCommon/react/renderer/core/conversions.h +++ b/ReactCommon/react/renderer/core/conversions.h @@ -34,6 +34,17 @@ inline int toInt(const LayoutDirection &layoutDirection) { } } +inline int toInt(const DisplayType &displayType) { + switch (displayType) { + case DisplayType::None: + return 0; + case DisplayType::Flex: + return 1; + case DisplayType::Inline: + return 2; + } +} + inline std::string toString(const DisplayType &displayType) { switch (displayType) { case DisplayType::None: