[ScrollView][iOS] Change padding to use max of safe area inset/container value

Author: huntie

packages/react-native/ReactCommon/react/renderer/components/scrollview/ScrollContentViewShadowNode.cpp

@@ -7,6 +7,7 @@
 
 #include "ScrollContentViewShadowNode.h"
 
+#include <algorithm>
 #include <array>
 
 namespace facebook::react {
@@ -78,16 +79,18 @@ StyleLength resolvedPadding(
 }
 
 /*
- * A percentage padding keeps the edge as the style set it, with no inset
- * added, since Yoga holds one length per edge and a percentage has no point
- * value to add to.
+ * The safe area is a floor on the padding, as it is on UIKit's layout
+ * margins: the edge takes the larger of its style padding and the inset. A
+ * percentage padding keeps the edge as the style set it, with no inset
+ * applied, since Yoga holds one length per edge and a percentage has no point
+ * value to compare.
  */
 StyleLength padded(StyleLength value, Float inset) {
   if (inset == 0 || value.isPercent()) {
     return value;
   }
-  auto base = value.isPoints() ? value.value().unwrap() : 0.0f;
-  return StyleLength::points(base + inset);
+  auto base = value.isPoints() ? Float{value.value().unwrap()} : Float{0};
+  return StyleLength::points(std::max(base, inset));
 }
 
 } // namespace

packages/react-native/ReactCommon/react/renderer/components/scrollview/ScrollContentViewShadowNode.h

@@ -21,8 +21,8 @@ extern const char ScrollContentViewComponentName[];
 
 /*
  * `ShadowNode` for the <ScrollContentView> component, the content container of
- * a <ScrollView>. It pads its content by the safe area in its state, on top of
- * the padding its style sets.
+ * a <ScrollView>. It pads its content by at least the safe area in its state,
+ * keeping any larger padding its style sets.
  */
 class ScrollContentViewShadowNode final : public ConcreteViewShadowNode<
                                               ScrollContentViewComponentName,
@@ -46,8 +46,8 @@ class ScrollContentViewShadowNode final : public ConcreteViewShadowNode<
   void applySafeAreaPadding();
 
   /*
-   * Pads `style` by `state.safeAreaPadding`, on top of the padding `style`
-   * resolves to on each edge, following the precedence Yoga gives the padding
+   * Raises the padding `style` resolves to on each edge to at least
+   * `state.safeAreaPadding`, following the precedence Yoga gives the padding
    * edges for `layoutDirection`. An edge with a percentage padding keeps it
    * and gets no inset.
    */

packages/react-native/ReactCommon/react/renderer/components/scrollview/tests/ScrollContentViewTest.cpp

@@ -47,28 +47,39 @@ TEST(ScrollContentViewShadowNodeTest, leavesStyleWithoutInsets) {
   EXPECT_FALSE(style.padding(Edge::Start).isDefined());
 }
 
-TEST(ScrollContentViewShadowNodeTest, padsRowOnTopOfResolvedPadding) {
+TEST(ScrollContentViewShadowNodeTest, raisesRowPaddingToTheInset) {
   yoga::Style style;
   style.setPadding(Edge::Horizontal, StyleLength::points(16));
 
   ScrollContentViewShadowNode::padForSafeArea(
       style, stateWith({.left = 0, .top = 0, .right = 84, .bottom = 0}), kLTR);
 
   EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(16));
-  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(100));
+  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(84));
   EXPECT_FALSE(style.padding(Edge::Left).isDefined());
   EXPECT_FALSE(style.padding(Edge::Right).isDefined());
 }
 
