Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix attempting to focus disabled textinputs #30695

Closed
wants to merge 2 commits into from

Conversation

vonovak
Copy link
Contributor

@vonovak vonovak commented Jan 6, 2021

Summary

when we call focus() upon a TextInput ref which has prop editable=false it marks the textinput as focused in TextInputState even though the focus is rejected by textinput itself because it is not editable.

then, when you change editable prop to true and call focus again, this condition or rather this one will evaluate to false and focus will not happen even though it can and should happen.

see also https://github.com/facebook/react-native/blob/0.64-stable/Libraries/Renderer/implementations/ReactNativeRenderer-dev.js#L3895

Changelog

[General] [Fixed] - focus() on TextInput to respect its editable state

Test Plan

Create a TextInput with prop editable=false and call ref.current.focus() upon its ref. TextInput should not be marked as focused in TextInputState.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 6, 2021
@vonovak vonovak changed the title fix focus fix attempting to focus disabled textinputs Jan 6, 2021
return;
}
// $FlowFixMe - `focus` is missing in `$ReadOnly`
Object.getPrototypeOf(current)?.focus?.call(current);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@yungsters had some feedback:
I think a better fix would be for the platform to return a boolean — true if the focus is successful, and false otherwise.

But for now, this logic should live in TextInputState.focusTextInput.

function focusTextInput(textField: ?ComponentRef) {
  // …

  if (currentlyFocusedInputRef !== textField && textField != null) {
    // EXPLANATION…
    if (textField.props?.editable === false) {
      return;
    }

    // …
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunaleaps @yungsters the PR was updated, thanks for review

Copy link

Choose a reason for hiding this comment

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

thanks

@analysis-bot
Copy link

analysis-bot commented Jan 6, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 2fdbf6a
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 1, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,188,178 -16,868
android hermes armeabi-v7a 7,788,310 -24,930
android hermes x86 8,558,123 -18,666
android hermes x86_64 8,511,165 -16,057
android jsc arm64-v8a 9,857,502 -16,356
android jsc armeabi-v7a 8,842,165 -24,421
android jsc x86 9,823,547 -18,136
android jsc x86_64 10,420,724 -15,518

Base commit: 2fdbf6a
Branch: main

@facebook-github-bot
Copy link
Contributor

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

return;
}
// $FlowFixMe - `focus` is missing in `$ReadOnly`
Object.getPrototypeOf(current)?.focus?.call(current);
Copy link
Contributor

Choose a reason for hiding this comment

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

@yungsters had some feedback:
I think a better fix would be for the platform to return a boolean — true if the focus is successful, and false otherwise.

But for now, this logic should live in TextInputState.focusTextInput.

function focusTextInput(textField: ?ComponentRef) {
  // …

  if (currentlyFocusedInputRef !== textField && textField != null) {
    // EXPLANATION…
    if (textField.props?.editable === false) {
      return;
    }

    // …
  }
}

@vonovak vonovak requested a review from lunaleaps March 2, 2022 16:04
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @vonovak in 8a5460c.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 22, 2022
@vonovak vonovak deleted the textinput-focus branch March 23, 2022 09:11
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
when we call `focus()` upon a TextInput ref which has prop `editable=false` it marks the textinput as focused in `TextInputState` even though the focus is rejected by textinput itself because it is not editable.

then, when you change `editable` prop to `true` and call `focus` again, [this condition](https://github.com/facebook/react-native/blob/e912c462eb0b7166ca5947bb5a3ee20761d910b6/Libraries/Components/TextInput/TextInputState.js#L46) or rather [this one](https://github.com/facebook/react-native/blob/1b2b2198e1b2383523b4655dc8c220d251b057d6/Libraries/Components/TextInput/TextInputState.js#L89) will evaluate to `false` and focus will not happen even though it can and should happen.

see also https://github.com/facebook/react-native/blob/0.64-stable/Libraries/Renderer/implementations/ReactNativeRenderer-dev.js#L3895

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - `focus()` on TextInput to respect its `editable` state

Pull Request resolved: facebook#30695

Test Plan: Create a `TextInput` with prop `editable=false` and call `ref.current.focus()` upon its ref. TextInput should not be marked as focused in `TextInputState`.

Reviewed By: yungsters

Differential Revision: D34357913

Pulled By: lunaleaps

fbshipit-source-id: 9a2fb819bbb05ef213c9b5d739dec583ae0a3e6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants