feat: Use animation matrix For Skew prop#38494
Conversation
|
Hi @xxrlzzz! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Original Effect: original.mp4Fixed Effect: fixed.mp4 |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Base commit: 8ff05b5 |
There was a problem hiding this comment.
You need to process all transforms in order, you can't have separate loops for translate and other properties, as it may cause the resulting matrix to be incorrect.
There was a problem hiding this comment.
I don't sure disorder of properties will leds a incorrect result matrix, since every property are handled by matrix multiply rather direct set matrix element.
The reason for separate translate and other properties is we need to know the real center of the view to set origin of rotate and scale (default origin is the top-left point of the view, rather center of the view like it used to be)
e2eeaaf to
14e6815
Compare
14e6815 to
87482db
Compare
|
I note there is already has We can use implementation form AdnroidX transition library to access this api saftly. |
|
Compatibility test (device with api before android Q) 2023-07-26.19.38.56.mov2023-07-26.19.18.00.mov |
87482db to
5ba54ff
Compare
|
This pr is current depend on #38558 to solve unable to get view width, height problem |
|
@xxrlzzz Is this PR still relevant? As I understand it, the PR on which this one depends was merged. |
ok, I will continue this PR ASAP. |
5ba54ff to
faa408e
Compare
|
without with |
javache
left a comment
There was a problem hiding this comment.
Nice work! Sorry for the delay on reviewing this, we should try to get this imported.
There was a problem hiding this comment.
I think we should account for this by doing a transform as a first step and then the reverse transform as the last step, similar to processTransform
fe24a84 to
74cc237
Compare
|
While the implementation seems right here, I'm a bit concerned about adopting this given the Android SDK docs
There's also significant complexity being introduced here in terms of what code paths can potentially triggered. Adding a different transform property to the array can significantly change how the transform is set. |
Summary: Fixes #27649. On Android, `skewX` / `skewY` transforms are silently dropped during view-prop application: the matrix math layer correctly extracts the shear into `MatrixDecompositionContext.skew[]`, but `BaseViewManager.setTransformProperty` reads only `translation`, `rotationDegrees`, `scale`, and `perspective` from the decomposition context and never consumes the `skew[]` field. Views with `skew*` end up rendered as rotated-and-scaled rectangles instead of true parallelograms. This PR adds a single dispatch in `BaseViewManager.setTransformProperty`: when the transform array contains `skewX` / `skewY` and is otherwise 2D-affine, build a `Matrix` directly from the operations and apply it via `View.setAnimationMatrix` on Android Q+. All other transform shapes (`rotateX`, `rotateY`, `perspective`, raw 4x4 `matrix`, `translateZ`) continue to flow through the existing decompose-and-set-View-props code unchanged. ### Root cause `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java` (the pre-fix range `~573-635`) iterates the decomposed `translation`, `rotationDegrees`, `scale`, and `perspective` fields onto the View. The `skew[]` field on `MatrixDecompositionContext`, computed correctly by `MatrixMathHelper.decomposeMatrix`, is never read. Android `View` exposes property setters for translation, rotation around pivot, scale, and camera distance, but no `setSkewX` / `setSkewY`, so there has historically been no application path for the residual shear. quantizor's trace in #27649 (comment) identified the exact site. ### Fix A new internal Kotlin helper `SkewMatrixHelper` (in `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/SkewMatrixHelper.kt`) exposes two `JvmStatic` functions: - `isAffine2DTransformWithSkew(transforms)`, linear scan; returns true iff the array contains at least one `skewX` / `skewY` AND no operation that can't be represented by a 2D Skia `Matrix` (rejects `matrix`, `perspective`, `rotateX`, `rotateY`, `translate` with a non-zero Z component, and the raw 16-element matrix shorthand used by Fabric LayoutAnimations). - `buildAffine2DMatrix(transforms, viewWidthDip, viewHeightDip, transformOrigin)`, walks the array left-to-right and applies each operation to a `Matrix` via `preRotate` / `preScale` / `preSkew` / `preTranslate` around the resolved pivot. Composition is pre-multiplication so the rightmost array entry is applied first to the point, matching CSS / iOS conventions and `MatrixMathHelper.multiplyInto` in `TransformHelper.processTransform`. Reuses `TransformHelper.convertToRadians` / `TransformHelper.parseTranslateValue` so degree/radian/percent parsing stays in one place. `BaseViewManager.setTransformProperty` adds a guarded dispatch immediately after the `transforms == null` reset block: ```java if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q && SkewMatrixHelper.isAffine2DTransformWithSkew(transforms)) { Matrix affine = SkewMatrixHelper.buildAffine2DMatrix( transforms, PixelUtil.toDIPFromPixel(view.getWidth()), PixelUtil.toDIPFromPixel(view.getHeight()), transformOrigin); view.setTranslationX(0); view.setTranslationY(0); view.setRotation(0); view.setRotationX(0); view.setRotationY(0); view.setScaleX(1); view.setScaleY(1); view.setCameraDistance(0); view.setAnimationMatrix(affine); view.setTag(R.id.skew_animation_matrix, affine); return; } ``` The `R.id.skew_animation_matrix` tag (declared in `ids.xml`) holds the affine `Matrix` itself. A small `clearSkewAnimationMatrixIfActive(view)` helper is invoked from the `transforms == null` branch and from the existing decompose-path tail, gating the `view.setAnimationMatrix(null)` call on the tag. Without this gate, every animated rotate / scale / translate frame on every View would fire `setAnimationMatrix(null)`, which unconditionally invalidates the RenderNode and would be a per-frame regression for non-skew animations. `View.getMatrix()` does not compose `mAnimationMatrix` into its return value, so the React-side hit-test traversal in `TouchTargetHelper.kt` would otherwise still see the original rectangular bounds. To close that gap, the `R.id.skew_animation_matrix` view tag stores the affine `Matrix` itself (rather than `Boolean.TRUE`), and `TouchTargetHelper.getChildPoint` checks the tag and uses it as the inverse-mapping matrix when present. Net effect: hit testing follows the rendered parallelogram on both platforms, matching iOS / `CATransform3D` behavior. Invalidation, layer caching, and accessibility-bounds reporting come for free from the existing `setAnimationMatrix` plumbing. ### Pre-Q Gated to API 29+ to mirror the existing `view.setAnimationMatrix(null)` cleanup at `BaseViewManager.java:118`. On API 24-28 (small and declining install share in 2026), skew continues to be silently dropped, matching today's behavior. AndroidX Transitions uses `setAnimationMatrix` via reflection on pre-Q; if that ever becomes a priority for skew on pre-Q, the same shim could be added here without touching the dispatch shape. ### Why not the prior attempts - **#28862 (`wcandillon`, May 2020), fixed a JS-side decomposition bug where `skew[0]` was zeroed by a duplicate orthogonalization. That fix landed via Phabricator (commit `797367c0890a38ec51cfaf7bd90b9cc7db0e97c7`) and is preserved in current `main`. It corrected `decomposeMatrix` but did not address the application layer; this PR is the missing application path. - **#38494 (`xxrlzzz`, Jul 2023, closed Apr 2024), closest in spirit to this fix. It built a Skia `Matrix` and applied via `setAnimationMatrix` for a broader class of 2D transforms, plus a reflection shim for pre-Q. javache's review raised two concerns: (1) the SDK doc framing of `setAnimationMatrix` as an animation API, and (2) "two divergent code paths" complexity. This PR addresses both: (1) AndroidX `androidx.transition.ViewUtilsApi21` invokes the same API for static transforms in production today, and the API was promoted to public in API 29; the precedent is established. (2) The dispatch is tightened to `isAffine2DTransformWithSkew`, so the new path runs only for transforms that are broken under the existing path; the rotate / scale / translate / rotateX / rotateY / perspective code stays bit-identical. The reflection shim is intentionally not adopted; pre-Q skew remains dropped. - **#42676 (`piaskowyk` + `bartlomiejbloniarz`, Jan 2024, closed Aug 2025), simulated skew via 3D rotation + non-uniform scale + perspective hack. The 2x2 sub-matrix matches a true skew, but the 4x4 differs in the third row/column, so composition with real `rotateX` / `rotateY` produces wrong results. The PR author acknowledged this in the description. Not the right shape. ## Changelog: [ANDROID] [FIXED] - skewX / skewY transforms now render correctly on Android Q+. Pull Request resolved: #56724 Test Plan: ### Unit tests - `./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests 'com.facebook.react.uimanager.SkewMatrixHelperTest*'`, 17 / 17 pass (predicate cases for `hasSkewTransform` and `isAffine2DTransform`; matrix-math cases for `buildAffine2DMatrix` covering pure skewX, scale-then-translate ordering, view-center pivot default, and `transformOrigin` overrides via Number values and "%" strings). - `yarn jest packages/react-native/Libraries/StyleSheet/__tests__/processTransform-test.js`, 16 / 16 + 19 / 19 snapshots, unchanged. - `./gradlew ktfmtCheck`, `yarn lint`, `yarn flow`, all clean. ### Manual verification (Android, rn_test AVD: Pixel 8 Pro, Android 16, arm64-v8a) RNTester -> APIs -> Transforms -> "Skew (#27649)" (the new permanent example added in this PR). | Before | After | | --- | --- | | <img src="https://raw.githubusercontent.com/qflen/react-native/f4585e841aba90b69c11474dbfdc3e9540b8f0ee/assets/27649/before_android_skew_scene.png" width="260"> | <img src="https://raw.githubusercontent.com/qflen/react-native/f4585e841aba90b69c11474dbfdc3e9540b8f0ee/assets/27649/after_android_skew_scene.png" width="260"> | | Boxes render as rotated rectangles. Decompose extracts ~20 deg of rotation plus non-uniform scale; the residual shear is dropped. | Boxes render as true parallelograms. Top and bottom edges horizontal, sides tilted by 20 deg. | Hit testing follows the rendered parallelogram. Without commit 2, the rendering fix would land alone: parallelograms render but `TouchTargetHelper.getChildPoint` would still inverse-map through `child.matrix` (which doesn't compose `mAnimationMatrix`) and clip to the original rectangle. Verified empirically by sweeping tap coordinates 1 px on either side of every parallelogram edge on the skewX box (rect bounds `[116, 549] [300, 732]` on the rn_test AVD): - `(100, 555)`: inside parallelogram top-left tip, outside the rect. With only commit 1: misses. With commit 2 added: registers as `skewX 20deg`. - `(330, 731)`: bottom-right tip. Same flip. - `(208, 640)`: parallelogram / rect center. Registers either way. - `(350, 640)`: outside the parallelogram at vertical-pivot y. Misses either way (correct). The new `Skew (https://github.com/facebook/react-native/issues/27649)` example also includes a `useNativeDriver: true` `Animated.timing` interpolating skewX from `0deg` to `20deg`. Native-driven animations re-emit the transform array per frame via `TransformAnimatedNode.collectViewUpdates -> setTransformProperty`, so the dispatch runs each frame and the skew animates smoothly. ### iOS iOS already handles skewX / skewY correctly via `CATransform3D` (Paper: `RCTConvert+Transform.m` sets `next.m21 = tanf(skew)` for skewX and `next.m12 = tanf(skew)` for skewY; Fabric: `RCTViewComponentView.mm` calls `resolveTransform` -> `RCTCATransform3DFromTransformMatrix` -> `layer.transform`). This PR does not touch that path; the AFTER Android rendering above matches the existing iOS rendering. ### Negative case Existing transform examples (Translate-Rotate-Scale, Perspective-Rotate-Animation, Rotate-Scale, Transform-using-a-string, Transform-Matrix-2D / 3D) render bit-identically to `origin/main`. The `hasSkewTransform` predicate filters them out of the new path, so they go through the unchanged decompose-and-set-View-props code. The new `setAnimationMatrix(null)` clearing call on the fallthrough path is gated by the `R.id.skew_animation_matrix` view tag, so it fires only on the skew -> non-skew transition; non-skew animations have no per-frame regression. Reviewed By: christophpurrer Differential Revision: D106497417 Pulled By: javache fbshipit-source-id: 68f378a34c389a92fc7f161a31c7134cf6d8ee36
Summary:
This PR intent to solve Skew property on Android: #27649.
After android Q, we can use
setAnimationMatrixto implement cssTransform. (This api can also be accessed before android Q)Old method for implement
Transformis maintain a local 4*4 matrix, after finish handle all properties decompose back to a 3*3 matrix, and set back to android view use apisetTranslation,setRotation,setScale. There is no such a api to direct setskewproperty. But the native layer of android is using a 3*3 skia matrix to handle all those transform properties.So we could handle those
Transformon our side and callsetAnimationMatrixdirectly.There is also some shortage for this fix, a 3*3 matrix is unable to handle 3d rotation and perspective. We will fail back to old implementation when those property is used.
Note this pr may have a conflict with #37606.
Changelog:
[ANDROID] [Fixed] - Implement of css
Transformproperty.Test Plan:
RNTester App: Apis -> Animated -> Transform Styles.