Skip to content

iOS: Fix autoCorrect doesn't revert to initial state#11055

Closed
rigdern wants to merge 4 commits into
react:masterfrom
rigdern:rigdern/iosAutoCorrect
Closed

iOS: Fix autoCorrect doesn't revert to initial state#11055
rigdern wants to merge 4 commits into
react:masterfrom
rigdern:rigdern/iosAutoCorrect

Conversation

@rigdern

@rigdern rigdern commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

There's an inconsistency in autoCorrect's default state:

  • If you don't specify a value for autoCorrect, it defaults to on.
  • If you specify true/false for autoCorrect and later specify null, autoCorrect turns off. It should have reverted to its initial state of on.

The reason for this discrepancy is that autoCorrect is exposed to JS as a boolean but it is actually an enum with three states in native:

  • UITextAutocorrectionTypeDefault (the default value)
  • UITextAutocorrectionTypeYes
  • UITextAutocorrectionTypeNo

This is fixed by explicitly mapping JS null to UITextAutocorrectionTypeDefault.

Test plan (required)

Verified that switching autoCorrect between true, false, and null all work correctly in single line and multiline TextInputs.

Adam Comella
Microsoft Corp.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

By analyzing the blame information on this pull request, we identified @javache and @janicduplessis to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 22, 2016
Comment thread Libraries/Text/RCTTextFieldManager.m Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: use correct type here UITextAutocorrectionType

Instead of repeating the logic here, extract this logic to an RCTConvert category.

Adam Comella added 2 commits November 22, 2016 20:47
There's an inconsistency in autoCorrect's default state:
  - If you don't specify a value for autoCorrect, it defaults to on.
  - If you specify true/false for autoCorrect and later specify null, autoCorrect turns off.

The reason for this discrepancy is that autoCorrect is exposed to JS as a boolean but it is actually an enum with three states in native:
  - UITextAutocorrectionTypeDefault (the default value)
  - UITextAutocorrectionTypeYes
  - UITextAutocorrectionTypeNo

This is fixed by explicitly mapping JS null to UITextAutocorrectionTypeDefault.
@rigdern
rigdern force-pushed the rigdern/iosAutoCorrect branch from e8a8238 to e8b53c2 Compare November 23, 2016 05:28
@rigdern

rigdern commented Nov 23, 2016

Copy link
Copy Markdown
Contributor Author

@javache Thanks for the feedback. I pushed a change to address your feedback and rebased my branch on master.

In my original change, I didn't implement an RCTConvert method for UITextAutocorrectionType because I figured my implementation deviated from the way somebody would expect the converter to work. Since it's an enum, the expected implementation would be something like:

RCT_ENUM_CONVERTER(UITextAutocorrectionType, (@{
  @"no": @(UITextAutocorrectionTypeNo),
  @"yes": @(UITextAutocorrectionTypeYes),
  @"default": @(UITextAutocorrectionDefault),
}), UITextAutocorrectionTypeDefault, integerValue)

Whereas my actual implementation which converts JS booleans to native enum values is a little unusual.

By the way, what's the reasoning for RCTConvertEnumValue special casing numbers? When json is a number, it appears to return it and ignore the value that particular key was supposed to map to. That seems surprising.

@javache

javache commented Nov 23, 2016

Copy link
Copy Markdown
Contributor

I'm not sure if the special casing for numbers is actually used. It allows you to just use the integers the enum maps to on the JS side to avoid passing over strings.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@rigdern
rigdern deleted the rigdern/iosAutoCorrect branch November 23, 2016 21:22
rigdern pushed a commit to rigdern/react-native that referenced this pull request Nov 23, 2016
This exposes iOS's spellCheckingType functionality to JavaScript. The native functionality is a three state enum. It gets exposed to JavaScript as a boolean. The initial value and JS null map to the third state.

An alternative design for this API would have been to expose a three state enum to JavaScript:
  - "on" which maps to UITextSpellCheckingTypeYes
  - "off" which maps to UITextSpellCheckingTypeNo
  - "auto" (default) which maps to UITextSpellCheckingTypeDefault

For consistency, I decided to use the same API design as spellCheck. We don't have many options for fixing spellCheck in react#11055 without introducing a breaking change.
facebook-github-bot pushed a commit that referenced this pull request Nov 25, 2016
Summary:
This exposes iOS's spellCheckingType functionality to JavaScript. The native functionality is a three state enum. It gets exposed to JavaScript as a boolean. The initial value and JS null map to the third state.

An alternative design for this API would have been to expose a three state enum to JavaScript:
  - "on" which maps to UITextSpellCheckingTypeYes
  - "off" which maps to UITextSpellCheckingTypeNo
  - "auto" (default) which maps to UITextSpellCheckingTypeDefault

For consistency, I decided to use the same API design as spellCheck. We don't have many options for fixing spellCheck in #11055 without introducing a breaking change.

**Test plan (required)**

Verified that switching `spellCheck` between `true`, `false`, and `null` all work correctly in single line and multiline `TextInputs`.
Closes #11056

Differential Revision: D4232802

Pulled By: javache

fbshipit-source-id: 79e03307fa6a30a169f7e2fd0ec5ac826663e7c1
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants