fix(android): delete enableAccumulatedUpdatesInRawPropsAndroid and make accumulated rawProps the default#56588
Draft
janicduplessis wants to merge 6 commits intofacebook:mainfrom
Conversation
…groundColor transition Summary: On Android, a View whose backgroundColor starts as transparent and later transitions to an opaque color loses its border-radius clipping and renders as a rectangle. Reported in facebook#52415; iOS is unaffected. Root cause: `setBorderRadius` only eagerly creates an inner `BackgroundDrawable` for `ImageView`. For a regular `View` with a transparent backgroundColor, the composite background drawable ends up with no inner `BackgroundDrawable` at first. When the color later becomes opaque, `setBackgroundColor` -> `ensureBackgroundDrawable` constructs a fresh `BackgroundDrawable` and swaps `view.background` with a new `CompositeBackgroundDrawable` via `withNewBackground`. The freshly constructed drawable starts with 0x0 bounds, and on the first draw the computed path is a zero-radius rectangle. Fix: always eagerly create the inner `BackgroundDrawable` inside `setBorderRadius`, not only for `ImageView`. This way subsequent `setBackgroundColor` calls mutate the existing drawable in place, avoiding the `view.background` replacement path where bounds/path are not yet primed. Changelog: [ANDROID] [FIXED] - View with borderRadius renders as rectangle after transparent → opaque backgroundColor transition Test Plan: Added an RNTester example under Border > "Border radius with transparent → opaque background" that starts with a transparent 50x50 box with borderRadius: 25 and toggles to red on tap. Without this change the opaque state renders as a square on Android; with the fix it renders as a circle. iOS rendering is unchanged.
meta-cla
Bot
added
the
CLA Signed
Revert the eager ensureBackgroundDrawable call inside setBorderRadius so views with borderRadius but no backgroundColor don't carry an always-allocated drawable. Instead, fix the root timing issue at the actual site of the bug: when ensureBackgroundDrawable lazily constructs a new BackgroundDrawable + new composite, carry the existing composite's bounds over before assigning view.background. This primes the new drawable with the view's real dimensions so its first draw computes the border-radius path correctly, without imposing cost on views that never hit this path. Test Plan: same RNTester repro (UI > Border > "Border radius with transparent -> opaque background").
… full props
Drop the earlier BackgroundStyleApplicator workaround — it targeted the
wrong layer. Fix the actual root cause in Props::initializeDynamicProps.
When a View with borderRadius + transparent backgroundColor mounts, Fabric
flattens it and no Android View is created. On a later JS update such as
backgroundColor -> opaque, the Differentiator sees the node un-flatten and
emits a CREATE mutation for a brand-new native View. FabricMountingManager
ships the props for that CREATE as newProps->rawProps.
The problem: under the default (non-accumulated) path, Props::initialize
overwrote rawProps with rawProps.toDynamic(filterObjectKeys) on every
clone — storing only the latest JS diff, not the full accumulated state.
So at un-flatten time, rawProps for the newly-concrete shadow node held
only {backgroundColor: <color>} and the Android View received only that
prop. borderRadius, borderWidth, etc. were never delivered, so the view
rendered without them.
The fix merges the previous rawProps with the incoming patch on every
clone, so rawProps always reflects the full accumulated prop set. The
existing getProps logic then ships the complete set on CREATE and the
newly-created View gets every prop it should have.
This was previously gated behind enableAccumulatedUpdatesInRawPropsAndroid.
Always running it costs an extra folly::dynamic merge per clone but fixes
a real correctness bug that could affect any view-flattening-prone
component with more than one style prop.
janicduplessis
changed the title
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
…ke accumulated rawProps the default Supersedes the earlier Props.cpp patch approach. The flag's expectedReleaseValue was already true and the non-accumulated code path has a latent correctness bug on view-flattening un-flatten (shadow node's rawProps gets overwritten with the latest JS patch only, so the CREATE mount item ships a partial prop set and newly- created native Views lose props like borderRadius). See facebook#52415. This removes the flag entirely and promotes the accumulated behavior to be the only behavior: - Props::initializeDynamicProps unconditionally merges previous rawProps with the incoming patch. - FabricMountingManager::getProps: non-Props-2.0 path now always emits full rawProps for CREATE and diffDynamicProps() for UPDATE. - FabricMountingManager executeMount INSERT: always emits UpdatePropsMountItem (dropped the shouldCreateView gate that only ran under the old flag-off branch). - FabricUIManagerBinding: schedulerDidFinishTransaction is a no-op (transactions are pulled only in schedulerShouldRenderTransactions); the pendingTransactions_ queue and its mutex are deleted with it. - ConcreteComponentDescriptor: enableExclusivePropsUpdateAndroid no longer pivots on the accumulated flag. - Regenerated all feature-flag accessor files (Kotlin + C++) via `yarn featureflags --update` to drop the API surface.
janicduplessis
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
On Android, a View with
borderRadius+backgroundColor: 'transparent'gets view-flattened on initial mount (no AndroidViewis created for it). When JS later changesbackgroundColorto an opaque color, Fabric's Differentiator un-flattens the node and emits a CREATE mutation so Android creates a nativeView. ThatViewreceives only the prop that changed (backgroundColor);borderRadius,borderWidth, etc. are never delivered, so the view renders without them. iOS is unaffected.Reproduces #52415.
Root cause
The shadow node's
rawProps(consumed byFabricMountingManager::getPropson the non-Props-2.0 path) is overwritten with only the incoming JS patch on every clone, rather than merged with the previous state:After a JS
{backgroundColor: red}commit, the cloned shadow node'srawPropsis{backgroundColor: red}— missing every prop from the initial mount. When a later un-flatten emits a CREATE for that node,getPropshands Java this partial map; the ViewManager only invokessetBackgroundColoron the fresh native View;setBorderRadiusis never called; theBackgroundDrawableends up with a nullborderRadiusand draws a rectangle.Confirmed on an Android emulator (API 35, new architecture) via adb logcat — instrumented
getProps+BackgroundDrawable.drawshowed the CREATE's prop map contained onlybackgroundColorand the draw took thedrawRectbranch withcomputedBR=null.Fix
enableAccumulatedUpdatesInRawPropsAndroidwas already markedexpectedReleaseValue: trueinReactNativeFeatureFlags.config.js, and its flag-on path is the correct one. This PR deletes the flag entirely and promotes the accumulated behavior to be the only behavior:Props::initializeDynamicPropsunconditionally merges previous rawProps with the incoming patch.FabricMountingManager::getProps(non-Props-2.0 path) always emits fullrawPropsfor CREATE anddiffDynamicProps()for UPDATE.FabricMountingManager::executeMountINSERT case: always emitsUpdatePropsMountItem(the old flag-off branch only did so when the view was newly allocated).FabricUIManagerBinding:schedulerDidFinishTransactionis now a no-op (transactions are pulled exclusively inschedulerShouldRenderTransactions). The now-unusedpendingTransactions_queue and its mutex are removed from the header.ConcreteComponentDescriptor: theenableExclusivePropsUpdateAndroidskip no longer pivots on the accumulated flag.yarn featureflags --updateto drop the JNI / Kotlin / JS API surface.Interaction with the other Props 2.0 flags
This PR touches only the removed flag. The other two flags keep their current defaults and behaviors:
enablePropsUpdateReconciliationAndroid(typed-diff Props 2.0 path ingetProps) — unchanged.enableExclusivePropsUpdateAndroid(skip Props 1.5 when Props 2.0 handles the component) — unchanged.Changelog:
[ANDROID] [REMOVED] - Removed
enableAccumulatedUpdatesInRawPropsAndroidfeature flag; its behavior is now the default.[ANDROID] [FIXED] - View with
borderRadiusloses border radius afterbackgroundColortransitions from transparent to opaque.Test Plan:
Added an RNTester example under UI > Border > "Border radius with transparent → opaque background". A 50×50 box with
borderRadius: 25mounts transparent and toggles to red on tap.Before: tapping turns the box red as a square.
After: tapping turns the box red as a circle.
Verified on an Android emulator (API 35, new architecture) both manually and via agent-device + logcat instrumentation. Also covers the reproducer in #52415.