BaseAutoFill: Fix a bug where BaseAutoFill would not work with composed languages#3713
Conversation
…se. Also made some small improvements to structure
dmichon-msft
left a comment
dmichon-msft
left a comment
There was a problem hiding this comment.
I'm concerned that values in this.props.enableAutoFillOnKeyPress still bypass the composition check, especially since the up and down arrows (which are the defaults) are the default keys for selecting inputs in the composition window.
| } | ||
|
|
||
| @autobind | ||
| private _onCompositionStart(ev: React.CompositionEvent<HTMLInputElement>) { |
There was a problem hiding this comment.
you should have a comment somewhere explaining what 'composition' is
| if (this.props.enableAutoFillOnKeyPress!.indexOf(ev.which) !== -1) { | ||
| this._autoFillEnabled = true; | ||
| break; | ||
| case KeyCodes.left: |
There was a problem hiding this comment.
left/right cases are identical and can be combined
| break; | ||
| default: | ||
| if (!this._autoFillEnabled) { | ||
| if (this.props.enableAutoFillOnKeyPress!.indexOf(ev.which) !== -1) { |
There was a problem hiding this comment.
I'm seeing a lot of this.props.foo!
Remember that Partial exists now. Maybe you can make an interface of IPropsAllRequired, and expose Partial as the actual props the component takes, and then use IPropsAllRequired internally.
| if (value && (ev.target as HTMLInputElement).selectionStart === value.length && !this._autoFillEnabled && value.length > this._value.length) { | ||
| let value: string = this._getCurrentInputValue(ev); | ||
| // Right now typing does not have isComposing, once that has been fixed any should be removed. | ||
| this._tryEnableAutoFill(value, this._value, (ev.nativeEvent as any).isComposing); |
There was a problem hiding this comment.
if the 'any' is giving warnings, you could also try ev.nativeEvent['isComposing']
| } | ||
|
|
||
| private _getCurrentInputValue(ev?: React.FormEvent<HTMLElement>) { | ||
| if (ev && ev.target && (ev.target as any).value) { |
There was a problem hiding this comment.
instead of "as any" why not just "as InputElement" (or whatever it's called) since you know what it's going to be
There was a problem hiding this comment.
Because we really should be using the text coming from the input event. Tests started failing if that wasn't a possibility.
| return (ev.target as any).value; | ||
| } | ||
| let value: string = this._inputElement.value; | ||
| return value; |
There was a problem hiding this comment.
return (ev && ev.target && ev.target.value) || this._inputElement.value;
| * @param isComposing | ||
| * @param isComposed | ||
| */ | ||
| private _tryEnableAutoFill(newValue: string, oldValue: string, isComposing?: boolean, isComposed?: boolean) { |
There was a problem hiding this comment.
is autofill one or two words? =)
| * Attempts to enable autofill. Whether or not autofill is enabled depends on the input value, | ||
| * whether or not any text is selected, and only if the new input value is longer than the old input value. | ||
| * isComposed is whether or not the new value is a composed value. | ||
| * Autofill should never be set to true if the value is composing. Once compositionEnd is called, then |
There was a problem hiding this comment.
"value is BEING composed"? not sure of the right verbiage here...
also you should describe what composition is
| * it should be completed. | ||
| * @param newValue | ||
| * @param oldValue | ||
| * @param isComposing |
There was a problem hiding this comment.
comment on the difference between isComposing and isComposed
jspurlin
left a comment
jspurlin
left a comment
There was a problem hiding this comment.
Make sure to validate that the behavior is not adversely affected for the other fabric components which are composed with BaseAutoFill before checking in
|
Yep! I made sure it worked in combo box as well. |
4 participants
Pull request checklist
$ npm run changeDescription of changes
BaseAutoFill: Fix a bug where BaseAutoFill would not work with composed languages
Focus areas to test
(optional)