Fix ctor/setProp prop-name gaps surfaced by iterator-setter audit#57330
Open
javache wants to merge 2 commits into
Open
Fix ctor/setProp prop-name gaps surfaced by iterator-setter audit#57330javache wants to merge 2 commits into
javache wants to merge 2 commits into
Conversation
Summary: RawPropsKey previously stored three `const char*` fields (prefix, name, suffix) that were concatenated at runtime to form property names. This is pretty niche, used to make a few patterns simpler, but also can lead to confusing conflicts when the same property name can be represented in different ways (e.g. T174300106). Iterator style props parsing also completely avoids it. Lets change the API to a flat name instead. This change is breaking, but could only find a single user (Nitro module) effected, searching through `react-native-libraries`. Changelog: [General][Breaking] - Remove RawPropsKey prefix and suffix Differential Revision: D94367880 Reviewed By: javache
Summary: Auditing every Props .cpp file in the renderer surfaced six cases where a prop key parsed by the 3-arg constructor's `convertRawProp(...)` call had no matching `case` in the same class's `setProp` method. With the iterator-setter path now routing every prop through `setProp` (instead of re-running the parsing constructor), each gap silently drops the prop on the floor when the iterator-setter path is active. Fix each gap: - `BaseScrollViewProps::setProp` — add `RAW_SET_PROP_SWITCH_CASE_BASIC(automaticallyAdjustKeyboardInsets)`. Parsed in ctor, missing case. - `BaseTextProps::setProp` — add `REBUILD_FIELD_SWITCH_CASE(... dynamicTypeRamp, "dynamicTypeRamp")`. Parsed in ctor, missing case. - `BaseTextProps::setProp` — rename the `baseWritingDirection` switch case key to `writingDirection`. JS sends `writingDirection` (the C++ field is named `baseWritingDirection` internally, but JS / Flow / TS / codegen all use `writingDirection` — verified across `StyleSheetTypes.js`, `StyleSheetTypes.d.ts`, `ReactNativeStyleAttributes.js`, `RCTTextInputViewConfig.js`, and the `Text-itest.js` integration test mounts `<rn-paragraph writingDirection="rtl">`). The setProp case was keyed off the C++ field name and never fired for any real JS prop. `appendTextAttributesProps` still emits `baseWritingDirection` for the C++→JS diff path — that's a separate, out-of-scope inconsistency. - `AccessibilityProps::setProp` — rename the `accessibilityOrder` switch case key to `experimental_accessibilityOrder`. JS sends the prefixed name (verified in `ViewPropTypes.js`, `BaseViewConfig.android.js`, `BaseViewConfig.ios.js`, `ViewProps.kt`'s `ACCESSIBILITY_ORDER = "experimental_accessibilityOrder"` constant, and `HostPlatformViewProps::getDebugProps` which serializes back out as `experimental_accessibilityOrder`). The unprefixed case never fired. - `BaseViewProps::setProp` — add `RAW_SET_PROP_SWITCH_CASE_BASIC(transformOrigin)`. Parsed in ctor, missing case. - `BaseViewProps::setProp` — add `SET_CASCADED_RECTANGLE_CORNERS(borderCurves, "border", "Curve", value)`. The ctor parses the full 13-key cascaded set (`borderCurve`, `borderTopLeftCurve`, … `borderStartStartCurve`) via `CascadedRectangleCornersNames`, but `setProp` had `SET_CASCADED_RECTANGLE_CORNERS` for `borderRadii` and `SET_CASCADED_RECTANGLE_EDGES` for `borderColors`/`borderStyles` — the corresponding `borderCurves` invocation was missing. Same gaps in the `third-party/react-native-macos/.../BaseViewProps.cpp` mirror — applied the same fixes there. Out-of-scope but flagged by the audit (left for follow-ups): - `propsConversions.h`'s `ViewEvents` converter doesn't handle `onGotPointerCapture` / `onLostPointerCapture` (explicit `// TODO` at line 541). `BaseViewProps::setProp` does. Asymmetry runs the other direction — iterator-setter handles these, classic ctor doesn't. - android `HostPlatformViewProps.cpp` defines `VIEW_EVENT_CASE` macro but never invokes it in its switch — dead code (events live on `BaseViewProps::events`). Cleanup candidate. Changelog: [General][Fixed] - Several view, text, scrollview, and accessibility props that the iterator-setter path silently dropped now propagate correctly through `setProp`: `automaticallyAdjustKeyboardInsets`, `dynamicTypeRamp`, `writingDirection`, `experimental_accessibilityOrder`, `transformOrigin`, and the full `borderCurves` cascaded set. Differential Revision: D109584760
|
@javache has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109584760. |
javache
added a commit
to javache/react-native
that referenced
this pull request
Jun 25, 2026
…act#57330) Summary: Auditing every Props .cpp file in the renderer surfaced six cases where a prop key parsed by the 3-arg constructor's `convertRawProp(...)` call had no matching `case` in the same class's `setProp` method. With the iterator-setter path now routing every prop through `setProp` (instead of re-running the parsing constructor), each gap silently drops the prop on the floor when the iterator-setter path is active. Fix each gap: - `BaseScrollViewProps::setProp` — add `RAW_SET_PROP_SWITCH_CASE_BASIC(automaticallyAdjustKeyboardInsets)`. Parsed in ctor, missing case. - `BaseTextProps::setProp` — add `REBUILD_FIELD_SWITCH_CASE(... dynamicTypeRamp, "dynamicTypeRamp")`. Parsed in ctor, missing case. - `BaseTextProps::setProp` — rename the `baseWritingDirection` switch case key to `writingDirection`. JS sends `writingDirection` (the C++ field is named `baseWritingDirection` internally, but JS / Flow / TS / codegen all use `writingDirection` — verified across `StyleSheetTypes.js`, `StyleSheetTypes.d.ts`, `ReactNativeStyleAttributes.js`, `RCTTextInputViewConfig.js`, and the `Text-itest.js` integration test mounts `<rn-paragraph writingDirection="rtl">`). The setProp case was keyed off the C++ field name and never fired for any real JS prop. `appendTextAttributesProps` still emits `baseWritingDirection` for the C++→JS diff path — that's a separate, out-of-scope inconsistency. - `AccessibilityProps::setProp` — rename the `accessibilityOrder` switch case key to `experimental_accessibilityOrder`. JS sends the prefixed name (verified in `ViewPropTypes.js`, `BaseViewConfig.android.js`, `BaseViewConfig.ios.js`, `ViewProps.kt`'s `ACCESSIBILITY_ORDER = "experimental_accessibilityOrder"` constant, and `HostPlatformViewProps::getDebugProps` which serializes back out as `experimental_accessibilityOrder`). The unprefixed case never fired. - `BaseViewProps::setProp` — add `RAW_SET_PROP_SWITCH_CASE_BASIC(transformOrigin)`. Parsed in ctor, missing case. - `BaseViewProps::setProp` — add `SET_CASCADED_RECTANGLE_CORNERS(borderCurves, "border", "Curve", value)`. The ctor parses the full 13-key cascaded set (`borderCurve`, `borderTopLeftCurve`, … `borderStartStartCurve`) via `CascadedRectangleCornersNames`, but `setProp` had `SET_CASCADED_RECTANGLE_CORNERS` for `borderRadii` and `SET_CASCADED_RECTANGLE_EDGES` for `borderColors`/`borderStyles` — the corresponding `borderCurves` invocation was missing. Same gaps in the `third-party/react-native-macos/.../BaseViewProps.cpp` mirror — applied the same fixes there. Out-of-scope but flagged by the audit (left for follow-ups): - `propsConversions.h`'s `ViewEvents` converter doesn't handle `onGotPointerCapture` / `onLostPointerCapture` (explicit `// TODO` at line 541). `BaseViewProps::setProp` does. Asymmetry runs the other direction — iterator-setter handles these, classic ctor doesn't. - android `HostPlatformViewProps.cpp` defines `VIEW_EVENT_CASE` macro but never invokes it in its switch — dead code (events live on `BaseViewProps::events`). Cleanup candidate. Changelog: [General][Fixed] - Several view, text, scrollview, and accessibility props that the iterator-setter path silently dropped now propagate correctly through `setProp`: `automaticallyAdjustKeyboardInsets`, `dynamicTypeRamp`, `writingDirection`, `experimental_accessibilityOrder`, `transformOrigin`, and the full `borderCurves` cascaded set. Differential Revision: D109584760
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
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:
Auditing every Props .cpp file in the renderer surfaced six cases where a prop key parsed by the 3-arg constructor's
convertRawProp(...)call had no matchingcasein the same class'ssetPropmethod. With the iterator-setter path now routing every prop throughsetProp(instead of re-running the parsing constructor), each gap silently drops the prop on the floor when the iterator-setter path is active.Fix each gap:
BaseScrollViewProps::setProp— addRAW_SET_PROP_SWITCH_CASE_BASIC(automaticallyAdjustKeyboardInsets). Parsed in ctor, missing case.BaseTextProps::setProp— addREBUILD_FIELD_SWITCH_CASE(... dynamicTypeRamp, "dynamicTypeRamp"). Parsed in ctor, missing case.BaseTextProps::setProp— rename thebaseWritingDirectionswitch case key towritingDirection. JS sendswritingDirection(the C++ field is namedbaseWritingDirectioninternally, but JS / Flow / TS / codegen all usewritingDirection— verified acrossStyleSheetTypes.js,StyleSheetTypes.d.ts,ReactNativeStyleAttributes.js,RCTTextInputViewConfig.js, and theText-itest.jsintegration test mounts<rn-paragraph writingDirection="rtl">). The setProp case was keyed off the C++ field name and never fired for any real JS prop.appendTextAttributesPropsstill emitsbaseWritingDirectionfor the C++→JS diff path — that's a separate, out-of-scope inconsistency.AccessibilityProps::setProp— rename theaccessibilityOrderswitch case key toexperimental_accessibilityOrder. JS sends the prefixed name (verified inViewPropTypes.js,BaseViewConfig.android.js,BaseViewConfig.ios.js,ViewProps.kt'sACCESSIBILITY_ORDER = "experimental_accessibilityOrder"constant, andHostPlatformViewProps::getDebugPropswhich serializes back out asexperimental_accessibilityOrder). The unprefixed case never fired.BaseViewProps::setProp— addRAW_SET_PROP_SWITCH_CASE_BASIC(transformOrigin). Parsed in ctor, missing case.BaseViewProps::setProp— addSET_CASCADED_RECTANGLE_CORNERS(borderCurves, "border", "Curve", value). The ctor parses the full 13-key cascaded set (borderCurve,borderTopLeftCurve, …borderStartStartCurve) viaCascadedRectangleCornersNames, butsetProphadSET_CASCADED_RECTANGLE_CORNERSforborderRadiiandSET_CASCADED_RECTANGLE_EDGESforborderColors/borderStyles— the correspondingborderCurvesinvocation was missing.Same gaps in the
third-party/react-native-macos/.../BaseViewProps.cppmirror — applied the same fixes there.Out-of-scope but flagged by the audit (left for follow-ups):
propsConversions.h'sViewEventsconverter doesn't handleonGotPointerCapture/onLostPointerCapture(explicit// TODOat line 541).BaseViewProps::setPropdoes. Asymmetry runs the other direction — iterator-setter handles these, classic ctor doesn't.HostPlatformViewProps.cppdefinesVIEW_EVENT_CASEmacro but never invokes it in its switch — dead code (events live onBaseViewProps::events). Cleanup candidate.Changelog:
[General][Fixed] - Several view, text, scrollview, and accessibility props that the iterator-setter path silently dropped now propagate correctly through
setProp:automaticallyAdjustKeyboardInsets,dynamicTypeRamp,writingDirection,experimental_accessibilityOrder,transformOrigin, and the fullborderCurvescascaded set.Differential Revision: D109584760