From 89539827fc21b6390c18935016ca2f256142fb44 Mon Sep 17 00:00:00 2001 From: Henry Stoke Date: Mon, 14 Mar 2022 12:06:36 +0900 Subject: [PATCH 1/3] Add timeout to composition end --- .../components/forms/asyncControllableInput.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/core/src/components/forms/asyncControllableInput.tsx b/packages/core/src/components/forms/asyncControllableInput.tsx index 1e04f551b8..52a4e20226 100644 --- a/packages/core/src/components/forms/asyncControllableInput.tsx +++ b/packages/core/src/components/forms/asyncControllableInput.tsx @@ -78,6 +78,8 @@ export class AsyncControllableInput extends React.PureComponent< value: this.props.value, }; + private compositionStatusTimeoutID: number | null = null; + public static getDerivedStateFromProps( nextProps: IAsyncControllableInputProps, nextState: IAsyncControllableInputState, @@ -134,17 +136,17 @@ export class AsyncControllableInput extends React.PureComponent< } 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. - nextValue: this.state.value, - }); + if (this.compositionStatusTimeoutID !== null) { + window.clearTimeout(this.compositionStatusTimeoutID); + this.compositionStatusTimeoutID = null; + } + + this.setState({ isComposing: true }); this.props.onCompositionStart?.(e); }; private handleCompositionEnd = (e: React.CompositionEvent) => { - this.setState({ isComposing: false }); + this.compositionStatusTimeoutID = window.setTimeout(() => this.setState({ isComposing: false }), 10); this.props.onCompositionEnd?.(e); }; From ad36a1e9d1d1f95c05a6debcf270ea7d91b48873 Mon Sep 17 00:00:00 2001 From: Henry Stoke Date: Mon, 14 Mar 2022 13:58:53 +0900 Subject: [PATCH 2/3] Add test and comment --- .../forms/asyncControllableInput.tsx | 3 +++ .../forms/asyncControllableInputTests.tsx | 23 ++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/core/src/components/forms/asyncControllableInput.tsx b/packages/core/src/components/forms/asyncControllableInput.tsx index 52a4e20226..205eba8dc7 100644 --- a/packages/core/src/components/forms/asyncControllableInput.tsx +++ b/packages/core/src/components/forms/asyncControllableInput.tsx @@ -146,6 +146,9 @@ export class AsyncControllableInput extends React.PureComponent< }; private handleCompositionEnd = (e: React.CompositionEvent) => { + // In some non-latin languages, a keystroke can end a composition event and immediately afterwards open another. + // This timeout unlocks nextValue to be overwritten by the `value` prop, if and only if a delay (10ms) has + // passed without a new composition event starting. this.compositionStatusTimeoutID = window.setTimeout(() => this.setState({ isComposing: false }), 10); this.props.onCompositionEnd?.(e); }; diff --git a/packages/core/test/forms/asyncControllableInputTests.tsx b/packages/core/test/forms/asyncControllableInputTests.tsx index 986ff6141d..66a3a6590e 100644 --- a/packages/core/test/forms/asyncControllableInputTests.tsx +++ b/packages/core/test/forms/asyncControllableInputTests.tsx @@ -86,6 +86,25 @@ describe("", () => { assert.strictEqual(wrapper.find("input").prop("value"), "hi "); }); + it("external updates DO NOT flush with immediately ongoing compositions", async () => { + const wrapper = mount(); + const input = wrapper.find("input"); + + input.simulate("compositionstart", { data: "" }); + input.simulate("compositionupdate", { data: " " }); + input.simulate("change", { target: { value: "hi " } }); + + wrapper.setProps({ value: "bye" }).update(); + + input.simulate("compositionend", { data: " " }); + input.simulate("compositionstart", { data: "" }); + + // Wait for the composition ending delay (10ms) to pass + await new Promise(resolve => setTimeout(() => resolve(null), 15)); + + assert.strictEqual(wrapper.find("input").prop("value"), "hi "); + }); + it("external updates flush after composition ends", async () => { const wrapper = mount(); const input = wrapper.find("input"); @@ -95,7 +114,9 @@ describe("", () => { input.simulate("change", { target: { value: "hi " } }); input.simulate("compositionend", { data: " " }); - await Promise.resolve(); + // Wait for the composition ending delay (10ms) to pass + await new Promise(resolve => setTimeout(() => resolve(null), 15)); + // we are "rejecting" the composition here by supplying a different controlled value wrapper.setProps({ value: "bye" }).update(); From 24483e3d2c6f8b1ecb012b0325ed1a0828816f7e Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 15 Mar 2022 15:49:06 +0000 Subject: [PATCH 3/3] minor refactor --- .../forms/asyncControllableInput.tsx | 29 ++++++++++++------- .../forms/asyncControllableInputTests.tsx | 12 +++++--- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/packages/core/src/components/forms/asyncControllableInput.tsx b/packages/core/src/components/forms/asyncControllableInput.tsx index 205eba8dc7..3acc3055cb 100644 --- a/packages/core/src/components/forms/asyncControllableInput.tsx +++ b/packages/core/src/components/forms/asyncControllableInput.tsx @@ -17,7 +17,7 @@ import * as React from "react"; import { polyfill } from "react-lifecycles-compat"; -import { DISPLAYNAME_PREFIX } from "../../common/props"; +import { AbstractPureComponent2, DISPLAYNAME_PREFIX } from "../../common"; export interface IAsyncControllableInputProps extends React.DetailedHTMLProps, HTMLInputElement> { @@ -65,12 +65,18 @@ export interface IAsyncControllableInputState { * Note: this component does not apply any Blueprint-specific styling. */ @polyfill -export class AsyncControllableInput extends React.PureComponent< +export class AsyncControllableInput extends AbstractPureComponent2< IAsyncControllableInputProps, IAsyncControllableInputState > { public static displayName = `${DISPLAYNAME_PREFIX}.AsyncControllableInput`; + /** + * The amount of time (in milliseconds) which the input will wait after a compositionEnd event before + * unlocking its state value for external updates via props. See `handleCompositionEnd` for more details. + */ + public static COMPOSITION_END_DELAY = 10; + public state: IAsyncControllableInputState = { hasPendingUpdate: false, isComposing: false, @@ -78,7 +84,7 @@ export class AsyncControllableInput extends React.PureComponent< value: this.props.value, }; - private compositionStatusTimeoutID: number | null = null; + private cancelPendingCompositionEnd: (() => void) | null = null; public static getDerivedStateFromProps( nextProps: IAsyncControllableInputProps, @@ -136,20 +142,21 @@ export class AsyncControllableInput extends React.PureComponent< } private handleCompositionStart = (e: React.CompositionEvent) => { - if (this.compositionStatusTimeoutID !== null) { - window.clearTimeout(this.compositionStatusTimeoutID); - this.compositionStatusTimeoutID = null; - } - + this.cancelPendingCompositionEnd?.(); this.setState({ isComposing: true }); this.props.onCompositionStart?.(e); }; private handleCompositionEnd = (e: React.CompositionEvent) => { - // In some non-latin languages, a keystroke can end a composition event and immediately afterwards open another. - // This timeout unlocks nextValue to be overwritten by the `value` prop, if and only if a delay (10ms) has + // In some non-latin languages, a keystroke can end a composition event and immediately afterwards start another. + // This can lead to unexpected characters showing up in the text input. In order to circumvent this problem, we + // use a timeout which creates a delay which merges the two composition events, creating a more natural and predictable UX. + // `this.state.nextValue` will become "locked" (it cannot be overwritten by the `value` prop) until a delay (10ms) has // passed without a new composition event starting. - this.compositionStatusTimeoutID = window.setTimeout(() => this.setState({ isComposing: false }), 10); + this.cancelPendingCompositionEnd = this.setTimeout( + () => this.setState({ isComposing: false }), + AsyncControllableInput.COMPOSITION_END_DELAY, + ); this.props.onCompositionEnd?.(e); }; diff --git a/packages/core/test/forms/asyncControllableInputTests.tsx b/packages/core/test/forms/asyncControllableInputTests.tsx index 66a3a6590e..d12ca664ed 100644 --- a/packages/core/test/forms/asyncControllableInputTests.tsx +++ b/packages/core/test/forms/asyncControllableInputTests.tsx @@ -99,8 +99,10 @@ describe("", () => { input.simulate("compositionend", { data: " " }); input.simulate("compositionstart", { data: "" }); - // Wait for the composition ending delay (10ms) to pass - await new Promise(resolve => setTimeout(() => resolve(null), 15)); + // Wait for the composition ending delay to pass + await new Promise(resolve => + setTimeout(() => resolve(null), AsyncControllableInput.COMPOSITION_END_DELAY + 5), + ); assert.strictEqual(wrapper.find("input").prop("value"), "hi "); }); @@ -114,8 +116,10 @@ describe("", () => { input.simulate("change", { target: { value: "hi " } }); input.simulate("compositionend", { data: " " }); - // Wait for the composition ending delay (10ms) to pass - await new Promise(resolve => setTimeout(() => resolve(null), 15)); + // Wait for the composition ending delay to pass + await new Promise(resolve => + setTimeout(() => resolve(null), AsyncControllableInput.COMPOSITION_END_DELAY + 5), + ); // we are "rejecting" the composition here by supplying a different controlled value wrapper.setProps({ value: "bye" }).update();