Skip to content

Commit

Permalink
BREAKING [react_native] Move to new C-based implementation of css-lay…
Browse files Browse the repository at this point in the history
…out in RN Android

Summary:
Moves from CSSNodeDEPRECATED to CSSNode. This has shown to be a huge performance win for layout time within FB.

This is BREAKING because CSSNode contains bug fixes that were not migrated to CSSNodeDEPRECATED which may change the way your layout appears. The most common of these by far involves `flex: 1`.

Previously, developers had to put `flex: 1` in many places it didn't belong in order to work around a bug in css-layout. Now `flex: 1` is treated properly and, unfortunately, this means that your layout may no longer look correct. Specifically, you may see that your layout looks collapsed, or children don't render. The fix is to simply remove `flex: 1` from those containers.

Reviewed By: emilsjolander

Differential Revision: D3992787

fbshipit-source-id: 7a3a2a34a8941c0524e6ba3c5379e434d3e03247
  • Loading branch information
astreet authored and Facebook Github Bot committed Nov 21, 2016
1 parent 27817be commit d63ba47
Show file tree
Hide file tree
Showing 16 changed files with 124 additions and 33 deletions.
27 changes: 17 additions & 10 deletions ReactAndroid/DEFS
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
# - At Facebook by running buck from the root of the fb repo
# - Outside of Facebook by running buck in the root of the git repo

IS_OSS_BUILD = True

with allow_unsafe_import():
import os

Expand Down Expand Up @@ -35,6 +37,8 @@ JSC_DEPS = [

CSSLAYOUT_TARGET = '//ReactAndroid/src/main/java/com/facebook:csslayout'
FBGLOGINIT_TARGET = '//ReactAndroid/src/main/jni/first-party/fbgloginit:fbgloginit'
FBJNI_TARGET = '//ReactAndroid/src/main/jni/first-party/fb:jni'
JNI_TARGET = '//ReactAndroid/src/main/jni/first-party/jni-hack:jni-hack'

# React property preprocessor
original_android_library=android_library
Expand Down Expand Up @@ -76,9 +80,7 @@ def android_library(
*args,
**kwargs)


def robolectric3_test(name, deps, vm_args=None, *args, **kwargs):

def rn_robolectric_test(name, srcs, vm_args = None, *args, **kwargs):
vm_args = vm_args or []

# We may need to create buck-out/gen/ if we're running after buck clean.
Expand All @@ -100,11 +102,16 @@ def robolectric3_test(name, deps, vm_args=None, *args, **kwargs):
'-Djava.io.tmpdir=%s' % os.path.join(os.path.abspath('.'),
'buck-out/bin'))

# defined in BUCK
# RN tests use Powermock, which means they get their own ClassLoaders.
# Because the csslayout native library (or any native library) can only be loaded into one
# ClassLoader at a time, we need to run each in its own process, hence fork_mode = 'per_test'.
robolectric_test(
name=name,
deps=deps,
vm_args=vm_args + extra_vm_args,
*args,
**kwargs
)
name = name,

This comment has been minimized.

Copy link
@leeight

leeight Nov 24, 2016

Contributor

@astreet buck test ReactAndroid/src/test/java/com/facebook/react/views will failed with error message FAILURE com.facebook.react.views.textinput.TextInputTest testPropsApplied: no csslayout in java.library.path, is there any way to fix it?

This comment has been minimized.

Copy link
@astreet

astreet Nov 24, 2016

Author Contributor

Buck should be doing that for you... it looks like things aren't building properly +@emilsjolander

What platform are you building on? What's the output of which java?

This comment has been minimized.

Copy link
@emilsjolander

emilsjolander Nov 24, 2016

Contributor

This happens when java8 is not set up outside /usr/bin due to macOS SIP. Install java8 manually and make sure to add JAVA_HOME/bin to the start of your PATH

