feat: Add logical border radius implementation #35572

gabrieldonadel · 2022-12-06T02:56:56Z

Summary

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

borderEndEndRadius, equivalent to borderBottomEndRadius.
borderEndStartRadius, equivalent to borderBottomStartRadius.
borderStartEndRadius, equivalent to borderTopEndRadius.
borderStartStartRadius, equivalent to borderTopStartRadius.

Changelog

[GENERAL] [ADDED] - Add logical border-radius implementation

Test Plan

Open the RNTester app and navigate to the RTLExample page
Test the new style properties through the Logical Border Radii Start/End section

Screen.Recording.2022-12-09.at.01.24.01.mov

gabrieldonadel · 2022-12-06T15:42:21Z

@NickGerleman quick question, on #35342 you used CompactValue for all new props but here I need to assign this to a CGFloat, do you know what's the easiest way to achieve that?

NickGerleman · 2022-12-06T15:58:03Z

I think this change might need to take a different approach. borderRadius isn't used by or forwarded to Yoga, so I would think this change will no-op. I think if we wanted to do this in native we would need to do it in the separate iOS and Android view managers.

NickGerleman · 2022-12-06T16:05:59Z

I think what controls this on Android for both Paper and Fabric are all of the setBorderRadius functions in ReactAndroid bound to ViewProps.BORDER_RADIUS and similar.

The layout props are reused between all views/platforms in Fabric, which is why we can get away with an easy native implementation for it. But for props that control style different from positioning/sizing, the implementation gets a bit more involved.

gabrieldonadel · 2022-12-07T12:37:57Z

Thanks for the explanation @NickGerleman! I'll update this PR so we use this other approach

analysis-bot · 2022-12-08T03:57:24Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: a3c47d3
Branch: main

analysis-bot · 2022-12-08T04:01:13Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,778,726	+1,923
android	hermes	armeabi-v7a	7,098,626	+2,033
android	hermes	x86	8,253,461	+1,756
android	hermes	x86_64	8,111,220	+2,139
android	jsc	arm64-v8a	8,971,312	+1,693
android	jsc	armeabi-v7a	7,704,507	+1,805
android	jsc	x86	9,035,307	+1,540
android	jsc	x86_64	9,512,632	+1,917

Base commit: a3c47d3
Branch: main

pull-bot · 2022-12-08T04:11:16Z

PR build artifact for 9bafd39 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-12-09T04:22:30Z

Hi @NickGerleman I managed to get this working on iOS on both Fabric and Paper, do you mind taking a look at this PR to check if I'm going in the right direction? I am not too familiar with Yoga so I could have implemented something totally off by accident

pull-bot · 2022-12-09T04:51:10Z

PR build artifact for 47972c0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-12-12T05:51:15Z

PR build artifact for dae78fd is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-12-12T13:03:48Z

PR build artifact for b0dcb5a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-12-12T13:30:46Z

@necolas just FYI this is now ready for review

NickGerleman

This is great work! I know this sort of change is a lot of pattern matching, so I will mostly defer to that this has been validated for the Matrix of iOS/Android Paper/Fabric, since this touches each of those slices.

I left a couple comments on the implementation, with the main one around how we should give precedence to cardinal directions over flow relative directions for consistency with the resolution of existing props.

Ideally we would add some UI tests with this change to validate it, but the infra to do that from OSS isn't really there yet. Though @necolas or I could potentially followup since there is some Meta internal infrastructure to do visual similarity (screenshot) testing of specific views inside RNTester.

NickGerleman · 2022-12-12T12:58:20Z

React/Views/RCTView.m

-    const CGFloat topEndRadius = RCTDefaultIfNegativeTo(_borderTopRightRadius, _borderTopEndRadius);
-    const CGFloat bottomStartRadius = RCTDefaultIfNegativeTo(_borderBottomLeftRadius, _borderBottomStartRadius);
-    const CGFloat bottomEndRadius = RCTDefaultIfNegativeTo(_borderBottomRightRadius, _borderBottomEndRadius);
+    const CGFloat topStartRadius = RCTDefaultIfNegativeTo(_borderTopLeftRadius, _borderStartStartRadius > 0 ? _borderStartStartRadius: _borderTopStartRadius);


