From c55be1b3739db316c21224bea802a205eb805ebe Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 11 Aug 2020 12:29:53 -0400 Subject: [PATCH] [core] fix(InputGroup): async controlled updates (#4266) InputGroup now uses a new private component called AsyncControllableInput which works around some problems in React dealing with async controlled updates to text inputs which arise during IME composition. --- .../forms/asyncControllableInput.tsx | 110 ++++++++++++++++++ .../core/src/components/forms/inputGroup.tsx | 7 +- .../core/test/controls/numericInputTests.tsx | 6 +- .../forms/asyncControllableInputTests.tsx | 82 +++++++++++++ packages/core/test/index.ts | 1 + 5 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 packages/core/src/components/forms/asyncControllableInput.tsx create mode 100644 packages/core/test/forms/asyncControllableInputTests.tsx diff --git a/packages/core/src/components/forms/asyncControllableInput.tsx b/packages/core/src/components/forms/asyncControllableInput.tsx new file mode 100644 index 0000000000..abcba9a18f --- /dev/null +++ b/packages/core/src/components/forms/asyncControllableInput.tsx @@ -0,0 +1,110 @@ +/* ! + * Copyright 2020 Palantir Technologies, Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as React from "react"; +import { polyfill } from "react-lifecycles-compat"; + +export interface IAsyncControllableInputProps + extends React.DetailedHTMLProps, HTMLInputElement> { + inputRef?: React.LegacyRef; +} + +export interface IAsyncControllableInputState { + /** + * Whether we are in the middle of a composition event. + * @default false + */ + isComposing: boolean; + + /** + * The source of truth for the input value. This is not updated during IME composition. + * It may be updated by a parent component. + * @default "" + */ + externalValue: IAsyncControllableInputProps["value"]; + + /** + * The latest input value, which updates during IME composition. If undefined, we use + * externalValue instead. + */ + localValue: IAsyncControllableInputProps["value"]; +} + +/** + * A stateful wrapper around the low-level component which works around a + * [React bug](https://github.com/facebook/react/issues/3926). This bug is reproduced when an input + * receives CompositionEvents (for example, through IME composition) and has its value prop updated + * asychronously. This might happen if a component chooses to do async validation of a value + * returned by the input's `onChange` callback. + * + * Implementation adapted from https://jsfiddle.net/m792qtys/ (linked in the above issue thread). + * + * Note: this component does not apply any Blueprint-specific styling. + */ +@polyfill +export class AsyncControllableInput extends React.PureComponent< + IAsyncControllableInputProps, + IAsyncControllableInputState +> { + public state: IAsyncControllableInputState = { + externalValue: this.props.value, + isComposing: false, + localValue: this.props.value, + }; + + public static getDerivedStateFromProps({ value }: IAsyncControllableInputProps) { + return { + externalValue: value, + }; + } + + public render() { + const { isComposing, externalValue, localValue } = this.state; + const { inputRef, ...restProps } = this.props; + return ( + + ); + } + + private handleCompositionStart = (e: React.CompositionEvent) => { + this.setState({ + isComposing: true, + // Make sure that localValue matches externalValue, in case externalValue + // has changed since the last onChange event. + localValue: this.state.externalValue, + }); + this.props.onCompositionStart?.(e); + }; + + private handleCompositionEnd = (e: React.CompositionEvent) => { + this.setState({ isComposing: false }); + this.props.onCompositionEnd?.(e); + }; + + private handleChange = (e: React.ChangeEvent) => { + const { value } = e.target; + + this.setState({ localValue: value }); + this.props.onChange?.(e); + }; +} diff --git a/packages/core/src/components/forms/inputGroup.tsx b/packages/core/src/components/forms/inputGroup.tsx index 6e606c82a9..5c92e98c2d 100644 --- a/packages/core/src/components/forms/inputGroup.tsx +++ b/packages/core/src/components/forms/inputGroup.tsx @@ -29,6 +29,7 @@ import { removeNonHTMLProps, } from "../../common/props"; import { Icon, IconName } from "../icon/icon"; +import { AsyncControllableInput } from "./asyncControllableInput"; // NOTE: This interface does not extend HTMLInputProps due to incompatiblity with `IControlledProps`. // Instead, we union the props in the component definition, which does work and properly disallows `string[]` values. @@ -106,7 +107,7 @@ export class InputGroup extends AbstractPureComponent2 {this.maybeRenderLeftElement()} - {this.maybeRenderRightElement()} diff --git a/packages/core/test/controls/numericInputTests.tsx b/packages/core/test/controls/numericInputTests.tsx index 6d17280158..131dfd4b8b 100644 --- a/packages/core/test/controls/numericInputTests.tsx +++ b/packages/core/test/controls/numericInputTests.tsx @@ -752,7 +752,7 @@ describe("", () => { const inputField = component.find("input"); inputField.simulate("change", { target: { value: "-5" } }); - inputField.simulate("blur"); + inputField.simulate("blur", { target: { value: "-5" } }); expect(component.state().value).to.equal(MIN.toString()); }); @@ -762,7 +762,7 @@ describe("", () => { const inputField = component.find("input"); inputField.simulate("change", { target: { value: "5" } }); - inputField.simulate("blur"); + inputField.simulate("blur", { target: { value: "5" } }); expect(component.state().value).to.equal(MAX.toString()); }); @@ -775,7 +775,7 @@ describe("", () => { const inputField = component.find("input"); inputField.simulate("change", { target: { value: "-5" } }); - inputField.simulate("blur"); + inputField.simulate("blur", { target: { value: "-5" } }); const args = onValueChange.getCall(1).args; expect(onValueChange.calledTwice).to.be.true; diff --git a/packages/core/test/forms/asyncControllableInputTests.tsx b/packages/core/test/forms/asyncControllableInputTests.tsx new file mode 100644 index 0000000000..66096a9673 --- /dev/null +++ b/packages/core/test/forms/asyncControllableInputTests.tsx @@ -0,0 +1,82 @@ +/* + * Copyright 2020 Palantir Technologies, Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { assert } from "chai"; +import { mount } from "enzyme"; +import * as React from "react"; +import { spy } from "sinon"; + +// this component is not part of the public API, but we want to test its implementation in isolation +import { AsyncControllableInput } from "../../src/components/forms/asyncControllableInput"; + +describe("", () => { + it("renders an input", () => { + const wrapper = mount(); + assert.strictEqual(wrapper.childAt(0).type(), "input"); + }); + + it("accepts controlled updates", () => { + const wrapper = mount(); + assert.strictEqual(wrapper.find("input").prop("value"), "hi"); + wrapper.setProps({ value: "bye" }); + assert.strictEqual(wrapper.find("input").prop("value"), "bye"); + }); + + it("triggers onChange events during composition", () => { + const handleChangeSpy = spy(); + const wrapper = mount(); + const input = wrapper.find("input"); + + input.simulate("compositionstart", { data: "" }); + input.simulate("compositionupdate", { data: " " }); + // some browsers trigger this change event during composition, so we test to ensure that our wrapper component does too + input.simulate("change", { target: { value: "hi " } }); + input.simulate("compositionupdate", { data: " ." }); + input.simulate("change", { target: { value: "hi ." } }); + input.simulate("compositionend", { data: " ." }); + + assert.strictEqual(handleChangeSpy.callCount, 2); + }); + + it("external updates do not override in-progress composition", async () => { + const wrapper = mount(); + const input = wrapper.find("input"); + + input.simulate("compositionstart", { data: "" }); + input.simulate("compositionupdate", { data: " " }); + input.simulate("change", { target: { value: "hi " } }); + + await Promise.resolve(); + wrapper.setProps({ value: "bye" }).update(); + + assert.strictEqual(wrapper.find("input").prop("value"), "hi "); + }); + + it("external updates flush after composition ends", async () => { + const wrapper = mount(); + const input = wrapper.find("input"); + + input.simulate("compositionstart", { data: "" }); + input.simulate("compositionupdate", { data: " " }); + input.simulate("change", { target: { value: "hi " } }); + input.simulate("compositionend", { data: " " }); + + await Promise.resolve(); + wrapper.setProps({ value: "bye" }).update(); + + assert.strictEqual(wrapper.find("input").prop("value"), "bye"); + }); +}); diff --git a/packages/core/test/index.ts b/packages/core/test/index.ts index 494e40d304..5fd98dc123 100644 --- a/packages/core/test/index.ts +++ b/packages/core/test/index.ts @@ -34,6 +34,7 @@ import "./controls/radioGroupTests"; import "./dialog/dialogTests"; import "./drawer/drawerTests"; import "./editable-text/editableTextTests"; +import "./forms/asyncControllableInputTests"; import "./forms/fileInputTests"; import "./forms/formGroupTests"; import "./forms/textAreaTests";