Skip to content

Commit

Permalink
performance(editor) Number Input Repeated Keypresses (#6395)
Browse files Browse the repository at this point in the history
- Changed `NumberInput` `repeatThreshold` to 600 to match the canvas.
- `NumberInput.onKeyDown` now makes use of a timeout to fire the
non-transient
  update after a brief pause.
- Removed `NumberInput.onKeyUp`.
  • Loading branch information
seanparsons authored Sep 23, 2024
1 parent 1d38e63 commit 844f47e
Show file tree
Hide file tree
Showing 5 changed files with 218 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ import { invalidGroupStateToString } from '../../canvas/canvas-strategies/strate
import selectEvent from 'react-select-event'
import { MixedPlaceholder } from '../../../uuiui/inputs/base-input'
import { cmdModifier } from '../../../utils/modifiers'
import type { SinonFakeTimers } from 'sinon'
import sinon from 'sinon'

async function getControl(
controlTestId: string,
Expand Down Expand Up @@ -163,7 +165,25 @@ async function pressKeyTimes(
})
}

function configureSinonSetupTeardown(): { clock: { current: SinonFakeTimers } } {
let clock: { current: SinonFakeTimers } = { current: null as any } // it will be non-null thanks to beforeEach
beforeEach(function () {
// TODO there is something wrong with sinon fake timers here that remotely break other tests that come after these. If your new browser tests are broken, this may be the reason.
clock.current = sinon.useFakeTimers({
// the timers will tick so the editor is not totally broken, but we can fast-forward time at will
// WARNING: the Sinon fake timers will advance in 20ms increments
shouldAdvanceTime: true,
})
})
afterEach(function () {
clock.current?.restore()
})
return { clock: clock }
}

