Only update ExtendedPicker state on input events that are not mid-com…

…position (#11303)

* Do not trigger input update in Extended picker on IME input

* Change files

* fix linter errors in comments + merge issues w/ tests

* rename wasComposing -> composing

* treat compositionEnd as composing=true, use event.nativeEvent.isComposing instead of this._isComposing in input handler

* update tests

change/office-ui-fabric-react-2019-11-25-12-34-56-u-mahuangh-extendedpicker-trigger-composition.json

@@ -0,0 +1,8 @@
+{
+  "type": "minor",
+  "comment": "Only update ExtendedPicker state on input events that are not mid-composition ",
+  "packageName": "office-ui-fabric-react",
+  "email": "mahuangh@microsoft.com",
+  "commit": "6caf1e9e0854a9ffc9fe8c4b3daaafe35ae892e9",
+  "date": "2019-11-25T20:34:56.928Z"
+}

packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md

@@ -154,7 +154,7 @@ export class BaseExtendedPicker<T, P extends IBaseExtendedPickerProps<T>> extend
     // (undocumented)
     protected onCopy: (ev: React.ClipboardEvent<HTMLElement>) => void;
     // (undocumented)
-    protected onInputChange: (value: string) => void;
+    protected onInputChange: (value: string, composing?: boolean | undefined) => void;
     // (undocumented)
     protected onInputClick: (ev: React.MouseEvent<HTMLInputElement | Autofill, MouseEvent>) => void;
     // (undocumented)
@@ -1528,8 +1528,8 @@ export interface IAutofillProps extends React.InputHTMLAttributes<HTMLInputEleme
     componentRef?: IRefObject<IAutofill>;
     defaultVisibleValue?: string;
     enableAutofillOnKeyPress?: KeyCodes[];
-    onInputChange?: (value: string) => string;
-    onInputValueChange?: (newValue?: string) => void;
+    onInputChange?: (value: string, composing: boolean) => string;
+    onInputValueChange?: (newValue?: string, composing?: boolean) => void;
     preventValueSelection?: boolean;
     shouldSelectFullInputValueInComponentDidUpdate?: () => boolean;
     suggestedDisplayValue?: string;

packages/office-ui-fabric-react/src/components/Autofill/Autofill.test.tsx

@@ -1,3 +1,5 @@
+jest.useFakeTimers();
+
 import * as React from 'react';
 
 import * as ReactTestUtils from 'react-dom/test-utils';
@@ -113,4 +115,152 @@ describe('Autofill', () => {
     expect(autofill.value).toBe('Updated');
     expect(autofill.inputElement!.value).toBe('Updated');
   });
+
+  it('will handle composition events', () => {
+    component = mount(<Autofill componentRef={autofillRef} suggestedDisplayValue="he" />);
+
+    autofill.inputElement!.value = 'he';
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+    expect(autofill.value).toBe('he');
+
+    ReactTestUtils.Simulate.compositionStart(autofill.inputElement!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.l, which: KeyCodes.l });
+    autofill.inputElement!.value = 'hel';
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.p, which: KeyCodes.p });
+    autofill.inputElement!.value = 'help';
+
+    ReactTestUtils.Simulate.compositionEnd(autofill.inputElement!, {});
+    autofill.inputElement!.value = '🆘';
+
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+
+    expect(autofill.value).toBe('🆘');
+  });
+
+  it('will handle composition events when multiple compositionEnd events are dispatched without a compositionStart', () => {
+    const onInputChange = jest.fn((a: string, b: boolean) => a);
+    component = mount(<Autofill componentRef={autofillRef} onInputChange={onInputChange} suggestedDisplayValue="he" />);
+
+    autofill.inputElement!.value = 'hel';
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+    expect(autofill.value).toBe('hel');
+
+    ReactTestUtils.Simulate.compositionStart(autofill.inputElement!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, {
+      keyCode: KeyCodes.p,
+      which: KeyCodes.p
+    });
+    autofill.inputElement!.value = 'help';
+    ReactTestUtils.Simulate.input(autofill.inputElement!, {
+      target: autofill.inputElement!,
+      nativeEvent: {
+        isComposing: true
+      } as any
+    });
+
+    ReactTestUtils.Simulate.compositionEnd(autofill.inputElement!, {});
+    autofill.inputElement!.value = '🆘';
+    ReactTestUtils.Simulate.input(autofill.inputElement!, {
+      target: autofill.inputElement!,
+      nativeEvent: {
+        isComposing: true
+      } as any
+    });
+    jest.runOnlyPendingTimers();
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, {
+      keyCode: KeyCodes.m,
+      which: KeyCodes.m,
+      nativeEvent: {
+        isComposing: true
+      } as any
+    });
+    autofill.inputElement!.value = '🆘m';
+    ReactTestUtils.Simulate.input(autofill.inputElement!, {
+      target: autofill.inputElement!,
+      nativeEvent: {
+        isComposing: true
+      } as any
+    });
+
+    ReactTestUtils.Simulate.compositionEnd(autofill.inputElement!, {});
+    autofill.inputElement!.value = '🆘Ⓜ';
+    ReactTestUtils.Simulate.input(autofill.inputElement!, {
+      target: autofill.inputElement!,
+      nativeEvent: {
+        isComposing: false
+      } as any
+    });
+    jest.runOnlyPendingTimers();
+
+    expect(onInputChange.mock.calls).toEqual([
+      ['hel', false],
+      ['help', true],
+      ['🆘', true], // from input event
+      ['🆘', false], // from timeout on compositionEnd event
+      ['🆘m', true],
+      ['🆘Ⓜ', false], // from input event
+      ['🆘Ⓜ', false] // from  timeout on compositionEnd event
+    ]);
+    expect(autofill.value).toBe('🆘Ⓜ');
+  });
+
+  it('will call onInputChange w/ composition events', () => {
+    const onInputChange = jest.fn((a: string, b: boolean) => a);
+
+    component = mount(<Autofill componentRef={autofillRef} onInputChange={onInputChange} suggestedDisplayValue="he" />);
+
+    autofill.inputElement!.value = 'he';
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+    expect(autofill.value).toBe('he');
+
+    ReactTestUtils.Simulate.compositionStart(autofill.inputElement!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.l, which: KeyCodes.l });
+    autofill.inputElement!.value = 'hel';
+    ReactTestUtils.Simulate.input(autofill.inputElement!!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.p, which: KeyCodes.p });
+    autofill.inputElement!.value = 'help';
+    ReactTestUtils.Simulate.input(autofill.inputElement!!, {});
+
+    ReactTestUtils.Simulate.compositionEnd(autofill.inputElement!, {});
+    autofill.inputElement!.value = '🆘';
+
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+
+    expect(onInputChange.mock.calls).toEqual([['he', false], ['hel', true], ['help', true], ['🆘', false]]);
+  });
+
+  it('will call onInputValueChanged w/ composition events', () => {
+    const onInputValueChange = jest.fn((a: string, b: boolean) => {
+      return void 0;
+    });
+
+    component = mount(<Autofill componentRef={autofillRef} onInputValueChange={onInputValueChange} suggestedDisplayValue="he" />);
+
+    autofill.inputElement!.value = 'he';
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+    expect(autofill.value).toBe('he');
+
+    ReactTestUtils.Simulate.compositionStart(autofill.inputElement!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.l, which: KeyCodes.l });
+    autofill.inputElement!.value = 'hel';
+    ReactTestUtils.Simulate.input(autofill.inputElement!!, {});
+
+    ReactTestUtils.Simulate.keyDown(autofill.inputElement!, { keyCode: KeyCodes.p, which: KeyCodes.p });
+    autofill.inputElement!.value = 'help';
+    ReactTestUtils.Simulate.input(autofill.inputElement!!, {});
+
+    ReactTestUtils.Simulate.compositionEnd(autofill.inputElement!, {});
+    autofill.inputElement!.value = '🆘';
+
+    ReactTestUtils.Simulate.input(autofill.inputElement!);
+
+    expect(onInputValueChange.mock.calls).toEqual([['he', false], ['hel', true], ['help', true], ['🆘', false]]);
+  });
 });

