BaseAutoFill: Fix a bug where BaseAutoFill would not work with composed languages #3713
Conversation
…se. Also made some small improvements to structure
| @@ -141,6 +141,19 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil | |||
| this._inputElement.setSelectionRange(0, 0); | |||
| } | |||
|
|
|||
| @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
|
Yep! I made sure it worked in combo box as well. |
…ed languages (microsoft#3713) * enable the base auto fill to work with composed languages like Japanese. Also made some small improvements to structure * adding change file * removing unecessary import * fix lint * Update BasePicker Snap * Fixing comments and minor bug in firefox * Make onchange better and update snaps * adding in comments * fixing comments * small code cleanup
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)