Skip to content

Conversation

@iamAbhi-916
Copy link
Contributor

@iamAbhi-916 iamAbhi-916 commented May 8, 2025

Summary:

for IOS and React native windows we can observe that the macro conversion is incorrect in ParagraphProps particularly for selectable prop.

current conversion

case ([]() constexpr -> RawPropsPropNameHash {   return facebook::react::fnv1a("isSelectable");  }()): fromRawValue(context, value, isSelectable, defaults.isSelectable); return;

issue is that isSelectable is not the raw prop therefore JS to native flow for the prop is not correct .
(Note : this works for Android as @ReactProp(name = "selectable"):

@ReactProp(name = "selectable")
public fun setSelectable(view: ReactTextView, isSelectable: Boolean) {
view.setTextIsSelectable(isSelectable)
}
)

after fix

case ([]() constexpr -> RawPropsPropNameHash {   return facebook::react::fnv1a("selectable");  }()): fromRawValue(context, value, selectable, defaults.selectable); return;

Current implementation selectable prop is not working for IOS and React native windows as the macro conversion is incorrect in ParagraphProps particularly for selectable prop.

Changelog:

Updated ParagraphProps declaration .h/.cpp file for selectable raw prop and then refactoring their respective implementations
all places required .

Test Plan:

Tested on react native windows playground

Sample code

export default class Bootstrap extends React.Component {
  render() {
    return (
      <View style={styles.container}>
        <Text style={styles.header}>Selectable vs Non-Selectable Text</Text>

        <Text selectable={true} style={styles.text}>
          ✅ This text is selectable. You can long-press and copy it.
        </Text>

        <Text selectable={false} style={styles.text}>
          ❌ This text is not selectable. You cannot copy it.
        </Text>
      </View>
    );
  }
}


before fix debug output , native unaware of selectable prop values from JS

ReactNative ['Samples\text'] (info): ''[Text.js] NativeText _selectable:', true'
ReactNative ['Samples\text'] (info): ''[Text.js] NativeText _selectable:', false'
[ParagraphComponentView] updateProps - old isSelectable: 0, new isSelectable: 0
[ParagraphComponentView] DrawText - isSelectable: 0
[ParagraphComponentView] DrawText - selection logic would be DISABLED here.
[ParagraphComponentView] updateProps - old isSelectable: 0, new isSelectable: 0
[ParagraphComponentView] DrawText - isSelectable: 0
[ParagraphComponentView] DrawText - selection logic would be DISABLED here.
[ParagraphComponentView] updateProps - old isSelectable: 0, new isSelectable: 0
[ParagraphComponentView] DrawText - isSelectable: 0
[ParagraphComponentView] DrawText - selection logic would be DISABLED here.

after fix debug output , native picks up selectable prop values from JS correctly

ReactNative ['Samples\text'] (info): ''[Text.js] NativeText _selectable:', true'
ReactNative ['Samples\text'] (info): ''[Text.js] NativeText _selectable:', false'
[ParagraphComponentView] updateProps - old selectable: 0, new selectable: 0
[ParagraphComponentView] DrawText - selectable: 0
[ParagraphComponentView] DrawText - selection logic would be DISABLED here.
[ParagraphComponentView] updateProps - old selectable: 0, new selectable: 1
[ParagraphComponentView] DrawText - selectable: 1
[ParagraphComponentView] DrawText - selection logic would be enabled here.
[ParagraphComponentView] updateProps - old selectable: 0, new selectable: 0
[ParagraphComponentView] DrawText - selectable: 0
[ParagraphComponentView] DrawText - selection logic would be DISABLED here.

@facebook-github-bot 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. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels May 8, 2025
@facebook-github-bot
Copy link
Contributor

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

@iamAbhi-916
Copy link
Contributor Author

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

Hey @philIip , just wanted to get some feedback for this PR also do we need to create an issue for the same?


switch (hash) {
RAW_SET_PROP_SWITCH_CASE_BASIC(isSelectable);
RAW_SET_PROP_SWITCH_CASE_BASIC(selectable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we instead use: RAW_SET_PROP_SWITCH_CASE(isSelectable, selectable) and not change anything alse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we can do that , makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cipolleschi closing this pr as this is to old , created a new one for the same .
#52599

@cipolleschi
Copy link
Contributor

Hi there!
The problem with this PR is that it is a breaking change. We run a raw search on GH and there are 178 results that depends on it. most of them are react native forks, but wix/react-navigation and datadog will break if we land and ship this diff.

One option is to try and make it backward=compatible, so we don't break other libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants