From 2dd34e0ac1eae19d87105668bd13155b543ca336 Mon Sep 17 00:00:00 2001 From: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com> Date: Thu, 9 Dec 2021 14:25:04 +0100 Subject: [PATCH] fix(concurrency): ensure panel stays closed after blur (#829) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François Chalifour --- bundlesize.config.json | 4 +- .../src/__tests__/concurrency.test.ts | 294 ++++++++++++++++-- .../src/__tests__/getEnvironmentProps.test.ts | 2 +- .../src/__tests__/getInputProps.test.ts | 4 +- .../src/__tests__/getSources.test.ts | 12 +- packages/autocomplete-core/src/createStore.ts | 1 + .../autocomplete-core/src/getPropGetters.ts | 30 +- packages/autocomplete-core/src/onInput.ts | 22 +- packages/autocomplete-core/src/onKeyDown.ts | 8 + .../src/types/AutocompleteStore.ts | 1 + .../createConcurrentSafePromise.test.ts | 51 ++- .../src/utils/createConcurrentSafePromise.ts | 56 ++-- 12 files changed, 410 insertions(+), 75 deletions(-) diff --git a/bundlesize.config.json b/bundlesize.config.json index 4721555a4..e71598ae0 100644 --- a/bundlesize.config.json +++ b/bundlesize.config.json @@ -2,11 +2,11 @@ "files": [ { "path": "packages/autocomplete-core/dist/umd/index.production.js", - "maxSize": "5.75 kB" + "maxSize": "6 kB" }, { "path": "packages/autocomplete-js/dist/umd/index.production.js", - "maxSize": "16.25 kB" + "maxSize": "16.5 kB" }, { "path": "packages/autocomplete-preset-algolia/dist/umd/index.production.js", diff --git a/packages/autocomplete-core/src/__tests__/concurrency.test.ts b/packages/autocomplete-core/src/__tests__/concurrency.test.ts index 193355508..8284887ea 100644 --- a/packages/autocomplete-core/src/__tests__/concurrency.test.ts +++ b/packages/autocomplete-core/src/__tests__/concurrency.test.ts @@ -1,39 +1,25 @@ import userEvent from '@testing-library/user-event'; import { AutocompleteState } from '..'; -import { createSource, defer } from '../../../../test/utils'; +import { createPlayground, createSource, defer } from '../../../../test/utils'; import { createAutocomplete } from '../createAutocomplete'; type Item = { label: string; }; +beforeEach(() => { + document.body.innerHTML = ''; +}); + describe('concurrency', () => { test('resolves the responses in order from getSources', async () => { - // These delays make the second query come back after the third one. - const sourcesDelays = [100, 150, 200]; - const itemsDelays = [0, 150, 0]; - let deferSourcesCount = -1; - let deferItemsCount = -1; - - const getSources = ({ query }) => { - deferSourcesCount++; - - return defer(() => { - return [ - createSource({ - getItems() { - deferItemsCount++; - - return defer( - () => [{ label: query }], - itemsDelays[deferItemsCount] - ); - }, - }), - ]; - }, sourcesDelays[deferSourcesCount]); - }; + const { timeout, delayedGetSources: getSources } = createDelayedGetSources({ + // These delays make the second query come back after the third one. + sources: [100, 150, 200], + items: [0, 150, 0], + }); + const onStateChange = jest.fn(); const autocomplete = createAutocomplete({ getSources, onStateChange }); const { onChange } = autocomplete.getInputProps({ inputElement: null }); @@ -45,10 +31,6 @@ describe('concurrency', () => { userEvent.type(input, 'b'); userEvent.type(input, 'c'); - const timeout = Math.max( - ...sourcesDelays.map((delay, index) => delay + itemsDelays[index]) - ); - await defer(() => {}, timeout); let stateHistory: Array< @@ -91,4 +73,258 @@ describe('concurrency', () => { document.body.removeChild(input); }); + + describe('closing the panel with pending requests', () => { + describe('without debug mode', () => { + test('keeps the panel closed on Escape', async () => { + const onStateChange = jest.fn(); + const { timeout, delayedGetSources } = createDelayedGetSources({ + sources: [100, 200], + }); + const getSources = jest.fn(delayedGetSources); + + const { inputElement } = createPlayground(createAutocomplete, { + onStateChange, + getSources, + }); + + userEvent.type(inputElement, 'ab{esc}'); + + await defer(() => {}, timeout); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: false, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(2); + }); + + test('keeps the panel closed on blur', async () => { + const onStateChange = jest.fn(); + const { timeout, delayedGetSources } = createDelayedGetSources({ + sources: [100, 200], + }); + const getSources = jest.fn(delayedGetSources); + + const { inputElement } = createPlayground(createAutocomplete, { + onStateChange, + getSources, + }); + + userEvent.type(inputElement, 'a{enter}'); + + await defer(() => {}, timeout); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: false, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(1); + }); + + test('keeps the panel closed on touchstart blur', async () => { + const onStateChange = jest.fn(); + const { timeout, delayedGetSources } = createDelayedGetSources({ + sources: [100, 200], + }); + const getSources = jest.fn(delayedGetSources); + + const { + getEnvironmentProps, + inputElement, + formElement, + } = createPlayground(createAutocomplete, { + onStateChange, + getSources, + }); + + const panelElement = document.createElement('div'); + + const { onTouchStart } = getEnvironmentProps({ + inputElement, + formElement, + panelElement, + }); + window.addEventListener('touchstart', onTouchStart); + + userEvent.type(inputElement, 'a'); + const customEvent = new CustomEvent('touchstart', { bubbles: true }); + window.document.dispatchEvent(customEvent); + + await defer(() => {}, timeout); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: false, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(1); + + window.removeEventListener('touchstart', onTouchStart); + }); + }); + + describe('with debug mode', () => { + const delay = 300; + + test('keeps the panel closed on Escape', async () => { + const onStateChange = jest.fn(); + const getSources = jest.fn(() => { + return defer(() => { + return [ + createSource({ + getItems: () => [{ label: '1' }, { label: '2' }], + }), + ]; + }, delay); + }); + const { inputElement } = createPlayground(createAutocomplete, { + debug: true, + onStateChange, + getSources, + }); + + userEvent.type(inputElement, 'a{esc}'); + + await defer(() => {}, delay); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: false, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(1); + }); + + test('keeps the panel open on blur', async () => { + const onStateChange = jest.fn(); + const getSources = jest.fn(() => { + return defer(() => { + return [ + createSource({ + getItems: () => [{ label: '1' }, { label: '2' }], + }), + ]; + }, delay); + }); + const { inputElement } = createPlayground(createAutocomplete, { + debug: true, + onStateChange, + getSources, + }); + + userEvent.type(inputElement, 'a{enter}'); + + await defer(() => {}, delay); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: true, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(1); + }); + + test('keeps the panel open on touchstart blur', async () => { + const onStateChange = jest.fn(); + const getSources = jest.fn(() => { + return defer(() => { + return [ + createSource({ + getItems: () => [{ label: '1' }, { label: '2' }], + }), + ]; + }, delay); + }); + const { + getEnvironmentProps, + inputElement, + formElement, + } = createPlayground(createAutocomplete, { + debug: true, + onStateChange, + getSources, + }); + + const panelElement = document.createElement('div'); + + const { onTouchStart } = getEnvironmentProps({ + inputElement, + formElement, + panelElement, + }); + window.addEventListener('touchstart', onTouchStart); + + userEvent.type(inputElement, 'a'); + const customEvent = new CustomEvent('touchstart', { bubbles: true }); + window.document.dispatchEvent(customEvent); + + await defer(() => {}, delay); + + expect(onStateChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ + isOpen: true, + status: 'idle', + }), + }) + ); + expect(getSources).toHaveBeenCalledTimes(1); + + window.removeEventListener('touchstart', onTouchStart); + }); + }); + }); }); + +function createDelayedGetSources(delays: { + sources: number[]; + items?: number[]; +}) { + let deferSourcesCount = -1; + let deferItemsCount = -1; + + const itemsDelays = delays.items || delays.sources.map(() => 0); + + const timeout = Math.max( + ...delays.sources.map((delay, index) => delay + itemsDelays[index]) + ); + + function delayedGetSources({ query }) { + deferSourcesCount++; + + return defer(() => { + return [ + createSource({ + getItems() { + deferItemsCount++; + + return defer( + () => [{ label: query }], + itemsDelays[deferItemsCount] + ); + }, + }), + ]; + }, delays.sources[deferSourcesCount]); + } + + return { timeout, delayedGetSources }; +} diff --git a/packages/autocomplete-core/src/__tests__/getEnvironmentProps.test.ts b/packages/autocomplete-core/src/__tests__/getEnvironmentProps.test.ts index 2c11704d3..788688ab6 100644 --- a/packages/autocomplete-core/src/__tests__/getEnvironmentProps.test.ts +++ b/packages/autocomplete-core/src/__tests__/getEnvironmentProps.test.ts @@ -30,7 +30,7 @@ describe('getEnvironmentProps', () => { }); describe('onTouchStart', () => { - test('is a noop when panel is not open', () => { + test('is a noop when panel is not open and status is idle', () => { const onStateChange = jest.fn(); const { getEnvironmentProps, diff --git a/packages/autocomplete-core/src/__tests__/getInputProps.test.ts b/packages/autocomplete-core/src/__tests__/getInputProps.test.ts index 0b27d5235..e94827e79 100644 --- a/packages/autocomplete-core/src/__tests__/getInputProps.test.ts +++ b/packages/autocomplete-core/src/__tests__/getInputProps.test.ts @@ -1894,7 +1894,7 @@ describe('getInputProps', () => { }); describe('onBlur', () => { - test('resets activeItemId and isOpen', () => { + test('resets activeItemId and isOpen', async () => { const onStateChange = jest.fn(); const { inputElement } = createPlayground(createAutocomplete, { onStateChange, @@ -1905,6 +1905,8 @@ describe('getInputProps', () => { inputElement.focus(); inputElement.blur(); + await runAllMicroTasks(); + expect(onStateChange).toHaveBeenLastCalledWith( expect.objectContaining({ state: expect.objectContaining({ diff --git a/packages/autocomplete-core/src/__tests__/getSources.test.ts b/packages/autocomplete-core/src/__tests__/getSources.test.ts index bf27cd79d..6c281501d 100644 --- a/packages/autocomplete-core/src/__tests__/getSources.test.ts +++ b/packages/autocomplete-core/src/__tests__/getSources.test.ts @@ -8,6 +8,10 @@ import { import { createAutocomplete } from '../createAutocomplete'; import * as handlers from '../onInput'; +beforeEach(() => { + document.body.innerHTML = ''; +}); + describe('getSources', () => { test('gets calls on input', () => { const getSources = jest.fn((..._args: any[]) => { @@ -140,7 +144,13 @@ describe('getSources', () => { const { inputElement } = createPlayground(createAutocomplete, { getSources() { - return [createSource({ sourceId: 'source1', getItems: () => {} })]; + return [ + createSource({ + sourceId: 'source1', + // @ts-expect-error + getItems: () => {}, + }), + ]; }, }); diff --git a/packages/autocomplete-core/src/createStore.ts b/packages/autocomplete-core/src/createStore.ts index c2197188b..8761b5e0b 100644 --- a/packages/autocomplete-core/src/createStore.ts +++ b/packages/autocomplete-core/src/createStore.ts @@ -35,5 +35,6 @@ export function createStore( onStoreStateChange({ state, prevState }); }, + shouldSkipPendingUpdate: false, }; } diff --git a/packages/autocomplete-core/src/getPropGetters.ts b/packages/autocomplete-core/src/getPropGetters.ts index f08885fd0..24cdbf41f 100644 --- a/packages/autocomplete-core/src/getPropGetters.ts +++ b/packages/autocomplete-core/src/getPropGetters.ts @@ -40,10 +40,16 @@ export function getPropGetters< // @TODO: support cases where there are multiple Autocomplete instances. // Right now, a second instance makes this computation return false. onTouchStart(event) { - if ( - store.getState().isOpen === false || - event.target === inputElement - ) { + // The `onTouchStart` event shouldn't trigger the `blur` handler when + // it's not an interaction with Autocomplete. We detect it with the + // following heuristics: + // - the panel is closed AND there are no running requests + // (no interaction with the autocomplete, no future state updates) + // - OR the touched target is the input element (should open the panel) + const isNotAutocompleteInteraction = + store.getState().isOpen === false && !onInput.isRunning(); + + if (isNotAutocompleteInteraction || event.target === inputElement) { return; } @@ -55,6 +61,14 @@ export function getPropGetters< if (isTargetWithinAutocomplete === false) { store.dispatch('blur', null); + + // If requests are still running when the user closes the panel, they + // could reopen the panel once they resolve. + // We want to prevent any subsequent query from reopening the panel + // because it would result in an unsolicited UI behavior. + if (!props.debug && onInput.isRunning()) { + store.shouldSkipPendingUpdate = true; + } } }, // When scrolling on touch devices (mobiles, tablets, etc.), we want to @@ -193,6 +207,14 @@ export function getPropGetters< // See explanation in `onTouchStart`. if (!isTouchDevice) { store.dispatch('blur', null); + + // If requests are still running when the user closes the panel, they + // could reopen the panel once they resolve. + // We want to prevent any subsequent query from reopening the panel + // because it would result in an unsolicited UI behavior. + if (!props.debug && onInput.isRunning()) { + store.shouldSkipPendingUpdate = true; + } } }, onClick: (event) => { diff --git a/packages/autocomplete-core/src/onInput.ts b/packages/autocomplete-core/src/onInput.ts index b542eb369..ba69409a6 100644 --- a/packages/autocomplete-core/src/onInput.ts +++ b/packages/autocomplete-core/src/onInput.ts @@ -115,11 +115,25 @@ export function onInput({ }) ) .then((collections) => { + // Parameters passed to `onInput` could be stale when the following code + // executes, because `onInput` calls may not resolve in order. + // If it becomes a problem we'll need to save the last passed parameters. + // See: https://codesandbox.io/s/agitated-cookies-y290z + setStatus('idle'); + + if (store.shouldSkipPendingUpdate) { + if (!runConcurrentSafePromise.isRunning()) { + store.shouldSkipPendingUpdate = false; + } + + return; + } + setCollections(collections as any); - const isPanelOpen = props.shouldPanelOpen({ - state: store.getState(), - }); + + const isPanelOpen = props.shouldPanelOpen({ state: store.getState() }); + setIsOpen( nextState.isOpen ?? ((props.openOnFocus && !query && isPanelOpen) || isPanelOpen) @@ -148,3 +162,5 @@ export function onInput({ } }); } + +onInput.isRunning = runConcurrentSafePromise.isRunning; diff --git a/packages/autocomplete-core/src/onKeyDown.ts b/packages/autocomplete-core/src/onKeyDown.ts index ee37a463d..f4fb79b9a 100644 --- a/packages/autocomplete-core/src/onKeyDown.ts +++ b/packages/autocomplete-core/src/onKeyDown.ts @@ -99,6 +99,14 @@ export function onKeyDown({ event.preventDefault(); store.dispatch(event.key, null); + + // Hitting the `Escape` key signals the end of a user interaction with the + // autocomplete. At this point, we should ignore any requests that are still + // running and could reopen the panel once they resolve, because that would + // result in an unsolicited UI behavior. + if (onInput.isRunning()) { + store.shouldSkipPendingUpdate = true; + } } else if (event.key === 'Enter') { // No active item, so we let the browser handle the native `onSubmit` form // event. diff --git a/packages/autocomplete-core/src/types/AutocompleteStore.ts b/packages/autocomplete-core/src/types/AutocompleteStore.ts index 7334a4a35..d9beaae0c 100644 --- a/packages/autocomplete-core/src/types/AutocompleteStore.ts +++ b/packages/autocomplete-core/src/types/AutocompleteStore.ts @@ -5,6 +5,7 @@ import { AutocompleteState } from './AutocompleteState'; export interface AutocompleteStore { getState(): AutocompleteState; dispatch(action: ActionType, payload: any): void; + shouldSkipPendingUpdate: boolean; } export type Reducer = ( diff --git a/packages/autocomplete-core/src/utils/__tests__/createConcurrentSafePromise.test.ts b/packages/autocomplete-core/src/utils/__tests__/createConcurrentSafePromise.test.ts index 6b0578b21..b605842c3 100644 --- a/packages/autocomplete-core/src/utils/__tests__/createConcurrentSafePromise.test.ts +++ b/packages/autocomplete-core/src/utils/__tests__/createConcurrentSafePromise.test.ts @@ -3,9 +3,7 @@ import { createConcurrentSafePromise } from '../createConcurrentSafePromise'; describe('createConcurrentSafePromise', () => { test('resolves the non-promise values in order', async () => { - type PromiseValue = { value: number }; - - const runConcurrentSafePromise = createConcurrentSafePromise(); + const runConcurrentSafePromise = createConcurrentSafePromise(); const concurrentSafePromise1 = runConcurrentSafePromise({ value: 1 }); const concurrentSafePromise2 = runConcurrentSafePromise({ value: 2 }); const concurrentSafePromise3 = runConcurrentSafePromise({ value: 3 }); @@ -18,9 +16,7 @@ describe('createConcurrentSafePromise', () => { }); test('resolves the values in order when sequenced', async () => { - type PromiseValue = { value: number }; - - const runConcurrentSafePromise = createConcurrentSafePromise(); + const runConcurrentSafePromise = createConcurrentSafePromise(); const concurrentSafePromise1 = runConcurrentSafePromise( defer(() => ({ value: 1 }), 100) ); @@ -39,9 +35,7 @@ describe('createConcurrentSafePromise', () => { }); test('resolves the value from the last call', async () => { - type PromiseValue = { value: number }; - - const runConcurrentSafePromise = createConcurrentSafePromise(); + const runConcurrentSafePromise = createConcurrentSafePromise(); const concurrentSafePromise1 = runConcurrentSafePromise( defer(() => ({ value: 1 }), 100) ); @@ -61,4 +55,43 @@ describe('createConcurrentSafePromise', () => { expect(await concurrentSafePromise2).toEqual({ value: 3 }); expect(await concurrentSafePromise3).toEqual({ value: 3 }); }); + + test('returns whether promises are currently running', async () => { + const runConcurrentSafePromise = createConcurrentSafePromise(); + const concurrentSafePromise1 = runConcurrentSafePromise( + defer(() => ({ value: 1 }), 0) + ); + const concurrentSafePromise2 = runConcurrentSafePromise( + defer(() => ({ value: 2 }), 0) + ); + const concurrentSafePromise3 = runConcurrentSafePromise( + defer(() => ({ value: 3 }), 0) + ); + + jest.runAllTimers(); + + expect(runConcurrentSafePromise.isRunning()).toBe(true); + + await concurrentSafePromise1; + await concurrentSafePromise2; + + expect(runConcurrentSafePromise.isRunning()).toBe(true); + + await concurrentSafePromise3; + + expect(runConcurrentSafePromise.isRunning()).toBe(false); + + const concurrentSafePromise4 = runConcurrentSafePromise( + defer(() => Promise.reject(new Error()), 400) + ); + + expect(runConcurrentSafePromise.isRunning()).toBe(true); + + try { + await concurrentSafePromise4; + // eslint-disable-next-line no-empty + } catch (err) {} + + expect(runConcurrentSafePromise.isRunning()).toBe(false); + }); }); diff --git a/packages/autocomplete-core/src/utils/createConcurrentSafePromise.ts b/packages/autocomplete-core/src/utils/createConcurrentSafePromise.ts index ae72460b7..6c992be11 100644 --- a/packages/autocomplete-core/src/utils/createConcurrentSafePromise.ts +++ b/packages/autocomplete-core/src/utils/createConcurrentSafePromise.ts @@ -10,35 +10,41 @@ export function createConcurrentSafePromise() { let basePromiseId = -1; let latestResolvedId = -1; let latestResolvedValue: unknown = undefined; + let runningPromisesCount = 0; - return function runConcurrentSafePromise( - promise: MaybePromise - ) { + function runConcurrentSafePromise(promise: MaybePromise) { basePromiseId++; + runningPromisesCount++; const currentPromiseId = basePromiseId; - return Promise.resolve(promise).then((x) => { - // The promise might take too long to resolve and get outdated. This would - // result in resolving stale values. - // When this happens, we ignore the promise value and return the one - // coming from the latest resolved value. - // - // +----------------------------------+ - // | 100ms | - // | run(1) +---> R1 | - // | 300ms | - // | run(2) +-------------> R2 (SKIP) | - // | 200ms | - // | run(3) +--------> R3 | - // +----------------------------------+ - if (latestResolvedValue && currentPromiseId < latestResolvedId) { - return latestResolvedValue as TValue; - } + return Promise.resolve(promise) + .then((x) => { + // The promise might take too long to resolve and get outdated. This would + // result in resolving stale values. + // When this happens, we ignore the promise value and return the one + // coming from the latest resolved value. + // + // +----------------------------------+ + // | 100ms | + // | run(1) +---> R1 | + // | 300ms | + // | run(2) +-------------> R2 (SKIP) | + // | 200ms | + // | run(3) +--------> R3 | + // +----------------------------------+ + if (latestResolvedValue && currentPromiseId < latestResolvedId) { + return latestResolvedValue as TValue; + } - latestResolvedId = currentPromiseId; - latestResolvedValue = x; + latestResolvedId = currentPromiseId; + latestResolvedValue = x; - return x; - }); - }; + return x; + }) + .finally(() => runningPromisesCount--); + } + + runConcurrentSafePromise.isRunning = () => runningPromisesCount > 0; + + return runConcurrentSafePromise; }