packages/office-ui-fabric-react/src/components/Autofill/Autofill.tsx

@@ -144,7 +144,7 @@ export class Autofill extends BaseComponent<IAutofillProps, IAutofillState> impl
 
   public clear() {
     this._autoFillEnabled = true;
-    this._updateValue('');
+    this._updateValue('', false);
     this._inputElement.current && this._inputElement.current.setSelectionRange(0, 0);
   }
 
@@ -161,7 +161,7 @@ export class Autofill extends BaseComponent<IAutofillProps, IAutofillState> impl
   // Find out more at https://developer.mozilla.org/en-US/docs/Web/Events/compositionstart
   private _onCompositionUpdate = () => {
     if (isIE11()) {
-      this._updateValue(this._getCurrentInputValue());
+      this._updateValue(this._getCurrentInputValue(), true);
     }
   };
 
@@ -174,8 +174,10 @@ export class Autofill extends BaseComponent<IAutofillProps, IAutofillState> impl
     this._isComposing = false;
     // Due to timing, this needs to be async, otherwise no text will be selected.
     this._async.setTimeout(() => {
-      // Call getCurrentInputValue here again since there can be a race condition where this value has changed during the async cal
-      this._updateValue(this._getCurrentInputValue());
+      // it's technically possible that the value of _isComposing is reset during this timeout,
+      // so explicitly trigger this with composing=true here, since it is supposed to be the
+      // update for composition end
+      this._updateValue(this._getCurrentInputValue(), false);
     }, 0);
   };
 
