feat: Add logical border inline color properties #36046

gabrieldonadel · 2023-02-03T04:49:42Z

Summary:

This PR implements logical border inline color properties as requested on #34425. This implementation includes the addition of the following style properties

borderInlineColor, equivalent to borderEndColor and borderStartColor.
borderInlineEndColor, equivalent to borderEndColor.
borderInlineStartColor, equivalent to borderStartColor.

Changelog:

[GENERAL] [ADDED] - Add logical border inline color properties

Test Plan:

Open the RNTester app and navigate to the View page
Test the new style properties through the Logical Border Color section

Android	iOS
Screen.Recording.2023-02-03.at.01.40.10.mov	Screen.Recording.2023-02-02.at.23.28.40.mov

gabrieldonadel · 2023-02-03T04:50:40Z

ReactCommon/react/renderer/components/view/primitives.h

+  OptionalT leftEdge{};
+  OptionalT topEdge{};
+  OptionalT rightEdge{};
+  OptionalT bottomEdge{};
+  OptionalT startEdge{};
+  OptionalT endEdge{};
+  OptionalT horizontalEdges{};
+  OptionalT verticalEdges{};
+  OptionalT allEdges{};
+  OptionalT blockEdges{};
+  OptionalT blockStartEdge{};
+  OptionalT blockEndEdge{};
+  OptionalT inlineEdges{};
+  OptionalT inlineStartEdge{};
+  OptionalT inlineEndEdge{};


Unfortunately I had to rename these because inline is a reserved word in C++

analysis-bot · 2023-02-03T05:15:15Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,521,091	+1,919
android	hermes	armeabi-v7a	7,836,854	+2,154
android	hermes	x86	9,001,725	+2,219
android	hermes	x86_64	8,856,514	+1,756
android	jsc	arm64-v8a	9,144,237	+1,763
android	jsc	armeabi-v7a	8,335,824	+1,982
android	jsc	x86	9,199,281	+2,052
android	jsc	x86_64	9,456,929	+1,594

Base commit: 803bb16
Branch: main

NickGerleman · 2023-03-14T08:44:22Z

React/Views/RCTView.m

+    UIColor *borderStartColor = _borderInlineStartColor ?: _borderInlineColor ?: _borderStartColor ?: _borderLeftColor;
+    UIColor *borderEndColor = _borderInlineEndColor ?: _borderInlineColor ?: _borderEndColor ?: _borderRightColor;

    directionAwareBorderLeftColor = isRTL ? borderEndColor : borderStartColor;
    directionAwareBorderRightColor = isRTL ? borderStartColor : borderEndColor;
  } else {
-    directionAwareBorderLeftColor = (isRTL ? _borderEndColor : _borderStartColor) ?: _borderLeftColor;
-    directionAwareBorderRightColor = (isRTL ? _borderStartColor : _borderEndColor) ?: _borderRightColor;
+    UIColor *borderStartColor = _borderInlineStartColor ?: _borderInlineColor ?: _borderStartColor;
+    UIColor *borderEndColor = _borderInlineEndColor ?: _borderInlineColor ?: _borderEndColor;
+
+    directionAwareBorderLeftColor = (isRTL ? borderEndColor : borderStartColor) ?: _borderLeftColor;
+    directionAwareBorderRightColor = (isRTL ? borderStartColor : borderEndColor) ?: _borderRightColor;
  }


Could you explain what these conditions are doing, and why this change puts precedence in this order?

NickGerleman · 2023-03-14T08:46:55Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/Spacing.java

+  /** Spacing type that represents inline directions (left, right). E.g. {@code marginInline}. */
+  public static final int INLINE = 12;
+  /**
+   * Spacing type that represents the inline end direction (right in left-to-right, left in
+   * right-to-left). E.g. {@code marginInlineEnd}.
+   */
+  public static final int INLINE_END = 13;
+  /**
+   * Spacing type that represents the inline start direction (left in left-to-right, right in
+   * right-to-left). E.g. {@code marginInlineStart}.
+   */
+  public static final int INLINE_START = 14;


