Migrate setNativeProps to state in OptionsSelector

Coming from #8503, enabling Fabric requires us to migrate off setNativeProps. We should move usages of setNativeProps to state in OptionsSelector

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@luacmartins

We had to revert the migration because a regression here - #11239

This is the only related change that caused it. And it is expected because we're using controlled selection (async setState).
@luacmartins I don't think there is much we can do to fix it.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 253 to 254 in 813b3d4

    
           selection={this.state.selection} 
        
           onSelectionChange={e => this.setState({selection: e.nativeEvent.selection})}

Exploring a fix

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

no updates here. couldn't make time

Update: I have no idea what was causing the regression and how to fix it #11239

Thoughts: The following reasons make me think that it's a purely performance related issue. To fix it, we'll have to make OptionsSelector more performant.

The ONLY code that's dealing with selection when typing text is -

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 253 to 254 in 813b3d4

    
           selection={this.state.selection} 
        
           onSelectionChange={e => this.setState({selection: e.nativeEvent.selection})}

^ It's the most basic form of controlled selection.

This issue is noticeable only on mobile devices.
OptionsSelector is an expensive component.

@luacmartins

@luacmartins @roryabraham I'm curious about your thoughts on #11053 (comment)

How should we proceed?

Get rid of controlled selection completely. And re-add it after Fabric has been enabled.
CONS:
a) we'll lose the feature that selects the search text when a option is selected - feature demo here
b) regression not being present on fabric is an assumption and we could be wrong.
Assign this issue to someone else
???

Can't promise anything, but I'm going try looking into this a bit

The following reasons make me think that it's a purely performance related issue.

I think this is correct. There's a noticeable delay between user input and state update, which was not present with setNativeProps since that updated the component directly. There's some discussion in this issue, but no conclusion yet. It seems like this performance regression is still experienced with Fabric enabled.

we'll lose the feature that selects the search text when a option is selected

I'm fine with this. FWIW we already have inconsistent behavior with the Split Bill page for example, where selecting a participant completely clears the input.

iou.mov

we'll lose the feature that selects the search text when a option is selected

Agree that this is an acceptable cost.

It seems like this performance regression is still experienced with Fabric enabled.

Agree, though it might be possible that this performance regression could be further alleviated by using startTransition instead of setState (only possible after Fabric upgrade)

@roryabraham

Can't promise anything, but I'm going try looking into this a bit

@roryabraham I know that you're looking into this, but it looks like we all agree that solution # 1 (get rid of controlled selection) is the way to go.

Please let me know if I should create a PR to replace controlled selection with clearing the input (as shown here #11053 (comment))

@rushatgabhane

@rushatgabhane yes, let's move on with getting rid of controlled selection.

I don't feel great about losing that feature, let's be sure to create a follow-up issue to try re-implementing after Fabric is enabled

I don't feel great about losing that feature, let's be sure to create a follow-up issue to try re-implementing after Fabric is enabled

Tracking issue #11679

Hey, just want to add to the discussion that TextInputs have an undocumented function called setSelection (I am not sure, but for web it might be named setSelectionRange), so you can call e.g.:

this.textInputRef.setSelection(start, end)

This way we can avoid having controlled inputs which are bad for performance + preserve the highlight functionality.

(note: sorry if I am writing out of context, was just quickly glimpsing and thought this information might adds value)

@hannojg

Thanks, that's very helpful @hannojg!

@luacmartins @roryabraham this.textInputRef.setSelection works perfectly on native.

However setSelectionRange doesn't work on web. It prevents me from selecting rows!!

Spent time on this, but I'm not sure what I'm not wrong.
Would you mind picking this task, I'm not able to make progress on making it work on web.

Screen.Recording.2022-10-11.at.9.17.36.PM.mov

Yes, i can give it a shot, although I am heavily focused on mobile, but will def try!

@rushatgabhane

Sorry @rushatgabhane but where is the PR where you started working on it? Would like to continue from there

@hannojg

@hannojg my bad, I had made a typo 🤦‍♂️😭

setSelection for native and setSelectionRange for web work great! 🎉
Thanks again for the help :)

Not sure why this isn't closed, but we can close it! #8503 (comment)

Navigation Menu

Repository navigation

Migrate setNativeProps to state in OptionsSelector #11053

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	selection={this.state.selection}
	onSelectionChange={e => this.setState({selection: e.nativeEvent.selection})}

Migrate setNativeProps to state in OptionsSelector #11053

Description

Activity

melvin-bot commented on Sep 23, 2022

rushatgabhane commented on Sep 26, 2022

rushatgabhane commented on Sep 26, 2022

melvin-bot commented on Sep 27, 2022

melvin-bot commented on Sep 27, 2022

rushatgabhane commented on Sep 30, 2022

rushatgabhane commented on Oct 6, 2022

rushatgabhane commented on Oct 6, 2022

roryabraham commented on Oct 6, 2022

luacmartins commented on Oct 6, 2022

roryabraham commented on Oct 6, 2022

rushatgabhane commented on Oct 7, 2022

luacmartins commented on Oct 7, 2022

roryabraham commented on Oct 7, 2022

rushatgabhane commented on Oct 7, 2022

hannojg commented on Oct 10, 2022

rushatgabhane commented on Oct 11, 2022

hannojg commented on Oct 11, 2022

hannojg commented on Oct 11, 2022

rushatgabhane commented on Oct 11, 2022

stitesExpensify commented on Oct 17, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions