Skip to content

Conversation

@acoates-ms
Copy link
Contributor

@acoates-ms acoates-ms commented Apr 26, 2021

Fixes #7638.

JS was not providing the mostRecentEventCount back to native. So in TextInputShadowNode::SetText, this condition would never be true, m_mostRecentEventCount == m_nativeEventCount, so any updated text value coming from JS would always be ignored.

Microsoft Reviewers: Open in CodeFlow

@acoates-ms acoates-ms requested a review from a team as a code owner April 26, 2021 21:05
@ghost ghost added the Area: TextInput label Apr 26, 2021
@NickGerleman
Copy link
Contributor

Wow this one is nasty...

Are we still actively investing in adding UI tests to RNTester examples?

@rectified95
Copy link
Contributor

@NickGerleman Yes, it's still on my to-do list.

@rectified95
Copy link
Contributor

@acoates-ms the m_mostRecentEventCount == m_nativeEventCount condition currently works for the live rewrite scenario in RNTester,

<TextInput
  onChangeText={text => {
    text = text.replace(/ /g, '_');
    this.setState({text});
  }}
  value={this.state.text}
/>

where m_mostRecentEventCount gets updated due to the setTextAndSelection command coming from JS. Can you check this still works after this change?

@acoates-ms
Copy link
Contributor Author

@rectified95 yes that example seems to work with these changes.

@acoates-ms acoates-ms added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 29, 2021
@ghost
Copy link

ghost commented Apr 29, 2021

Hello @acoates-ms!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@NickGerleman
Copy link
Contributor

@NickGerleman Yes, it's still on my to-do list.

I think test coverage will have to be a bit of a group effort. We just have too much untested code to do otherwise.

I think it would be valuable if we started adding coverage when fixing native control bugs. I think we're at the point where the framework should allow testing the sort of scenario this PR addresses without too much burden.

@NickGerleman
Copy link
Contributor

OTOH there's definitely been times in the past where I felt like adding too scope (adding tests) would make a fix take too long for me to take on.

Maybe we can do something to reduce that initial barrier.

@acoates-ms acoates-ms merged commit 1d8410d into microsoft:master May 3, 2021
acoates-ms added a commit to acoates-ms/react-native-windows that referenced this pull request May 3, 2021
* If initial value is set, updated TextInput.value changes will not be reflected

* Change files

* default member initializers for bit-fields requires at least '/std:c++latest'
ghost pushed a commit that referenced this pull request May 4, 2021
* If initial value is set, updated TextInput.value changes will not be reflected

* Change files

* default member initializers for bit-fields requires at least '/std:c++latest'
@acoates-ms acoates-ms deleted the textinput branch July 23, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: TextInput AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextInput does not match update until it receives focus

5 participants