describe('inspector tests with real metadata', () => {
const { clock } = configureSinonSetupTeardown()

it('padding controls', async () => {
const renderResult = await renderTestEditorWithCode(
makeTestProjectCodeWithSnippet(`
Expand Down Expand Up @@ -1731,6 +1751,7 @@ describe('inspector tests with real metadata', () => {
expect(widthControl.value).toBe('0')

await pressKeyTimes(widthControl, renderResult, ['ArrowUp'])
clock.current.tick(700)
expect(widthControl.value).toBe('1')

await setControlValue('frame-width-number-input', '100', renderResult.renderedDOM)
Expand Down
10 changes: 9 additions & 1 deletion editor/src/uuiui/inputs/number-input.spec.browser2.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "checkPlaceholder"] }] */
import type { RenderResult } from '@testing-library/react'
import { render } from '@testing-library/react'
import { act, fireEvent, render } from '@testing-library/react'
import React from 'react'
import {
getStoreHook,
TestInspectorContextProvider,
} from '../../components/inspector/common/inspector.test-utils'
import { NumberInput } from './number-input'
import type { CSSNumber, UnknownOrEmptyInput } from '../../components/inspector/common/css-utils'
import {
cssNumber,
isEmptyInputValue,
isUnknownInputValue,
} from '../../components/inspector/common/css-utils'
import keycode from 'keycode'
import { wait } from '../../utils/utils.test-utils'

describe('NumberInput', () => {
function checkPlaceholder(renderResult: RenderResult, expectedPlaceholder: string | null): void {
Expand Down
21 changes: 0 additions & 21 deletions editor/src/uuiui/inputs/number-input.spec.ts

This file was deleted.

160 changes: 160 additions & 0 deletions editor/src/uuiui/inputs/number-input.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import React from 'react'
import type { CSSNumber, UnknownOrEmptyInput } from '../../components/inspector/common/css-utils'
import {
cssNumber,
isEmptyInputValue,
isUnknownInputValue,
} from '../../components/inspector/common/css-utils'
import { act, fireEvent, render } from '@testing-library/react'
import {
getStoreHook,
TestInspectorContextProvider,
} from '../../components/inspector/common/inspector.test-utils'
import { calculateDragDirectionDelta, NumberInput } from './number-input'
import keycode from 'keycode'

jest.useFakeTimers()

describe('NumberInput', () => {
it('calculateDragDirectionDelta should be able to invert its results', () => {
const testCases = [
[5, 2],
[2, 5],
[-5.5, 2],
[-2, 5],
[0, 4],
[3.2, 1],
[-6, 3],
[-3, 6],
]

testCases.forEach(([delta, scaling]) => {
const { result, inverse } = calculateDragDirectionDelta(delta, scaling)
expect(inverse(result)).toEqual(delta)
})
})
it('triggers multiple transient updates and a single non-transient one when holding a key down and eventually releasing it', async () => {
const storeHookForTest = getStoreHook()
let value = cssNumber(100)
let transientUpdates: Array<UnknownOrEmptyInput<CSSNumber>> = []

function updateValue(number: UnknownOrEmptyInput<CSSNumber>): void {
if (isUnknownInputValue(number) || isEmptyInputValue(number)) {
throw new Error(`Unexpected value: ${number}`)
} else {
value = number
}
reRender()
}

function onTransientSubmitValue(number: UnknownOrEmptyInput<CSSNumber>): void {
transientUpdates.push(number)
updateValue(number)
}
let nonTransientUpdates: Array<UnknownOrEmptyInput<CSSNumber>> = []
function onSubmitValue(number: UnknownOrEmptyInput<CSSNumber>): void {
nonTransientUpdates.push(number)
updateValue(number)
}

function reRender() {
return render(
<TestInspectorContextProvider
selectedViews={storeHookForTest.getState().editor.selectedViews}
editorStoreData={storeHookForTest}
>
<NumberInput
testId='keydowntest'
controlStatus='simple'
value={value}
numberType={'AnyValid'}
defaultUnitToHide={null}
// eslint-disable-next-line react/jsx-no-bind
onTransientSubmitValue={onTransientSubmitValue}
// eslint-disable-next-line react/jsx-no-bind
onSubmitValue={onSubmitValue}
/>
</TestInspectorContextProvider>,
)
}

const result = reRender()
const inputElement = result.getByTestId('keydowntest')
for (let count = 0; count < 10; count++) {
act(() => {
fireEvent(
inputElement,
new KeyboardEvent('keydown', {
bubbles: true,
cancelable: true,
key: 'ArrowUp',
keyCode: keycode('ArrowUp'),
}),
)
fireEvent(
inputElement,
new KeyboardEvent('keyup', {
bubbles: true,
cancelable: true,
key: 'ArrowUp',
keyCode: keycode('ArrowUp'),
}),
)
})
}
// Need this for the delay that is in the `NumberInput`.
jest.runAllTimers()
expect(transientUpdates).toMatchInlineSnapshot(`
Array [
Object {
"unit": null,
"value": 101,
},
Object {
"unit": null,
"value": 102,
},
Object {
"unit": null,
"value": 103,
},
Object {
"unit": null,
"value": 104,
},
Object {
"unit": null,
"value": 105,
},
Object {
"unit": null,
"value": 106,
},
Object {
"unit": null,
"value": 107,
},
Object {
"unit": null,
"value": 108,
},
Object {
"unit": null,
"value": 109,
},
Object {
"unit": null,
"value": 110,
},
]
`)
expect(nonTransientUpdates).toMatchInlineSnapshot(`
Array [
Object {
"unit": null,
"value": 110,
},
]
`)
})
})
51 changes: 28 additions & 23 deletions editor/src/uuiui/inputs/number-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function calculateDragDelta(

let incrementTimeout: number | undefined = undefined
let incrementAnimationFrame: number | undefined = undefined
const repeatThreshold: number = 500
const repeatThreshold: number = 600

export interface NumberInputOptions {
innerLabel?: React.ReactChild
Expand Down Expand Up @@ -464,32 +464,39 @@ export const NumberInput = React.memo<NumberInputProps>(
[inputProps],
)

const clearIncrementTimeouts = React.useCallback(() => {
if (incrementTimeout != null) {
window.clearTimeout(incrementTimeout)
incrementTimeout = undefined
}
if (incrementAnimationFrame != null) {
window.cancelAnimationFrame(incrementAnimationFrame ?? 0)
incrementAnimationFrame = undefined
}
}, [])

const onKeyDown = React.useCallback(
(e: React.KeyboardEvent<HTMLInputElement>) => {
if (e.key === 'ArrowUp') {
updateValue(incrementBy(stepSize, e.shiftKey, true))
} else if (e.key === 'ArrowDown') {
if (e.key === 'ArrowUp' || e.key === 'ArrowDown') {
e.preventDefault()
updateValue(incrementBy(-stepSize, e.shiftKey, true))
const shiftKey = e.shiftKey
const changeBy = e.key === 'ArrowUp' ? stepSize : -stepSize
const newValue = incrementBy(changeBy, shiftKey, true)
clearIncrementTimeouts()
incrementTimeout = window.setTimeout(() => {
if (onSubmitValue != null) {
onSubmitValue(newValue, false)
} else if (onForcedSubmitValue != null) {
onForcedSubmitValue(newValue, false)
}
}, repeatThreshold)
} else if (e.key === 'Enter' || e.key === 'Escape') {
e.nativeEvent.stopImmediatePropagation()
e.preventDefault()
ref.current?.blur()
}
},
[updateValue, incrementBy, stepSize],
)

const onKeyUp = React.useCallback(
(e: React.KeyboardEvent<HTMLInputElement>) => {
// todo make sure this isn't doubling up the value submit
if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && onForcedSubmitValue != null) {
if (value != null) {
onForcedSubmitValue(value, false)
}
}
},
[onForcedSubmitValue, value],
[stepSize, incrementBy, clearIncrementTimeouts, onSubmitValue, onForcedSubmitValue],
)

const onBlur = React.useCallback(
Expand Down Expand Up @@ -633,10 +640,9 @@ export const NumberInput = React.memo<NumberInputProps>(
window.addEventListener('mouseup', onDecrementMouseUp)
const shiftKey = e.shiftKey
const newValue = incrementBy(-stepSize, shiftKey, false)
incrementTimeout = window.setTimeout(
() => repeatIncrement(newValue, -stepSize, shiftKey, true),
repeatThreshold,
)
incrementTimeout = window.setTimeout(() => {
repeatIncrement(newValue, -stepSize, shiftKey, true)
}, repeatThreshold)
}
},
[incrementBy, stepSize, repeatIncrement, onDecrementMouseUp, disabled],
Expand Down Expand Up @@ -879,7 +885,6 @@ export const NumberInput = React.memo<NumberInputProps>(
placeholder={inputProps.placeholder ?? placeholder}
onFocus={onFocus}
onKeyDown={onKeyDown}
onKeyUp={onKeyUp}
onBlur={onBlur}
onChange={onChange}
autoComplete='off'
Expand Down

0 comments on commit 844f47e

Please sign in to comment.