@@ -224,7 +226,9 @@ export class Autofill extends BaseComponent<IAutofillProps, IAutofillState> impl
 
     // If it is not IE11 and currently composing, update the value
     if (!(isIE11() && this._isComposing)) {
-      this._updateValue(value);
+      const nativeEventComposing = (ev.nativeEvent as any).isComposing;
+      const isComposing = nativeEventComposing === undefined ? this._isComposing : nativeEventComposing;
+      this._updateValue(value, isComposing);
     }
   };
 
@@ -268,28 +272,28 @@ export class Autofill extends BaseComponent<IAutofillProps, IAutofillState> impl
     }
   }
 
-  private _notifyInputChange(newValue: string): void {
+  private _notifyInputChange(newValue: string, composing: boolean): void {
     if (this.props.onInputValueChange) {
-      this.props.onInputValueChange(newValue);
+      this.props.onInputValueChange(newValue, composing);
     }
   }
 
   /**
    * Updates the current input value as well as getting a new display value.
    * @param newValue - The new value from the input
    */
-  private _updateValue = (newValue: string) => {
+  private _updateValue = (newValue: string, composing: boolean) => {
     // Only proceed if the value is nonempty and is different from the old value
     // This is to work around the fact that, in IE 11, inputs with a placeholder fire an onInput event on focus
     if (!newValue && newValue === this._value) {
       return;
     }
-    this._value = this.props.onInputChange ? this.props.onInputChange(newValue) : newValue;
+    this._value = this.props.onInputChange ? this.props.onInputChange(newValue, composing) : newValue;
     this.setState(
       {
         displayValue: this._getDisplayValue(this._value, this.props.suggestedDisplayValue)
       },
-      () => this._notifyInputChange(this._value)
+      () => this._notifyInputChange(this._value, composing)
     );
   };
 

packages/office-ui-fabric-react/src/components/Autofill/Autofill.types.ts

@@ -59,8 +59,12 @@ export interface IAutofillProps extends React.InputHTMLAttributes<HTMLInputEleme
 
   /**
    * A callback for when the current input value changes.
+   *
+   * @param composing - true if the change event was triggered while the
+   * inner input was in the middle of a multi-character composition.
+   * (for example, jp-hiragana IME input)
    */
-  onInputValueChange?: (newValue?: string) => void;
+  onInputValueChange?: (newValue?: string, composing?: boolean) => void;
 
   /**
    * When the user uses left arrow, right arrow, clicks, or deletes text autofill is disabled
@@ -97,8 +101,12 @@ export interface IAutofillProps extends React.InputHTMLAttributes<HTMLInputEleme
 
   /**
    * A callback used to modify the input string.
+   *
+   * @param composing - true if the change event was triggered while the
+   * inner input was in the middle of a multi-character composition.
+   * (for example, jp-hiragana IME input)
    */
-  onInputChange?: (value: string) => string;
+  onInputChange?: (value: string, composing: boolean) => string;
 
   /**
    * Should the value of the input be selected? True if we're focused on our input, false otherwise.

packages/office-ui-fabric-react/src/components/ExtendedPicker/BaseExtendedPicker.tsx

@@ -178,10 +178,13 @@ export class BaseExtendedPicker<T, P extends IBaseExtendedPickerProps<T>> extend
     );
   }
 
-  protected onInputChange = (value: string): void => {
-    this.setState({ queryString: value });
-    if (this.floatingPicker.current) {
-      this.floatingPicker.current.onQueryStringChanged(value);
+  protected onInputChange = (value: string, composing?: boolean): void => {
+    // We don't want to update the picker's suggestions when the input is still being composed
+    if (!composing) {
+      this.setState({ queryString: value });
+      if (this.floatingPicker.current) {
+        this.floatingPicker.current.onQueryStringChanged(value);
+      }
     }
   };