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

Make PlainInput controllable #13795

Merged
merged 17 commits into from Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions shared/common-adapters/hoc-timers.js
Expand Up @@ -78,6 +78,10 @@ function HOCTimers<Props: TimerProps>(
}
}

// TODO forward a ref to `WrappedComponent` when react-redux is patched to
// work with React.forwardRef.
// https://github.com/reduxjs/react-redux/pull/1000

return TimersComponent
}

Expand Down
13 changes: 11 additions & 2 deletions shared/common-adapters/new-input.js
Expand Up @@ -17,6 +17,7 @@ export type _Props = {
containerStyle?: StylesCrossPlatform,
decoration?: React.Node,
error?: boolean,
forwardedRef: React.Ref<typeof PlainInput>,
hideBorder?: boolean,
icon?: IconType,
}
Expand All @@ -32,7 +33,7 @@ type State = {
focused: boolean,
}

class NewInput extends React.Component<DefaultProps & Props, State> {
class ReflessNewInput extends React.Component<DefaultProps & Props, State> {
static defaultProps = {
flexable: true,
keyboardType: 'default',
Expand All @@ -54,6 +55,7 @@ class NewInput extends React.Component<DefaultProps & Props, State> {

render() {
const textStyle = getTextStyle(this.props.textType)
const {containerStyle, decoration, error, forwardedRef, hideBorder, icon, ...plainInputProps} = this.props
return (
<Box2
direction="horizontal"
Expand All @@ -75,12 +77,19 @@ class NewInput extends React.Component<DefaultProps & Props, State> {
/>
</Box>
)}
<PlainInput {...this.props} onFocus={this._onFocus} onBlur={this._onBlur} />
<PlainInput
{...plainInputProps}
onFocus={this._onFocus}
onBlur={this._onBlur}
ref={this.props.forwardedRef}
/>
{this.props.decoration}
</Box2>
)
}
}
// $FlowIssue doesn't know about forwardRef (https://github.com/facebook/flow/issues/6103)
const NewInput = React.forwardRef((props, ref) => <ReflessNewInput {...props} forwardedRef={ref} />)

const styles = styleSheetCreate({
container: platformStyles({
Expand Down
38 changes: 34 additions & 4 deletions shared/common-adapters/plain-input.desktop.js
Expand Up @@ -2,9 +2,11 @@
import * as React from 'react'
import {getStyle as getTextStyle} from './text.desktop'
import {collapseStyles, globalColors, styleSheetCreate, platformStyles} from '../styles'
import {pick} from 'lodash-es'
import logger from '../logger'

import type {_StylesDesktop} from '../styles/css'
import type {InternalProps, TextInfo} from './plain-input'
import type {InternalProps, TextInfo, Selection} from './plain-input'
import {checkTextInfo} from './input.shared'

// A plain text input component. Handles callbacks, text styling, and auto resizing but
Expand Down Expand Up @@ -73,6 +75,13 @@ class PlainInput extends React.PureComponent<InternalProps> {
}

transformText = (fn: TextInfo => TextInfo, reflectChange?: boolean) => {
const controlled = !!this.props.value
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you check if it's null or undefined instead? since empty string can be a valid value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, thanks!

if (controlled) {
const errMsg =
'Attempted to use transformText on controlled input component. Use props.value and setSelection instead.'
logger.error(errMsg)
throw new Error(errMsg)
}
const n = this._input
if (n) {
const textInfo: TextInfo = {
Expand All @@ -96,6 +105,29 @@ class PlainInput extends React.PureComponent<InternalProps> {
}
}

getSelection = () => {
const n = this._input
if (n) {
return {start: n.selectionStart, end: n.selectionEnd}
}
return null
}

setSelection = (s: Selection) => {
const controlled = !!this.props.value
if (!controlled) {
const errMsg =
'Attempted to use setSelection on uncontrolled input component. Use transformText instead'
logger.error(errMsg)
throw new Error(errMsg)
}
const n = this._input
if (n) {
n.selectionStart = s.start
n.selectionEnd = s.end
}
}

_onCompositionStart = () => {
this._isComposingIME = true
}
Expand Down Expand Up @@ -129,6 +161,7 @@ class PlainInput extends React.PureComponent<InternalProps> {

_getCommonProps = () => {
let commonProps: any = {
...pick(this.props, ['maxLength', 'value']), // Props we should only passthrough if supplied
autoFocus: this.props.autoFocus,
className: this.props.className,
onBlur: this._onBlur,
Expand All @@ -145,9 +178,6 @@ class PlainInput extends React.PureComponent<InternalProps> {
if (this.props.disabled) {
commonProps.readOnly = 'readonly'
}
if (this.props.maxLength) {
commonProps.maxlength = this.props.maxLength
}
return commonProps
}

Expand Down
27 changes: 21 additions & 6 deletions shared/common-adapters/plain-input.js.flow
Expand Up @@ -20,7 +20,7 @@ export type KeyboardType =
// Android Only
| 'visible-password'

export type Props = {
export type Props = {|
autoFocus?: boolean,
className?: string,
disabled?: boolean,
Expand All @@ -38,6 +38,7 @@ export type Props = {
style?: StylesCrossPlatform,
textType?: TextType,
type?: 'password' | 'text' | 'number',
value?: string, // Makes this a controlled input when passed. Also disables mutating value via `transformText`, see note at component API

/* Platform discrepancies */
// Maps to onSubmitEditing on native
Expand All @@ -55,7 +56,7 @@ export type Props = {
returnKeyType?: 'done' | 'go' | 'next' | 'search' | 'send',
selectTextOnFocus?: boolean,
onEndEditing?: () => void,
}
|}

// Use this to mix your props with input props like type Props = PropsWithInput<{foo: number}>
export type PropsWithInput<P> = {|
Expand All @@ -73,10 +74,10 @@ export type PropsWithInput<P> = {|
* use `InternalProps`.
* See more discussion here: https://github.com/facebook/flow/issues/1660
*/
export type DefaultProps = {
export type DefaultProps = {|
keyboardType: KeyboardType,
textType: TextType,
}
|}

export type Selection = {start: number, end: number}

Expand All @@ -85,11 +86,25 @@ export type TextInfo = {
selection: Selection,
}

export type InternalProps = DefaultProps & Props
export type InternalProps = {...DefaultProps, ...Props}
declare export default class PlainInput extends React.Component<Props> {
static defaultProps: DefaultProps;
blur: () => void;
focus: () => void;
// Supported only on desktop right now
getSelection: () => ?Selection;
/**
* This can only be used when the input is controlled. Use `transformText` if
* you want to do this on an uncontrolled input. Make sure the Selection is
* valid against the `value` prop. Avoid changing `value` and calling this at
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...so now I'm confused by this API. We almost always want to change both the text and the selection at the same time...AIUI the existing use cases are inserting text (like emoji, mentions, etc.) at the cursor. So that's why I suggested having a selection prop along with the value. Did that not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into issues early on with the parent actively managing the selection and value at the same time. AFAICT on desktop there's no way to listen to events that change the cursor position (without selecting anything). Without the parent knowing the selection beforehand, I think the semantics of the prop would be weird, and even then I'm not sure if there's a way to insert text at the cursor the way we want. I'm beginning to think having a transformText that works with controlled inputs would be a good idea, though I'm not sure how to handle that along with updating props.value...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, I remember running into that when I tried. @chrisnojima did mention there was some way to listen to all DOM events and then filter out the selection ones on a given component, but I didn't know enough to pursue that.

...and then, yeah, you can try implementing transformText on a controlled component, but then things get murky w.r.t. the source of truth...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akalin-keybase OK, rather than going down the murky source of truth route, I added a getSelection method. I added an example of a controlled input setting both at the same time (getSelection -> change props.value -> setSelection). Let me know what you think

* the same time if you don't want bad things to happen. Note that a
* selection will only appear when the input is focused. Call `focus()`
* before this if you want to be sure the user will see the selection.
**/
setSelection: Selection => void;
/**
* This can only be used when the input is uncontrolled. Like `setSelection`,
* if you want to be sure the user will see a selection use `focus()` before
* calling this.
**/
transformText: (fn: (TextInfo) => TextInfo, reflectChange?: boolean) => void;
}
88 changes: 85 additions & 3 deletions shared/common-adapters/plain-input.native.js
Expand Up @@ -4,8 +4,11 @@ import {getStyle as getTextStyle} from './text'
import {NativeTextInput} from './native-wrappers.native'
import {collapseStyles, globalColors, styleSheetCreate} from '../styles'
import {isIOS} from '../constants/platform'
import {checkTextInfo} from './input.shared'
import {pick} from 'lodash-es'
import logger from '../logger'

import type {InternalProps} from './plain-input'
import type {InternalProps, TextInfo, Selection} from './plain-input'

type ContentSizeChangeEvent = {nativeEvent: {contentSize: {width: number, height: number}}}

Expand All @@ -26,18 +29,92 @@ class PlainInput extends Component<InternalProps, State> {
focused: false,
height: null,
}

_input: ?NativeTextInput
_lastNativeText: ?string
_lastNativeSelection: ?Selection

// TODO remove this when we can use forwardRef with react-redux. That'd let us
// use HOCTimers with this component.
// https://github.com/reduxjs/react-redux/pull/1000
_timeoutIDs = []

_setInputRef = (ref: ?NativeTextInput) => {
this._input = ref
}

_setTimeout = (fn: () => void, timeoutMS: number) => {
this._timeoutIDs.push(setTimeout(fn, timeoutMS))
}

componentWillUnmount() {
this._timeoutIDs.forEach(clearTimeout)
}

// Needed to support wrapping with e.g. a ClickableBox. See
// https://facebook.github.io/react-native/docs/direct-manipulation.html .
setNativeProps = (nativeProps: Object) => {
this._input && this._input.setNativeProps(nativeProps)
}

transformText = (fn: TextInfo => TextInfo) => {
const controlled = !!this.props.value
if (controlled) {
const errMsg =
'Attempted to use transformText on controlled input component. Use props.value and setSelection instead.'
logger.error(errMsg)
throw new Error(errMsg)
}
const currentTextInfo = {
text: this._lastNativeText || '',
selection: this._lastNativeSelection || {start: 0, end: 0},
}
const newTextInfo = fn(currentTextInfo)
checkTextInfo(newTextInfo)
this.setNativeProps({text: newTextInfo.text})
this._lastNativeText = newTextInfo.text
this._setSelection(newTextInfo.selection)
}

getSelection = () => this._lastNativeSelection || {start: 0, end: 0}

setSelection = (s: Selection) => {
const controlled = !!this.props.value
if (!controlled) {
const errMsg =
'Attempted to use setSelection on uncontrolled input component. Use transformText instead'
logger.error(errMsg)
throw new Error(errMsg)
}
this._setSelection(s)
}

_setSelection = (selection: Selection) => {
this._setTimeout(() => {
// Validate that this selection makes sense with current value
let {start, end} = selection
const text = this._lastNativeText || '' // TODO write a good internal getValue fcn for this
end = Math.max(0, Math.min(end, text.length))
start = Math.min(start, end)
const newSelection = {start, end}
this.setNativeProps({selection: newSelection})
this._lastNativeSelection = selection
}, 0)
}

_onChangeText = (t: string) => {
this._lastNativeText = t
this.props.onChangeText && this.props.onChangeText(t)
}

_onSelectionChange = (event: {nativeEvent: {selection: Selection}}) => {
const {start: _start, end: _end} = event.nativeEvent.selection
// Work around Android bug which sometimes puts end before start:
// https://github.com/facebook/react-native/issues/18579 .
const start = Math.min(_start, _end)
const end = Math.max(_start, _end)
this._lastNativeSelection = {start, end}
}

_onContentSizeChange = (event: ContentSizeChangeEvent) => {
if (this.props.multiline) {
let height = event.nativeEvent.contentSize.height
Expand Down Expand Up @@ -116,16 +193,18 @@ class PlainInput extends Component<InternalProps, State> {

_getProps = () => {
const common: any = {
...pick(this.props, ['maxLength', 'value']), // Props we should only passthrough if supplied
autoCapitalize: this.props.autoCapitalize || 'none',
autoCorrect: !!this.props.autoCorrect,
autoFocus: this.props.autoFocus,
editable: !this.props.disabled,
keyboardType: this.props.keyboardType,
multiline: false,
onBlur: this._onBlur,
onChangeText: this.props.onChangeText,
onChangeText: this._onChangeText,
onEndEditing: this.props.onEndEditing,
onFocus: this._onFocus,
onSelectionChange: this._onSelectionChange,
onSubmitEditing: this.props.onEnterKeyDown,
placeholder: this.props.placeholder,
placeholderTextColor: this.props.placeholderColor || globalColors.black_40,
Expand All @@ -151,6 +230,9 @@ class PlainInput extends Component<InternalProps, State> {

render = () => {
const props = this._getProps()
if (props.value) {
this._lastNativeText = props.value
}
return <NativeTextInput {...props} />
}
}
Expand Down