Same comment as the inline PR, it could create confusion to have Spacing.END not be the last enum here. As we are adding, I think we should update this. If there is existing code relying on Spacing.END which only needs the ones before it, we should add a new well-known enum where Spacing.END used to be.

NickGerleman · 2023-03-14T08:49:00Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/Spacing.java

      YogaConstants.UNDEFINED,
      YogaConstants.UNDEFINED,
      YogaConstants.UNDEFINED,
+      YogaConstants.UNDEFINED,


Is it possible to specify in the type that the array is of size Spacing.ALL so that when we add an enum, we are forced to update this?

NickGerleman · 2023-03-14T08:56:40Z

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java

          }
        }

+        // colorInlineStart and colorInlineEnd  have precedence over colorEnd and colorStart values


This function is already pretty chunky. Can we cleanly pull out the logic to resolve these values?

NickGerleman · 2023-03-14T08:58:31Z

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java

+      final boolean isColorInlineStartDefined = isBorderColorDefined(Spacing.INLINE_START);
+      final boolean isColorInlineEndDefined = isBorderColorDefined(Spacing.INLINE_END);
+
+      final boolean isDirectionAwareColorInlineLeftDefined =


Is this the same as the above? Can we avoid duplication?

necolas · 2023-06-01T17:23:03Z

What are the next steps to get this PR merged?

necolas · 2023-09-11T17:55:59Z

Should we expect this PR to be further updated?

jkasprzyk17 · 2024-01-27T22:35:48Z

Hey, @gabrieldonadel @NickGerleman are you still working on this or can I continue this on my own ?

NickGerleman · 2024-01-29T01:29:07Z

Hey @JakubKasprzyk17, I have not looked at this change in a while, but I have actually been spending some more time recently around this layer, and we have some more tests over this now, so we are probably in a better place to make the change than before.

For you, or anyone else interested in fleshing out logical edges for border props, I have a few updates:

We have now more universally been targeting new architecture only. This is a lot easier to reason about, because of how it stores props, and cuts down the matrix a good bit. E.g. on iOS, we only need to worry about ComponentView and not the Paper native component impl.
We have more UI tests against border scenarios in RNTester, for both Paper and Fabric. Tests against RNTester and products run continuously, but Android (and soon iOS) we will run tests when we try to land, which sees if rendering of RNTester scenarios (we have more for border now) changed.
I have been trying to move up the layer where we "compute" physical edges from logical edges, so interesting code should always get a representation that is just "left, right, top, bottom". That makes the eventual sink dealing with borders a lot easier to reason about, and more efficient
This code has changed a lot since this PR
I have a current change in progress, that I'm trying to land next week, that refactors how we store and process these border values in the RN Fabric layout prop storage logic (YogaStylableProps). This is an invasive change, but will make the model a lot easier to reason about.

NickGerleman · 2024-01-29T01:40:31Z

Oh, we also removed the MapBuffer ViewProps, and iterator style props parser code for layout props (and we could also for view props if we want to).

I.e. matrix for this should be a lottt smaller than before. And we no longer want to worry about Paper specific bugs.

NickGerleman · 2024-01-29T02:34:55Z

Also no more deprecated-prop-types in a separate repo

jkasprzyk17 · 2024-01-29T12:54:05Z

I made:
• borderInlineColor equivalent to borderEndColor and borderStartColor
• borderInlineEndColor equivalent to borderEndColor
• borderInlineStartColor equivalent to borderStartColor
Following @gabrieldonadel work in this PR #35999.
Without changes in MapBuffer View Props, because was removed.

If you want to check my changes I can push PR.

NickGerleman · 2024-01-29T19:29:00Z

@JakubKasprzyk17 please take caution the PR you mention matching against ended up needing changes after changing some border rendering behavior.