I don't think > 0 is the correct check, since it would mean we would ignore something setting the value to 0. On this line it looks like RCTDefaultIfNegativeTo is also already being used to set precedence, so mixing it with a ternary doing the same is a little confusing.

Yeah good point, I guess we could check for > -1 as all _border**Radius variables are initialized using -1, but I've refactored this a bit to remove the ternaries and now everything is using RCTDefaultIfNegativeTo

NickGerleman · 2022-12-12T13:04:52Z

React/Views/RCTView.m

-    const CGFloat bottomEndRadius = RCTDefaultIfNegativeTo(_borderBottomRightRadius, _borderBottomEndRadius);
+    const CGFloat topStartRadius = RCTDefaultIfNegativeTo(_borderTopLeftRadius, _borderStartStartRadius > 0 ? _borderStartStartRadius: _borderTopStartRadius);
+    const CGFloat topEndRadius = RCTDefaultIfNegativeTo(_borderTopRightRadius, _borderStartEndRadius > 0 ? _borderStartEndRadius: _borderTopEndRadius);
+    const CGFloat bottomStartRadius = RCTDefaultIfNegativeTo(_borderBottomLeftRadius, _borderEndStartRadius > 0 ? _borderEndStartRadius: _borderBottomStartRadius);


For consistency with other properties here (and Yoga's stylesheet behavior), we should give preference to the cardinal direction over the flow relative one. E.g. Top should be taken instead of Start when both are present.

Makes sense, I've just updated it

NickGerleman · 2022-12-12T13:17:59Z

Libraries/NativeComponent/BaseViewConfig.ios.js

@@ -221,6 +221,10 @@ const validAttributesForNonEventProps = {
  borderBottomRightRadius: true,
  borderBottomStartRadius: true,
  borderBottomEndRadius: true,
+  borderEndEndRadius: true,


cc @yungsters who recently updated these for prop-types support facebook/react-native-deprecated-modules@5902411

RN's exported prop-types for View aren't enforced by RN itself, like it does for Text or Image. But I suspect we might still want to update ViewPropTypes before RN re-ejects it.

NickGerleman · 2022-12-12T13:21:28Z

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

-      final float directionAwareTopRightRadius = isRTL ? topStartRadius : topEndRadius;
-      final float directionAwareBottomLeftRadius = isRTL ? bottomEndRadius : bottomStartRadius;
-      final float directionAwareBottomRightRadius = isRTL ? bottomStartRadius : bottomEndRadius;
+      final float logicalTopStartRadius = !YogaConstants.isUndefined(startStartRadius) ? startStartRadius : topStartRadius;


Same comment as above re reversing the precedence here.

Sure, updated

NickGerleman · 2022-12-12T13:31:06Z

ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+  if (props.borderRadii.startEnd.has_value()) {
+    props.borderRadii.topEnd = props.borderRadii.startEnd;
+    props.borderRadii.startEnd.reset();
+  }
+
+  if (props.borderRadii.startStart.has_value()) {
+    props.borderRadii.topStart = props.borderRadii.startStart;
+    props.borderRadii.startStart.reset();
+  }
+
+  if (props.borderRadii.endEnd.has_value()) {
+    props.borderRadii.bottomEnd = props.borderRadii.endEnd;
+    props.borderRadii.endEnd.reset();
+  }
+
+  if (props.borderRadii.endStart.has_value()) {
+    props.borderRadii.bottomStart = props.borderRadii.endStart;
+    props.borderRadii.endStart.reset();
+  }
+


Looking at the examples below, they are always changing a left-or-right cardinal direction to a flow-relative one. E.g. nothing is done to borderRadii.topStart. But the added code seems to be doing the opposite? E.g. changing start/end to a top-bottom direction.

Are you sure the added code here is needed?

Thanks for the review @NickGerleman, I did some investigation and actually, this code is not necessary, everything works just fine without it. I've pushed another commit and now all comments should've been addressed

NickGerleman · 2022-12-12T13:36:35Z

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

+    const auto topLeading = isRTL ? startEnd ? startEnd : topEnd
+        : startStart              ? startStart
+                                  : topStart;


Same comment as elsewhere about precedence, but this ternary is a bit hard to read as-is. Could we avoid the nested ternary (or maybe this is fine when formatted)?

Yeah that logic was pretty confusing, I've just updated it making things a bit easier to read

pull-bot · 2022-12-13T01:41:52Z

PR build artifact for d3d2bd9 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-12-13T02:59:59Z

PR build artifact for 71fcbdd is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-12-13T16:28:29Z

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

gabrieldonadel · 2022-12-14T13:07:53Z

Hi @NickGerleman do you mind checking for me what's this Facebook Internal Linter warning about?

NickGerleman · 2022-12-14T22:09:26Z

Hi @NickGerleman do you mind checking for me what's this Facebook Internal Linter warning about?

It's complaining because there was a CircleCI failure (unrelated to this change). Everything else is passing minus some clang format warnings we can fixup.

gabrieldonadel · 2023-01-05T01:15:44Z

Hi @NickGerleman, hope you had a great New Year's holiday! I just wanted to follow up on this PR. Is there anything that still needs to be addressed or resolved on my side?

NickGerleman · 2023-01-06T06:30:22Z

Nothing on your side, this should be imported soon (have a bit of a queue atm).

facebook-github-bot · 2023-01-06T14:03:48Z

@NickGerleman merged this pull request in 4ae4984.

Summary: This PR implements logical border-radius as requested on facebook#34425. This implementation includes the addition of the following style properties - `borderEndEndRadius`, equivalent to `borderBottomEndRadius`. - `borderEndStartRadius`, equivalent to `borderBottomStartRadius`. - `borderStartEndRadius`, equivalent to `borderTopEndRadius`. - `borderStartStartRadius`, equivalent to `borderTopStartRadius`. ## Changelog [GENERAL] [ADDED] - Add logical border-radius implementation Pull Request resolved: facebook#35572 Test Plan: 1. Open the RNTester app and navigate to the `RTLExample` page 2. Test the new style properties through the `Logical Border Radii Start/End` section https://user-images.githubusercontent.com/11707729/206623732-6d542347-93f9-40da-be97-f7dcd5f66ca9.mov Reviewed By: necolas Differential Revision: D42002043 Pulled By: NickGerleman fbshipit-source-id: a0aa9783c280398b437aeb7a00c6eb3f767657a5

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 Dec 6, 2022

gabrieldonadel force-pushed the feat/add-logical-border-radius branch from e3d2a0f to c656a18 Compare December 6, 2022 03:26

necolas mentioned this pull request Dec 7, 2022

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

Open

74 tasks

necolas requested a review from NickGerleman December 7, 2022 21:57

gabrieldonadel changed the title ~~feat: Add Fabric logical border radius implementation~~ feat: Add logical border radius implementation Dec 8, 2022

gabrieldonadel force-pushed the feat/add-logical-border-radius branch from c656a18 to 9bafd39 Compare December 8, 2022 03:37

gabrieldonadel added 3 commits December 11, 2022 19:03

feat: Add logical border radius Paper implementation for iOS

f165b23

feat: Add logical border radius Fabric implementation for iOS

04db275

feat: Add logical border radius Paper implementation for Android

dae78fd

gabrieldonadel force-pushed the feat/add-logical-border-radius branch from 47972c0 to dae78fd Compare December 12, 2022 04:34

feat: Add logical border radius Fabric implementation for Android

b0dcb5a

gabrieldonadel marked this pull request as ready for review December 12, 2022 12:32

NickGerleman requested changes Dec 12, 2022

View reviewed changes

chore: Update logic to give precedence to cardinal directions

d3d2bd9

chore: Revert Yoga swapLeftAndRightInViewProps changes

71fcbdd

gabrieldonadel requested a review from NickGerleman December 13, 2022 02:29

NickGerleman mentioned this pull request Jan 6, 2023

feat: Add inset logical properties facebook/react-native-deprecated-modules#12

Merged

facebook-github-bot closed this in 4ae4984 Jan 6, 2023

facebook-github-bot added the Merged This PR has been merged. label Jan 6, 2023

gabrieldonadel deleted the feat/add-logical-border-radius branch January 6, 2023 14:27

tarunrajput mentioned this pull request Jun 2, 2023

add logical border radius props facebook/react-native-website#3745

Merged

NickGerleman mentioned this pull request Jun 8, 2023

RN 0.72: Broken border colors / radius on Android #37753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add logical border radius implementation #35572

feat: Add logical border radius implementation #35572

gabrieldonadel commented Dec 6, 2022 •

edited

Loading

gabrieldonadel commented Dec 6, 2022

NickGerleman commented Dec 6, 2022 •

edited

Loading

NickGerleman commented Dec 6, 2022

gabrieldonadel commented Dec 7, 2022

analysis-bot commented Dec 8, 2022 •

edited

Loading

analysis-bot commented Dec 8, 2022 •

edited

Loading

pull-bot commented Dec 8, 2022

gabrieldonadel commented Dec 9, 2022

pull-bot commented Dec 9, 2022

pull-bot commented Dec 12, 2022

pull-bot commented Dec 12, 2022

gabrieldonadel commented Dec 12, 2022

NickGerleman left a comment

NickGerleman Dec 12, 2022

gabrieldonadel Dec 13, 2022

NickGerleman Dec 12, 2022

gabrieldonadel Dec 13, 2022

NickGerleman Dec 12, 2022

NickGerleman Dec 12, 2022

gabrieldonadel Dec 13, 2022

NickGerleman Dec 12, 2022

gabrieldonadel Dec 13, 2022

NickGerleman Dec 12, 2022

gabrieldonadel Dec 13, 2022

pull-bot commented Dec 13, 2022

pull-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

gabrieldonadel commented Dec 14, 2022

NickGerleman commented Dec 14, 2022

gabrieldonadel commented Jan 5, 2023

NickGerleman commented Jan 6, 2023

facebook-github-bot commented Jan 6, 2023

feat: Add logical border radius implementation #35572

feat: Add logical border radius implementation #35572

Conversation

gabrieldonadel commented Dec 6, 2022 • edited Loading

Summary

Changelog

Test Plan

gabrieldonadel commented Dec 6, 2022

NickGerleman commented Dec 6, 2022 • edited Loading

NickGerleman commented Dec 6, 2022

gabrieldonadel commented Dec 7, 2022

analysis-bot commented Dec 8, 2022 • edited Loading

analysis-bot commented Dec 8, 2022 • edited Loading

pull-bot commented Dec 8, 2022

gabrieldonadel commented Dec 9, 2022

pull-bot commented Dec 9, 2022

pull-bot commented Dec 12, 2022

pull-bot commented Dec 12, 2022

gabrieldonadel commented Dec 12, 2022

NickGerleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented Dec 13, 2022

pull-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

gabrieldonadel commented Dec 14, 2022

NickGerleman commented Dec 14, 2022

gabrieldonadel commented Jan 5, 2023

NickGerleman commented Jan 6, 2023

facebook-github-bot commented Jan 6, 2023

gabrieldonadel commented Dec 6, 2022 •

edited

Loading

NickGerleman commented Dec 6, 2022 •

edited

Loading

analysis-bot commented Dec 8, 2022 •

edited

Loading

analysis-bot commented Dec 8, 2022 •

edited

Loading