Skip to content

Commit

Permalink
Remove ReactHorizontalScrollView Legacy Background Path (facebook#46161)
Browse files Browse the repository at this point in the history 
Summary:
Pull Request resolved: facebook#46161

## This Diff

This removes the legacy path from ReactHorizontalScrollView and its view manager.

## This Stack

This removes the non-Style-applicator background management paths of the different native components. There have been multiple conflicting changes, and bugs added bc harder to reason about, which motivates making this change as soon as possible. This also lets us formalize guarantees that BaseViewManager may safely manipulate background styling of all built in native components.

There is one still known issue, where BackgroundStyleApplicator does not propagate I18nManager derived layout direction to borders (compared to Android derived root direction). This is mostly an issue for apps that with LTR and RTL context, or force a layout direction, which I would guess is relatively rare, so my plan is to forward fix this later this by enabling set_android_layout_direction which will solve that problem mopre generically.

Changelog: [Internal]

Reviewed By: tdn120

Differential Revision: D61658082

fbshipit-source-id: 98cab5dfcad8beee6d131fcfe122313730a6f665
  • Loading branch information
@NickGerleman @facebook-github-bot
NickGerleman authored and facebook-github-bot committed Sep 17, 2024
1 parent 426b300 commit e58d300
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 93 deletions.
60 changes: 15 additions & 45 deletions ...ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import com.facebook.react.uimanager.ReactClippingViewGroupHelper;
import com.facebook.react.uimanager.ReactOverflowViewWithInset;
import com.facebook.react.uimanager.StateWrapper;
import com.facebook.react.uimanager.ViewProps;
import com.facebook.react.uimanager.events.NativeGestureUtil;
import com.facebook.react.uimanager.style.BorderRadiusProp;
import com.facebook.react.uimanager.style.BorderStyle;
Expand All @@ -61,7 +60,6 @@
import com.facebook.react.views.scroll.ReactScrollViewHelper.HasSmoothScroll;
import com.facebook.react.views.scroll.ReactScrollViewHelper.HasStateWrapper;
import com.facebook.react.views.scroll.ReactScrollViewHelper.ReactScrollViewScrollState;
import com.facebook.react.views.view.ReactViewBackgroundManager;
import com.facebook.systrace.Systrace;
import java.lang.reflect.Field;
import java.util.ArrayList;
Expand Down Expand Up @@ -118,7 +116,6 @@ public class ReactHorizontalScrollView extends HorizontalScrollView
private boolean mSnapToStart = true;
private boolean mSnapToEnd = true;
private int mSnapToAlignment = SNAP_ALIGNMENT_DISABLED;
private ReactViewBackgroundManager mReactBackgroundManager;
private boolean mPagedArrowScrolling = false;
private int pendingContentOffsetX = UNSET_CONTENT_OFFSET;
private int pendingContentOffsetY = UNSET_CONTENT_OFFSET;
Expand All @@ -139,7 +136,6 @@ public ReactHorizontalScrollView(Context context) {

public ReactHorizontalScrollView(Context context, @Nullable FpsListener fpsListener) {
super(context);
mReactBackgroundManager = new ReactViewBackgroundManager(this);
mFpsListener = fpsListener;

ViewCompat.setAccessibilityDelegate(this, new ReactScrollViewAccessibilityDelegate());
Expand All @@ -155,7 +151,6 @@ public ReactHorizontalScrollView(Context context, @Nullable FpsListener fpsListe

setOnHierarchyChangeListener(this);
setClipChildren(false);
mReactBackgroundManager.setOverflow(ViewProps.SCROLL);
}

public boolean getScrollEnabled() {
Expand Down Expand Up @@ -290,7 +285,6 @@ public void setOverflow(@Nullable String overflow) {
mOverflow = parsedOverflow == null ? Overflow.SCROLL : parsedOverflow;
}

mReactBackgroundManager.setOverflow(overflow == null ? ViewProps.SCROLL : overflow);
invalidate();
}

Expand Down Expand Up @@ -334,12 +328,8 @@ public Rect getOverflowInset() {

@Override
public void onDraw(Canvas canvas) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
if (mOverflow != Overflow.VISIBLE) {
BackgroundStyleApplicator.clipToPaddingBox(this, canvas);
}
} else {
mReactBackgroundManager.maybeClipToPaddingBox(canvas);
if (mOverflow != Overflow.VISIBLE) {
BackgroundStyleApplicator.clipToPaddingBox(this, canvas);
}
super.onDraw(canvas);
}
Expand Down Expand Up @@ -1330,55 +1320,35 @@ private void smoothScrollToNextPage(int direction) {

@Override
public void setBackgroundColor(int color) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBackgroundColor(this, color);
} else {
mReactBackgroundManager.setBackgroundColor(color);
}
BackgroundStyleApplicator.setBackgroundColor(this, color);
}

public void setBorderWidth(int position, float width) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBorderWidth(
this, LogicalEdge.values()[position], PixelUtil.toDIPFromPixel(width));
} else {
mReactBackgroundManager.setBorderWidth(position, width);
}
BackgroundStyleApplicator.setBorderWidth(
this, LogicalEdge.values()[position], PixelUtil.toDIPFromPixel(width));
}