For future contributions, I would want us to:

Not touch Paper at all. I.e. no edits to RCTView or RCTViewManager.
Add test cases, that we can show are rendering correctly
Avoid any code which needs to do interesting logic with 12 or more edges, that should really only be dealing with the four physical edges. I.e. factor this as converting logical edge, to physical edge, before interesting logic, and not otherwise ever dealing with logical edge directly.

Some of the churn with this original series of changes was that we would sometimes find a visual regression, that we had a hard time reproing and communication, but I think if we follow all the above guidelines, we can end up with a more scoped, lower risk change.

jkasprzyk17 · 2024-01-29T19:44:30Z

@NickGerleman if I'm right we must change whole flow with new borderColor and rewrite
borderInline and borderBlock. Operate only on 4 physical edges. But now I've no idea how to do it without changes in RCTView and RCTViewManager

NickGerleman · 2024-01-29T21:27:12Z

But now I've no idea how to do it without changes in RCTView and RCTViewManager

These are only used by legacy architecture, compared to RCTViewComponentView for new architecture.

react-native-bot · 2024-07-28T05:01:01Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot · 2024-07-28T05:01:01Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot · 2024-08-04T05:13:24Z

This PR was closed because it has been stalled for 7 days with no activity.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 3, 2023

gabrieldonadel commented Feb 3, 2023

View reviewed changes

gabrieldonadel mentioned this pull request Feb 3, 2023

[w3c] ☂️ Web Styles (Part 1) umbrella issue #34425

Closed

74 tasks

gabrieldonadel mentioned this pull request Feb 5, 2023

feat: Add logical border color props facebook/react-native-website#3554

Closed

NickGerleman self-requested a review February 10, 2023 02:50

gabrieldonadel force-pushed the feat/add-logical-border-inline-color branch from 6e4263f to 918bbbf Compare March 14, 2023 03:05

NickGerleman reviewed Mar 14, 2023

View reviewed changes

gabrieldonadel added 4 commits March 29, 2023 22:13

feat: Add borderInlineColor implementation on iOS Paper

ea4fdea

Add Edge suffix to CascadedRectangleEdges properties

e232d91

Add borderInlineColor implementation on iOS Fabric

3551999

feat: Add borderBlockColor implementation on Android

1484508

gabrieldonadel force-pushed the feat/add-logical-border-inline-color branch from 918bbbf to 1484508 Compare March 30, 2023 01:14

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 8, 2023

react-native-bot added Stale There has been a lack of activity on this issue and it may be closed soon. labels Jul 28, 2024

react-native-bot closed this Aug 4, 2024

feat: Add logical border inline color properties #36046

feat: Add logical border inline color properties #36046

Uh oh!

Conversation

gabrieldonadel commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changelog:

Test Plan:

Uh oh!

gabrieldonadel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

analysis-bot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickGerleman Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

NickGerleman Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

NickGerleman Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

NickGerleman Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

NickGerleman Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

necolas commented Jun 1, 2023

Uh oh!

necolas commented Sep 11, 2023

Uh oh!

jkasprzyk17 commented Jan 27, 2024

Uh oh!

NickGerleman commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickGerleman commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickGerleman commented Jan 29, 2024

Uh oh!

jkasprzyk17 commented Jan 29, 2024

Uh oh!

NickGerleman commented Jan 29, 2024

Uh oh!

jkasprzyk17 commented Jan 29, 2024

Uh oh!

NickGerleman commented Jan 29, 2024

Uh oh!

react-native-bot commented Jul 28, 2024

Uh oh!

react-native-bot commented Jul 28, 2024

Uh oh!

react-native-bot commented Aug 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gabrieldonadel commented Feb 3, 2023 •

edited

Loading

analysis-bot commented Feb 3, 2023 •

edited

Loading

NickGerleman commented Jan 29, 2024 •

edited

Loading

NickGerleman commented Jan 29, 2024 •

edited

Loading