feat(android): percentage support in translate #43193

intergalacticspacehighway · 2024-02-25T14:19:08Z

Summary:

This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews.

Changelog:

[Android] [ADDED] - Percentage support in translate

Test Plan:

Checkout TransformExample.js -> Translate percentage example.
Added a simple test in processTransform-test.js. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!)

Related PRs - #43191, #43192

analysis-bot · 2024-02-25T14:48:17Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,882,328	+198
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,240,669	+98
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 12f2c3c
Branch: main

javache

Looks good! A few questions.

packages/react-native/Libraries/StyleSheet/processTransform.js

javache · 2024-03-04T12:41:36Z

...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java

@@ -177,18 +177,15 @@ public void setBackgroundColor(@NonNull T view, int backgroundColor) {
  public void setTransform(@NonNull T view, @Nullable ReadableArray matrix) {
    view.setTag(R.id.transform, matrix);
    view.setTag(R.id.invalidate_transform, true);
+    view.addOnLayoutChangeListener(this);


We probably discussed this in the original transformOrigin PR, but why can't we override layout (or as Android prefers onLayout) to update the transform?

Yeah i remember discussing it here - #38558 (comment). To override layout i think we'll have to move transform logic to ReactViewGroup.

onLayout runs during layout pass and can be used to update child view positions, in our case we update transform of the view. Not very ideal, i guess in the long run moving it to RootViewGroup might be better.

I checked code, and Android will no-op if adding existing listener, but it's a little bit smelly that we can add ourselves as a listener, after we have already added ourselves as listener. Not sure a better way though (probably don't want to waste cycles to always attach).

Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

Summary: This PR adds percentage support in translate properties for new arch iOS. Isolating this PR for easier reviews. The approach taken here introduces usage of `ValueUnit` struct for transform operations so it can support `%` in translates and delay the generation of actual transform matrix until view dimensions are known. I have tried to keep the changes minimal and reuse existing APIs, open to changes if there's an alternative approach. ## Changelog: [IOS] [ADDED] - Percentage support in translate in new arch. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #43192 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - #43193, #43191 Reviewed By: javache Differential Revision: D56802425 Pulled By: NickGerleman fbshipit-source-id: 978cbbdde004afe1e68ffee9a3c7eb7d16336b46

NickGerleman

Left a couple of minor comments, but this seems pretty nicely scoped 👍

NickGerleman · 2024-05-22T13:02:24Z

...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java

@@ -177,18 +177,15 @@ public void setBackgroundColor(@NonNull T view, int backgroundColor) {
  public void setTransform(@NonNull T view, @Nullable ReadableArray matrix) {
    view.setTag(R.id.transform, matrix);
    view.setTag(R.id.invalidate_transform, true);
+    view.addOnLayoutChangeListener(this);


I checked code, and Android will no-op if adding existing listener, but it's a little bit smelly that we can add ourselves as a listener, after we have already added ourselves as listener. Not sure a better way though (probably don't want to waste cycles to always attach).

NickGerleman · 2024-05-22T13:07:40Z

...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java

@@ -171,7 +171,7 @@ public void onLayoutChange(
    if ((currentHeight != oldHeight || currentWidth != oldWidth)) {
      ReadableArray transformOrigin = (ReadableArray) v.getTag(R.id.transform_origin);
      ReadableArray transformMatrix = (ReadableArray) v.getTag(R.id.transform);
-      if (transformMatrix != null && transformOrigin != null) {
+      if (transformMatrix != null || transformOrigin != null) {


This isn't a matrix most of the time, right? Can we rename?

Agree, renamed to transforms since it's an array.

NickGerleman · 2024-05-22T13:10:36Z

...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/TransformHelper.java

@@ -130,6 +152,14 @@ public static void processTransform(
    }
  }

+  private static double parsePercentageToValue(String stringValue, double dimension) {
+    if (stringValue.endsWith("%")) {


I'm not sure the old code was reliable here, but we usually want to avoid native crashes on invalid style values. So we probably want to be handling NumberFormatException somewhere.

makes sense, updated!

intergalacticspacehighway · 2024-05-22T18:30:25Z

I checked code, and Android will no-op if adding existing listener, but it's a little bit smelly that we can add ourselves as a listener, after we have already added ourselves as listener. Not sure a better way though (probably don't want to waste cycles to always attach).

i moved it in onAfterUpdateTransaction so it gets called a bit less and yes duplicate calls for same listener will be ignored. But i agree this is not very optimal. If all views are getting extended from ReactViewGroup then we can move the entire transform logic there as it already has a onSizeCallback as mentioned here.

facebook-github-bot · 2024-05-23T12:47:10Z

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

facebook-github-bot · 2024-05-24T12:31:41Z

@NickGerleman merged this pull request in c13790f.

github-actions · 2024-05-24T12:31:44Z

This pull request was successfully merged by @intergalacticspacehighway in c13790f.

^{When will my fix make it into a release? | How to file a pick request?}

Summary: This PR adds percentage support in translate properties for new arch iOS. Isolating this PR for easier reviews. The approach taken here introduces usage of `ValueUnit` struct for transform operations so it can support `%` in translates and delay the generation of actual transform matrix until view dimensions are known. I have tried to keep the changes minimal and reuse existing APIs, open to changes if there's an alternative approach. ## Changelog: [IOS] [ADDED] - Percentage support in translate in new arch. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#43192 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - facebook#43193, facebook#43191 Reviewed By: javache Differential Revision: D56802425 Pulled By: NickGerleman fbshipit-source-id: 978cbbdde004afe1e68ffee9a3c7eb7d16336b46

Summary: This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews. ## Changelog: [Android] [ADDED] - Percentage support in translate <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#43193 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - facebook#43191, facebook#43192 Reviewed By: joevilches Differential Revision: D57723216 Pulled By: NickGerleman fbshipit-source-id: c9da007678341b62745df858f043821bcc662a98

intergalacticspacehighway added 6 commits February 19, 2024 12:00

android translate percentage support

145a69d

process transform update

42ebf7c

Merge branch 'main' into android-translate-percentage

e1f6c08

Merge branch 'main' into android-translate-percentage

e1948d9

translate regex to support %

f25cf04

translate percentage test

4faf27e

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 25, 2024

This was referenced Feb 25, 2024

feat(iOS/fabric): percentage support in translate #43192

Closed

feat(iOS/paper): percentage support in translate #43191

Open

javache reviewed Mar 4, 2024

View reviewed changes

intergalacticspacehighway and others added 4 commits March 31, 2024 01:27

Update packages/react-native/Libraries/StyleSheet/processTransform.js

91812cd

Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

flow check fix

274ba79

prettier fix

f6a528c

fix flow check

eaee88b

Merge branch 'main' into android-translate-percentage

55e2453

intergalacticspacehighway requested a review from javache May 20, 2024 03:36

NickGerleman reviewed May 22, 2024

View reviewed changes

intergalacticspacehighway added 3 commits May 22, 2024 23:26

handle NumberFormatException exception

6b1a151

rename transforms array

bf8e03b

move add layout listener to after update transaction

5e70511

facebook-github-bot closed this in c13790f May 24, 2024

facebook-github-bot added the Merged This PR has been merged. label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(android): percentage support in translate #43193

feat(android): percentage support in translate #43193

intergalacticspacehighway commented Feb 25, 2024 •

edited

analysis-bot commented Feb 25, 2024

javache left a comment

javache Mar 4, 2024

intergalacticspacehighway Mar 30, 2024

NickGerleman May 22, 2024

NickGerleman left a comment

NickGerleman May 22, 2024

NickGerleman May 22, 2024

intergalacticspacehighway May 22, 2024

NickGerleman May 22, 2024

intergalacticspacehighway May 22, 2024

intergalacticspacehighway commented May 22, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 24, 2024

github-actions bot commented May 24, 2024

feat(android): percentage support in translate #43193

feat(android): percentage support in translate #43193

Conversation

intergalacticspacehighway commented Feb 25, 2024 • edited

Summary:

Changelog:

Test Plan:

analysis-bot commented Feb 25, 2024

javache 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

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

intergalacticspacehighway commented May 22, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 24, 2024

github-actions bot commented May 24, 2024

intergalacticspacehighway commented Feb 25, 2024 •

edited