public void setBorderColor(int position, @Nullable Integer color) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBorderColor(this, LogicalEdge.values()[position], color);
} else {
mReactBackgroundManager.setBorderColor(position, color);
}
BackgroundStyleApplicator.setBorderColor(this, LogicalEdge.values()[position], color);
}

public void setBorderRadius(float borderRadius) {
setBorderRadius(borderRadius, BorderRadiusProp.BORDER_RADIUS.ordinal());
}

public void setBorderRadius(float borderRadius, int position) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
@Nullable
LengthPercentage radius =
Float.isNaN(borderRadius)
? null
: new LengthPercentage(
PixelUtil.toDIPFromPixel(borderRadius), LengthPercentageType.POINT);
BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius);
} else {
mReactBackgroundManager.setBorderRadius(borderRadius, position);
}
@Nullable
LengthPercentage radius =
Float.isNaN(borderRadius)
? null
: new LengthPercentage(
PixelUtil.toDIPFromPixel(borderRadius), LengthPercentageType.POINT);
BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius);
}

public void setBorderStyle(@Nullable String style) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBorderStyle(
this, style == null ? null : BorderStyle.fromString(style));
} else {
mReactBackgroundManager.setBorderStyle(style);
}
BackgroundStyleApplicator.setBorderStyle(
this, style == null ? null : BorderStyle.fromString(style));
}

/**
Expand Down
62 changes: 14 additions & 48 deletions ...droid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.RetryableMountingLayerException;
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.uimanager.BackgroundStyleApplicator;
import com.facebook.react.uimanager.LengthPercentage;
Expand Down Expand Up @@ -264,36 +263,20 @@ public void setBottomFillColor(ReactHorizontalScrollView view, int color) {
},
defaultFloat = Float.NaN)
public void setBorderRadius(ReactHorizontalScrollView view, int index, float borderRadius) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
@Nullable
LengthPercentage radius =
Float.isNaN(borderRadius)
? null
: new LengthPercentage(borderRadius, LengthPercentageType.POINT);
BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius);
} else {
if (!Float.isNaN(borderRadius)) {
borderRadius = PixelUtil.toPixelFromDIP(borderRadius);
}

if (index == 0) {
view.setBorderRadius(borderRadius);
} else {
view.setBorderRadius(borderRadius, index - 1);
}
}
@Nullable
LengthPercentage radius =
Float.isNaN(borderRadius)
? null
: new LengthPercentage(borderRadius, LengthPercentageType.POINT);
BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius);
}

@ReactProp(name = "borderStyle")
public void setBorderStyle(ReactHorizontalScrollView view, @Nullable String borderStyle) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
@Nullable
BorderStyle parsedBorderStyle =
borderStyle == null ? null : BorderStyle.fromString(borderStyle);
BackgroundStyleApplicator.setBorderStyle(view, parsedBorderStyle);
} else {
view.setBorderStyle(borderStyle);
}
@Nullable
BorderStyle parsedBorderStyle =
borderStyle == null ? null : BorderStyle.fromString(borderStyle);
BackgroundStyleApplicator.setBorderStyle(view, parsedBorderStyle);
}

@ReactPropGroup(
Expand All @@ -306,14 +289,7 @@ public void setBorderStyle(ReactHorizontalScrollView view, @Nullable String bord
},
defaultFloat = Float.NaN)
public void setBorderWidth(ReactHorizontalScrollView view, int index, float width) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBorderWidth(view, LogicalEdge.values()[index], width);
} else {
if (!Float.isNaN(width)) {
width = PixelUtil.toPixelFromDIP(width);
}
view.setBorderWidth(SPACING_TYPES[index], width);
}
BackgroundStyleApplicator.setBorderWidth(view, LogicalEdge.values()[index], width);
}

@ReactPropGroup(
Expand All @@ -326,11 +302,7 @@ public void setBorderWidth(ReactHorizontalScrollView view, int index, float widt
},
customType = "Color")
public void setBorderColor(ReactHorizontalScrollView view, int index, @Nullable Integer color) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBorderColor(view, LogicalEdge.ALL, color);
} else {
view.setBorderColor(SPACING_TYPES[index], color);
}
BackgroundStyleApplicator.setBorderColor(view, LogicalEdge.ALL, color);
}

@ReactProp(name = "overflow")
Expand Down Expand Up @@ -392,17 +364,11 @@ public void setHorizontal(ReactHorizontalScrollView view, boolean horizontal) {

@ReactProp(name = ViewProps.BOX_SHADOW, customType = "BoxShadow")
public void setBoxShadow(ReactHorizontalScrollView view, @Nullable ReadableArray shadows) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBoxShadow(view, shadows);
}
BackgroundStyleApplicator.setBoxShadow(view, shadows);
}

@Override
public void setBackgroundColor(ReactHorizontalScrollView view, @ColorInt int backgroundColor) {
if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) {
BackgroundStyleApplicator.setBackgroundColor(view, backgroundColor);
} else {
super.setBackgroundColor(view, backgroundColor);
}
BackgroundStyleApplicator.setBackgroundColor(view, backgroundColor);
}
}

0 comments on commit e58d300

Please sign in to comment.