use_cxx_libraries = True,
cxx_library_whitelist = [
'//ReactAndroid/src/main/jni/first-party/csslayoutjni:jni',
],
fork_mode = 'per_test',
srcs = srcs,
vm_args = vm_args + extra_vm_args,
*args, **kwargs)
1 change: 0 additions & 1 deletion ReactAndroid/src/androidTest/js/UIManagerTestModule.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ var CenteredTextView = React.createClass({
width: 200,
height: 100,
backgroundColor: '#aa3311',
flex: 1,
justifyContent: 'center',
alignItems: 'center',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
import com.facebook.csslayout.CSSFlexDirection;
import com.facebook.csslayout.CSSJustify;
import com.facebook.csslayout.CSSLayoutContext;
import com.facebook.csslayout.CSSNode;
import com.facebook.csslayout.CSSNodeAPI;
import com.facebook.csslayout.CSSNodeDEPRECATED;
import com.facebook.csslayout.CSSOverflow;
import com.facebook.csslayout.CSSPositionType;
import com.facebook.csslayout.CSSWrap;
Expand All @@ -30,9 +30,9 @@

/**
* Base node class for representing virtual tree of React nodes. Shadow nodes are used primarily
* for layouting therefore it extends {@link CSSNodeDEPRECATED} to allow that. They also help with handling
* Common base subclass of {@link CSSNodeDEPRECATED} for all layout nodes for react-based view. It extends
* {@link CSSNodeDEPRECATED} by adding additional capabilities.
* for layouting therefore it extends {@link CSSNode} to allow that. They also help with handling
* Common base subclass of {@link CSSNode} for all layout nodes for react-based view. It extends
* {@link CSSNode} by adding additional capabilities.
*
* Instances of this class receive property updates from JS via @{link UIManagerModule}. Subclasses
* may use {@link #updateShadowNode} to persist some of the updated fields in the node instance that
Expand Down Expand Up @@ -74,7 +74,7 @@ public class ReactShadowNode {
private float mAbsoluteBottom;
private final Spacing mDefaultPadding = new Spacing(0);
private final Spacing mPadding = new Spacing(CSSConstants.UNDEFINED);
private final CSSNodeDEPRECATED mCSSNode = new CSSNodeDEPRECATED();
private final CSSNode mCSSNode = new CSSNode();

/**
* Nodes that return {@code true} will be treated as "virtual" nodes. That is, nodes that are not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@

package com.facebook.react.views.text;

import com.facebook.csslayout.CSSNodeDEPRECATED;
import com.facebook.csslayout.CSSNode;
import com.facebook.react.uimanager.LayoutShadowNode;

/**
* Base class for {@link CSSNodeDEPRECATED}s that represent inline images.
* Base class for {@link CSSNode}s that represent inline images.
*/
public abstract class ReactTextInlineImageShadowNode extends LayoutShadowNode {

Expand All @@ -22,5 +22,4 @@ public abstract class ReactTextInlineImageShadowNode extends LayoutShadowNode {
* place of this node.
*/
public abstract TextInlineImageSpan buildInlineImageSpan();

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@
import android.net.Uri;

import com.facebook.common.util.UriUtil;
import com.facebook.csslayout.CSSNodeDEPRECATED;
import com.facebook.drawee.controller.AbstractDraweeControllerBuilder;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.uimanager.annotations.ReactProp;
import com.facebook.react.views.text.ReactTextInlineImageShadowNode;
import com.facebook.react.views.text.TextInlineImageSpan;

/**
* {@link CSSNodeDEPRECATED} that represents an inline image. Loading is done using Fresco.
* Shadow node that represents an inline image. Loading is done using Fresco.
*
*/
public class FrescoBasedReactTextInlineImageShadowNode extends ReactTextInlineImageShadowNode {
Expand Down Expand Up @@ -113,5 +112,4 @@ public AbstractDraweeControllerBuilder getDraweeControllerBuilder() {
public @Nullable Object getCallerContext() {
return mCallerContext;
}

}
23 changes: 23 additions & 0 deletions ReactAndroid/src/main/jni/first-party/csslayoutjni/BUCK
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
include_defs('//ReactAndroid/DEFS')

# This target is only used in open source
if IS_OSS_BUILD:
cxx_library(
name = 'jni',
soname = 'libcsslayout.$(ext)',
srcs = glob(['jni/*.cpp']),
header_namespace = '',
compiler_flags = [
'-fno-omit-frame-pointer',
'-fexceptions',
'-Wall',
'-Werror',
'-O3',
'-std=c++11',
],
deps = [
'//ReactCommon/CSSLayout:CSSLayout',
FBJNI_TARGET,
],
visibility = ['PUBLIC'],
)
27 changes: 27 additions & 0 deletions ReactAndroid/src/main/jni/first-party/fb/BUCK
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
include_defs('//ReactAndroid/DEFS')

# This target is only used in open source
if IS_OSS_BUILD:
cxx_library(
name = 'jni',
soname = 'libfb.$(ext)',
srcs = glob(['*.cpp', 'jni/*.cpp', 'lyra/*.cpp']),
header_namespace = '',
compiler_flags = [
'-fno-omit-frame-pointer',
'-fexceptions',
'-Wall',
'-Werror',
'-std=c++11',
'-DDISABLE_CPUCAP',
'-DDISABLE_XPLAT',
],
exported_headers = subdir_glob([
('include', 'fb/**/*.h'),
('include', 'jni/*.h'),
]),
deps = [
JNI_TARGET,
],
visibility = ['PUBLIC'],
)
2 changes: 1 addition & 1 deletion ReactAndroid/src/test/java/com/facebook/react/BUCK
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
name = 'react',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
name = 'animated',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
2 changes: 1 addition & 1 deletion ReactAndroid/src/test/java/com/facebook/react/bridge/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ android_library(
],
)

robolectric3_test(
rn_robolectric_test(
name = 'bridge',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
name = 'devsupport',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
2 changes: 1 addition & 1 deletion ReactAndroid/src/test/java/com/facebook/react/modules/BUCK
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
name = 'modules',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
name = 'uimanager',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

import javax.annotation.Nullable;

import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.JavaOnlyMap;
import com.facebook.react.uimanager.annotations.ReactProp;
import com.facebook.react.uimanager.annotations.ReactPropGroup;

Expand All @@ -22,8 +25,6 @@
import org.powermock.modules.junit4.rule.PowerMockRule;
import org.robolectric.RobolectricTestRunner;

import static com.facebook.react.uimanager.ReactPropAnnotationSetterTest.ViewManagerUpdatesReceiver;
import static com.facebook.react.uimanager.ReactPropAnnotationSetterTest.buildStyles;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.verify;
Expand All @@ -41,6 +42,25 @@ public class ReactPropForShadowNodeSetterTest {
@Rule
public PowerMockRule rule = new PowerMockRule();

public interface ViewManagerUpdatesReceiver {
void onBooleanSetterCalled(boolean value);
void onIntSetterCalled(int value);
void onDoubleSetterCalled(double value);
void onFloatSetterCalled(float value);
void onStringSetterCalled(String value);
void onBoxedBooleanSetterCalled(Boolean value);
void onBoxedIntSetterCalled(Integer value);
void onArraySetterCalled(ReadableArray value);
void onMapSetterCalled(ReadableMap value);
void onFloatGroupPropSetterCalled(int index, float value);
void onIntGroupPropSetterCalled(int index, int value);
void onBoxedIntGroupPropSetterCalled(int index, Integer value);
}

public static ReactStylesDiffMap buildStyles(Object... keysAndValues) {
return new ReactStylesDiffMap(JavaOnlyMap.of(keysAndValues));
}

private class ShadowViewUnderTest extends ReactShadowNode {

private ViewManagerUpdatesReceiver mViewManagerUpdatesReceiver;
Expand All @@ -65,8 +85,8 @@ public void setBoxedIntProp(@Nullable Integer value) {
}

@ReactPropGroup(names = {
"floatGroupPropFirst",
"floatGroupPropSecond",
"floatGroupPropFirst",
"floatGroupPropSecond",
})
public void setFloatGroupProp(int index, float value) {
mViewManagerUpdatesReceiver.onFloatGroupPropSetterCalled(index, value);
Expand Down
2 changes: 1 addition & 1 deletion ReactAndroid/src/test/java/com/facebook/react/views/BUCK
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include_defs('//ReactAndroid/DEFS')

robolectric3_test(
rn_robolectric_test(
name = 'views',
# Please change the contact to the oncall of your team
contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'],
Expand Down
18 changes: 18 additions & 0 deletions ReactCommon/CSSLayout/BUCK
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
cxx_library(
name = 'CSSLayout',
force_static = True,
srcs = glob(['CSSLayout/*.c']),
header_namespace = '',
compiler_flags = [
'-fno-omit-frame-pointer',
'-fexceptions',
'-Wall',
'-Werror',
'-std=c99',
'-O3',
],
exported_headers = glob(['CSSLayout/*.h']),
deps = [
],
visibility = ['PUBLIC'],
)

0 comments on commit d63ba47

Please sign in to comment.