+TEST(ScrollContentViewShadowNodeTest, keepsPaddingLargerThanTheInset) {
+  yoga::Style style;
+  style.setPadding(Edge::Horizontal, StyleLength::points(100));
+
+  ScrollContentViewShadowNode::padForSafeArea(
+      style, stateWith({.left = 0, .top = 0, .right = 84, .bottom = 0}), kLTR);
+
+  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(100));
+  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(100));
+}
+
 TEST(ScrollContentViewShadowNodeTest, prefersLogicalOverPhysicalEdge) {
   yoga::Style style;
   style.setPadding(Edge::Left, StyleLength::points(4));
-  style.setPadding(Edge::Start, StyleLength::points(12));
+  style.setPadding(Edge::Start, StyleLength::points(40));
 
   ScrollContentViewShadowNode::padForSafeArea(
       style, stateWith({.left = 20, .top = 0, .right = 0, .bottom = 0}), kLTR);
 
-  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(32));
+  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(40));
 }
 
 TEST(ScrollContentViewShadowNodeTest, keepsPercentageAndSkipsInsetOnThatEdge) {
@@ -92,7 +103,7 @@ TEST(ScrollContentViewShadowNodeTest, padsPointsEdgeBesideAPercentageEdge) {
       style, stateWith({.left = 20, .top = 0, .right = 84, .bottom = 0}), kLTR);
 
   EXPECT_EQ(style.padding(Edge::Start), StyleLength::percent(10));
-  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(88));
+  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(84));
 }
 
 TEST(ScrollContentViewShadowNodeTest, padsPhysicalEdgesInRTL) {
@@ -103,7 +114,7 @@ TEST(ScrollContentViewShadowNodeTest, padsPhysicalEdgesInRTL) {
       style, stateWith({.left = 0, .top = 0, .right = 84, .bottom = 0}), kRTL);
 
   // In RTL the start edge is on the right, where the inset is.
-  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(96));
+  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(84));
   EXPECT_FALSE(style.padding(Edge::End).isDefined());
 }
 
@@ -118,15 +129,15 @@ TEST(ScrollContentViewShadowNodeTest, readsSwappedPhysicalEdgesInRTL) {
       kRTL);
 
   // Left is copied over start at layout, so it lands on the right, and the
-  // inset on the left adds to the end edge.
+  // inset on the left raises the end edge.
   EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(8));
-  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(86));
+  EXPECT_EQ(style.padding(Edge::End), StyleLength::points(84));
   EXPECT_FALSE(style.padding(Edge::Left).isDefined());
 }
 
 TEST(ScrollContentViewShadowNodeTest, readsSwappedPhysicalEdgesInLTRSubtree) {
   yoga::Style style;
-  style.setPadding(Edge::Left, StyleLength::points(8));
+  style.setPadding(Edge::Left, StyleLength::points(100));
   style.setPadding(Edge::Start, StyleLength::points(2));
 
   ScrollContentViewShadowNode::padForSafeArea(
@@ -136,7 +147,7 @@ TEST(ScrollContentViewShadowNodeTest, readsSwappedPhysicalEdgesInLTRSubtree) {
 
   // The swap applies to the whole tree, so left is still copied over start,
   // which a node laid out left to right keeps on the left.
-  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(92));
+  EXPECT_EQ(style.padding(Edge::Start), StyleLength::points(100));
   EXPECT_FALSE(style.padding(Edge::End).isDefined());
 }
 
@@ -147,8 +158,8 @@ TEST(ScrollContentViewShadowNodeTest, padsColumnForHorizontalScrollView) {
   ScrollContentViewShadowNode::padForSafeArea(
       style, stateWith({.left = 0, .top = 20, .right = 0, .bottom = 34}), kLTR);
 
-  EXPECT_EQ(style.padding(Edge::Top), StyleLength::points(24));
-  EXPECT_EQ(style.padding(Edge::Bottom), StyleLength::points(38));
+  EXPECT_EQ(style.padding(Edge::Top), StyleLength::points(20));
+  EXPECT_EQ(style.padding(Edge::Bottom), StyleLength::points(34));
   EXPECT_EQ(style.padding(Edge::Vertical), StyleLength::points(4));
 }
 

packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js

@@ -1832,6 +1832,7 @@ const styles = StyleSheet.create({
   },
   fullScreenContentContainer: {
     backgroundColor: '#aae3b6',
+    paddingHorizontal: 16,
   },
   fullScreenControls: {
     backgroundColor: '#eeeeee',