feat: Enable default prop value in pseudoselectors#9631
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CSS pseudoselector pipeline so that an explicit undefined selector value (and a missing default) is forwarded through to native, allowing the C++ interpolator layer to interpret it as “use the platform/default prop value” during transitions.
Changes:
- Preserve
defaultentries for pseudoselector props even whendefaultis omitted (so missing defaults can be represented downstream). - After building native props, reinstate
undefinedpseudoselector/default values asnullso native can treat them as “default value”. - Extend unit tests and add an example screen section demonstrating “reset to default” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react-native-reanimated/src/css/utils/props.ts | Keeps per-selector defaultStyle[prop] even when the default is omitted (undefined). |
| packages/react-native-reanimated/src/css/native/managers/CSSPseudoStylesManager.ts | Reintroduces stripped undefined values post-build as null before constructing transition configs. |
| packages/react-native-reanimated/src/css/native/managers/tests/CSSPseudoStylesManager.test.ts | Adds/updates tests to assert undefined/missing-default is forwarded to native as null. |
| apps/common-app/src/apps/css/examples/transitions/screens/pseudoSelectors/Active.tsx | Adds demo cases showing reset-to-default behavior for undefined selector values and omitted defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MatiPl01
left a comment
MatiPl01
left a comment
There was a problem hiding this comment.
Left a few suggestions to implement
| selectorStyle: { backgroundColor: null }, | ||
| defaultStyle: { backgroundColor: 'red' }, | ||
| transition: { | ||
| backgroundColor: expect.objectContaining({ | ||
| value: ['red', null], | ||
| }), | ||
| }, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| test('forwards an omitted default to native as null so C++ can inject the default', () => { | ||
| pushStyle(manager, { | ||
| backgroundColor: { ':hover': 'red' }, | ||
| }); | ||
|
|
||
| expect(registerPseudoStyle).toHaveBeenCalledWith( | ||
| shadowNodeWrapper, | ||
| expect.objectContaining({ | ||
| selector: ':hover', | ||
| selectorStyle: { backgroundColor: 'red' }, | ||
| defaultStyle: { backgroundColor: null }, | ||
| transition: { | ||
| backgroundColor: expect.objectContaining({ | ||
| value: ['red', undefined], | ||
| value: [null, 'red'], |
There was a problem hiding this comment.
Shouldn't we better pass undefined for consistency with animation keyframes that can also contain undefined values? It should be automatically converted to null in the folly::dynamic then so the resulting behavior should be unchanged.
| }); | ||
| }); | ||
|
|
||
| describe('undefined / missing-default resolution (against a stripping builder)', () => { |
There was a problem hiding this comment.
I wonder if we need these stripping builder tests. I think that the pseudo selector processing logic is a bit unrelated to the builder and these tests don't add much value. What do you think? Can you see any value in keeping them?
| @@ -133,6 +146,22 @@ export default class CSSPseudoStylesManager implements ICSSPseudoStylesManager { | |||
| } | |||
| } | |||
|
|
|||
| /* Re-adds keys whose value was `undefined` in the source style (and thus dropped by the props builder) | |||
| * as `null`, which is interpreted on the C++ side as “use default value”. | |||
| * Only keys present in `supportedKeys` are revived. | |||
| */ | |||
| function reinstateResolvedUndefinedProps( | |||
| source: UnknownRecord, | |||
| built: UnknownRecord, | |||
| supportedKeys: Set<string> | |||
| ): void { | |||
| for (const key in source) { | |||
| if (source[key] === undefined && supportedKeys.has(key)) { | |||
| built[key] = null; | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I think that this entire complex logic might be replaced with just a proper build call on the props builder which is passed the includeUnprocessed: true option in its config. Since undefined values aren't processed, they are normally includeUnprocessed` to true changes this behavior and we include any value, even if it is unchanged during processing.
| const resets = Object.keys(selectorStyle) | ||
| .filter((prop) => selectorStyle[prop] === undefined) | ||
| .map((prop) => `${kebabizeCamelCase(prop)}: unset !important`) | ||
| .join('; '); |
There was a problem hiding this comment.
I don't like this filter/map combo. Can't we maybe extend the web builder in such a way that we replace undefined with initial? Maybe it could do this just when we pass includeUnprocessed: true?
Summary
Previously passing 'undefined' as value to a pseudoselector would result in dropping it - now it's pushed towards the c++ layer anyways, which interprets 'undefined' as 'default prop value'.
Test plan
There are tests for that under CSS -> Transitions -> Pseudoselectors -> Active