From 95ae936aa634394bf358546841cf8975fd484b02 Mon Sep 17 00:00:00 2001 From: Seth Kirby Date: Tue, 19 Jul 2016 13:15:55 -0700 Subject: [PATCH] Fix for ViewManager commands being run before view updates. Summary: Since Nodes' manageChildren doesn't enqueue the child updates immediately, commands were being directed to non-updated views. Previously we applied updates for the shadow node before dispatching the command, but we can instead wait to fire commands until after we update the view hierarchy. Reviewed By: ahmedre Differential Revision: D3568541 --- .../react/flat/FlatUIImplementation.java | 23 ++++-------- .../react/flat/FlatUIViewOperationQueue.java | 37 +++++++++++++++++++ .../com/facebook/react/flat/StateBuilder.java | 19 ++++++++++ 3 files changed, 64 insertions(+), 15 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java index 6580d36bff0..741246fc716 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java @@ -71,7 +71,7 @@ public class FlatUIImplementation extends UIImplementation { /** * Helper class that sorts moveTo/moveFrom arrays passed to #manageChildren(). * Not used outside of the said method. - */ + */ private final MoveProxy mMoveProxy = new MoveProxy(); private final StateBuilder mStateBuilder; private @Nullable ReactImageManager mReactImageManager; @@ -224,14 +224,13 @@ public class FlatUIImplementation extends UIImplementation { callback); } - private boolean ensureMountsToViewAndBackingViewIsCreated(int reactTag) { + private void ensureMountsToViewAndBackingViewIsCreated(int reactTag) { FlatShadowNode node = (FlatShadowNode) resolveShadowNode(reactTag); if (node.isBackingViewCreated()) { - return false; + return; } node.forceMountToView(); mStateBuilder.ensureBackingViewIsCreated(node); - return true; } @Override @@ -253,16 +252,10 @@ public class FlatUIImplementation extends UIImplementation { @Override public void dispatchViewManagerCommand(int reactTag, int commandId, ReadableArray commandArgs) { - if (ensureMountsToViewAndBackingViewIsCreated(reactTag)) { - // need to make sure any ui operations (UpdateViewGroup, for example, etc) have already - // happened before we actually dispatch the view manager command (since otherwise, the command - // may go to an empty shell parent without its children, which is against the specs). note - // that we only want to applyUpdates if the view has not yet been created so that it does - // get created (otherwise, we may end up changing the View's position when we're not supposed - // to, for example). - mStateBuilder.applyUpdates((FlatShadowNode) resolveShadowNode(reactTag)); - } - super.dispatchViewManagerCommand(reactTag, commandId, commandArgs); + // Make sure that our target view is actually a view, then delay command dispatch until after + // we have updated the view hierarchy. + ensureMountsToViewAndBackingViewIsCreated(reactTag); + mStateBuilder.enqueueViewManagerCommand(reactTag, commandId, commandArgs); } @Override @@ -414,7 +407,7 @@ public class FlatUIImplementation extends UIImplementation { ++addToIndex; if (addToIndex == numNodesToAdd) { - addToChildIndex = Integer.MAX_VALUE; + addToChildIndex = Integer.MAX_VALUE; } else { addToChildIndex = addAtIndices.getInt(addToIndex); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java index 957937a6c6c..7cf0f80e7be 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java @@ -17,6 +17,7 @@ import android.view.ViewGroup; import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.ReactApplicationContext; +import com.facebook.react.bridge.ReadableArray; import com.facebook.react.uimanager.IllegalViewOperationException; import com.facebook.react.uimanager.NoSuchNativeViewException; import com.facebook.react.uimanager.PixelUtil; @@ -314,6 +315,31 @@ import com.facebook.react.uimanager.UIViewOperationQueue; } } + /** + * Used to delay view manager command dispatch until after the view hierarchy is updated. + * Mirrors command operation dispatch, but is only used in Nodes for view manager commands. + */ + public final class ViewManagerCommand implements UIOperation { + + private final int mReactTag; + private final int mCommand; + private final @Nullable ReadableArray mArgs; + + public ViewManagerCommand( + int reactTag, + int command, + @Nullable ReadableArray args) { + mReactTag = reactTag; + mCommand = command; + mArgs = args; + } + + @Override + public void execute() { + mNativeViewHierarchyManager.dispatchCommand(mReactTag, mCommand, mArgs); + } + } + public FlatUIViewOperationQueue( ReactApplicationContext reactContext, FlatNativeViewHierarchyManager nativeViewHierarchyManager) { @@ -359,6 +385,17 @@ import com.facebook.react.uimanager.UIViewOperationQueue; enqueueUIOperation(updateViewBounds); } + public ViewManagerCommand createViewManagerCommand( + int reactTag, + int command, + @Nullable ReadableArray args) { + return new ViewManagerCommand(reactTag, command, args); + } + + public void enqueueViewManagerCommand(ViewManagerCommand viewManagerCommand) { + enqueueUIOperation(viewManagerCommand); + } + public void enqueueSetPadding( int reactTag, int paddingLeft, diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java index c696c004f20..6cd0761f953 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java @@ -14,6 +14,7 @@ import javax.annotation.Nullable; import java.util.ArrayList; import com.facebook.csslayout.Spacing; +import com.facebook.react.bridge.ReadableArray; import com.facebook.react.uimanager.OnLayoutEvent; import com.facebook.react.uimanager.ReactShadowNode; import com.facebook.react.uimanager.ReactStylesDiffMap; @@ -48,6 +49,8 @@ import com.facebook.react.uimanager.events.EventDispatcher; private final ArrayList mOnLayoutEvents = new ArrayList<>(); private final ArrayList mUpdateViewBoundsOperations = new ArrayList<>(); + private final ArrayList mViewManagerCommands = + new ArrayList<>(); private @Nullable FlatUIViewOperationQueue.DetachAllChildrenFromViews mDetachAllChildrenFromViews; @@ -99,6 +102,14 @@ import com.facebook.react.uimanager.events.EventDispatcher; } mUpdateViewBoundsOperations.clear(); + // Process view manager commands after bounds operations, so that any ui operations have already + // happened before we actually dispatch the view manager command. This prevents things like + // commands going to empty parents and views not yet being created. + for (int i = 0, size = mViewManagerCommands.size(); i != size; i++) { + mOperationsQueue.enqueueViewManagerCommand(mViewManagerCommands.get(i)); + } + mViewManagerCommands.clear(); + // This could be more efficient if EventDispatcher had a batch mode // to avoid multiple synchronized calls. for (int i = 0, size = mOnLayoutEvents.size(); i != size; ++i) { @@ -134,6 +145,14 @@ import com.facebook.react.uimanager.events.EventDispatcher; mAttachDetachListeners.add(listener); } + /* package */ void enqueueViewManagerCommand( + int reactTag, + int commandId, + ReadableArray commandArgs) { + mViewManagerCommands.add( + mOperationsQueue.createViewManagerCommand(reactTag, commandId, commandArgs)); + } + /* package */ void enqueueCreateOrUpdateView( FlatShadowNode node, @Nullable ReactStylesDiffMap styles) {