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

BaseAutoFill: Fix a bug where BaseAutoFill would not work with composed languages #3713

Merged
merged 10 commits into from
Jan 23, 2018
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "BaseAutoFill: Fixed a bug where baseautofill would not work with composed languages like Japanese",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "joschect@microsoft.com"
}
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ exports[`ComboBox Renders ComboBox correctly 1`] = `
onBlur={[Function]}
onChange={[Function]}
onClick={[Function]}
onCompositionEnd={[Function]}
onCompositionStart={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onKeyUp={[Function]}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil
newValue = this.props.updateValueInWillReceiveProps();
}

if (this._autoFillEnabled && this._doesTextStartWith(nextProps.suggestedDisplayValue!, newValue ? newValue : this._value)) {
newValue = nextProps.suggestedDisplayValue;
}
newValue = this._getDisplayValue(newValue ? newValue : this._value, nextProps.suggestedDisplayValue);

if (typeof newValue === 'string') {
this.setState({ displayValue: newValue });
Expand Down Expand Up @@ -118,11 +116,13 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil
const nativeProps = getNativeProps(this.props, inputProperties);
return (
<input
{ ...nativeProps}
{ ...nativeProps }
ref={ this._resolveRef('_inputElement') }
value={ displayValue }
autoCapitalize={ 'off' }
autoComplete={ 'off' }
onCompositionStart={ this._onCompositionStart }
onCompositionEnd={ this._onCompositionEnd }
onChange={ this._onChange }
onKeyDown={ this._onKeyDown }
onClick={ this.props.onClick ? this.props.onClick : this._onClick }
Expand All @@ -141,6 +141,25 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil
this._inputElement.setSelectionRange(0, 0);
}

// Composition events are used when the character/text requires several keystrokes to be completed.
// Some examples of this are mobile text input and langauges like Japanese or Arabic.
// Find out more at https://developer.mozilla.org/en-US/docs/Web/Events/compositionstart
@autobind
private _onCompositionStart(ev: React.CompositionEvent<HTMLInputElement>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should have a comment somewhere explaining what 'composition' is

this._autoFillEnabled = false;
}

// Composition events are used when the character/text requires several keystrokes to be completed.
// Some examples of this are mobile text input and langauges like Japanese or Arabic.
// Find out more at https://developer.mozilla.org/en-US/docs/Web/Events/compositionstart
@autobind
private _onCompositionEnd(ev: React.CompositionEvent<HTMLInputElement>) {
let inputValue = this._getCurrentInputValue();
this._tryEnableAutofill(inputValue, this.value, false, true);
// Due to timing, this needs to be async, otherwise no text will be selected.
this._async.setTimeout(() => this._updateValue(inputValue), 0);
}

@autobind
private _onClick() {
if (this._value && this._value !== '' && this._autoFillEnabled) {
Expand All @@ -154,37 +173,66 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil
this.props.onKeyDown(ev);
}

switch (ev.which) {
case KeyCodes.backspace:
this._autoFillEnabled = false;
break;
case KeyCodes.left:
if (this._autoFillEnabled) {
// If the event is actively being composed, then don't alert autofill.
// Right now typing does not have isComposing, once that has been fixed any should be removed.
if (!(ev.nativeEvent as any).isComposing) {
switch (ev.which) {
case KeyCodes.backspace:
this._autoFillEnabled = false;
}
break;
case KeyCodes.right:
if (this._autoFillEnabled) {
this._autoFillEnabled = false;
}
break;
default:
if (!this._autoFillEnabled) {
if (this.props.enableAutoFillOnKeyPress!.indexOf(ev.which) !== -1) {
this._autoFillEnabled = true;
break;
case KeyCodes.left:
Copy link
Contributor

Choose a reason for hiding this comment

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

left/right cases are identical and can be combined

case KeyCodes.right:
if (this._autoFillEnabled) {
this._value = this.state.displayValue!;
this._autoFillEnabled = false;
}
}
break;
break;
default:
if (!this._autoFillEnabled) {
if (this.props.enableAutoFillOnKeyPress!.indexOf(ev.which) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

this._autoFillEnabled = true;
}
}
break;
}
}
}

@autobind
private _onChange(ev: React.FormEvent<HTMLElement>) {
let value: string = (ev.target as HTMLInputElement).value;
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);
this._updateValue(value);
}

private _getCurrentInputValue(ev?: React.FormEvent<HTMLElement>) {
if (ev && ev.target && (ev.target as any).value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of "as any" why not just "as InputElement" (or whatever it's called) since you know what it's going to be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

return (ev && ev.target && ev.target.value) || this._inputElement.value;

}

/**
* 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.
* Autofill should never be set to true if the value is composing. Once compositionEnd is called, then
Copy link
Contributor

Choose a reason for hiding this comment

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

"value is BEING composed"? not sure of the right verbiage here...
also you should describe what composition is

* it should be completed.
* See https://developer.mozilla.org/en-US/docs/Web/API/CompositionEvent for more information on composition.
* @param newValue
* @param oldValue
* @param isComposing if true then the text is actively being composed and it has not completed.
* @param isComposed if the text is a composed text value.
*/
private _tryEnableAutofill(newValue: string, oldValue: string, isComposing?: boolean, isComposed?: boolean) {
if (!isComposing
&& newValue
&& this._inputElement.selectionStart === newValue.length
&& !this._autoFillEnabled
&& (newValue.length > oldValue.length || isComposed)) {
this._autoFillEnabled = true;
}
this._updateValue(value);
}

private _notifyInputChange(newValue: string) {
Expand All @@ -193,20 +241,36 @@ export class BaseAutoFill extends BaseComponent<IBaseAutoFillProps, IBaseAutoFil
}
}

/**
* Updates the current input value as well as getting a new display value.
* @param newValue The new value from the input
*/
@autobind
private _updateValue(newValue: string) {
this._value = this.props.onInputChange ? this.props.onInputChange(newValue) : newValue;
let displayValue = newValue;
if (this.props.suggestedDisplayValue &&
this._doesTextStartWith(this.props.suggestedDisplayValue, displayValue)
&& this._autoFillEnabled) {
displayValue = this.props.suggestedDisplayValue;
}
this.setState({
displayValue: newValue
displayValue: this._getDisplayValue(this._value, this.props.suggestedDisplayValue)
}, () => this._notifyInputChange(this._value));
}

/**
* Returns a string that should be used as the display value.
* It evaluates this based on whether or not the suggested value starts with the input value
* and whether or not autofill is enabled.
* @param inputValue the value that the input currently has.
* @param suggestedDisplayValue the possible full value
*/
private _getDisplayValue(inputValue: string, suggestedDisplayValue?: string) {
let displayValue = inputValue;
if (suggestedDisplayValue
&& inputValue
&& this._doesTextStartWith(suggestedDisplayValue, displayValue)
&& this._autoFillEnabled) {
displayValue = suggestedDisplayValue;
}
return displayValue;
}

private _doesTextStartWith(text: string, startWith: string) {
if (!text || !startWith) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ exports[`Pickers BasePicker renders BasePicker correctly 1`] = `
disabled={undefined}
onChange={[Function]}
onClick={[Function]}
onCompositionEnd={[Function]}
onCompositionStart={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
role="combobox"
Expand Down Expand Up @@ -119,6 +121,8 @@ exports[`Pickers TagPicker renders TagPicker correctly 1`] = `
disabled={undefined}
onChange={[Function]}
onClick={[Function]}
onCompositionEnd={[Function]}
onCompositionStart={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
role="combobox"
Expand Down