From 7b937241151cad5cfc9e5a03fa70c4b70ac0cbea Mon Sep 17 00:00:00 2001 From: Westbrook Johnson Date: Mon, 16 Oct 2023 18:42:33 -0400 Subject: [PATCH] fix(number-field): prevent over excited "change" events --- packages/number-field/src/NumberField.ts | 52 ++++++----- .../number-field/test/number-field.test.ts | 80 ++++++++++++---- packages/slider/test/index.ts | 93 ++++++++++++++++++- packages/slider/test/slider.test.ts | 13 ++- test/testing-helpers.ts | 2 +- 5 files changed, 192 insertions(+), 48 deletions(-) diff --git a/packages/number-field/src/NumberField.ts b/packages/number-field/src/NumberField.ts index 172ee802b9..aa68dfbadf 100644 --- a/packages/number-field/src/NumberField.ts +++ b/packages/number-field/src/NumberField.ts @@ -158,14 +158,9 @@ export class NumberField extends TextfieldBase { if (value === this.value) { return; } + this.lastCommitedValue = value; const oldValue = this._value; this._value = value; - if (!this.managedInput && this.lastCommitedValue !== this.value) { - this.dispatchEvent( - new Event('change', { bubbles: true, composed: true }) - ); - this.lastCommitedValue = this.value; - } this.requestUpdate('value', oldValue); } @@ -181,7 +176,23 @@ export class NumberField extends TextfieldBase { public override _value = NaN; private _trackingValue = ''; - private lastCommitedValue = NaN; + private lastCommitedValue?: number; + + private setValue(value: number = this.value): void { + this.value = value; + if ( + typeof this.lastCommitedValue === 'undefined' || + this.lastCommitedValue === this.value + ) { + // Do not announce when the value is unchanged. + return; + } + + this.dispatchEvent( + new Event('change', { bubbles: true, composed: true }) + ); + this.lastCommitedValue = this.value; + } /** * Retreive the value of the element parsed to a Number. @@ -287,13 +298,8 @@ export class NumberField extends TextfieldBase { this.buttons.releasePointerCapture(event.pointerId); cancelAnimationFrame(this.nextChange); clearTimeout(this.safty); - if (this.lastCommitedValue !== this.value) { - this.dispatchEvent( - new Event('change', { bubbles: true, composed: true }) - ); - this.lastCommitedValue = this.value; - } this.managedInput = false; + this.setValue(); } private doNextChange(event: PointerEvent): number { @@ -314,10 +320,9 @@ export class NumberField extends TextfieldBase { let value = this.value; value += count * this._step; if (isNaN(this.value)) { - this.value = min; - } else { - this.value = value; + value = min; } + this._value = this.validateInput(value); this.dispatchEvent( new Event('input', { bubbles: true, composed: true }) ); @@ -339,10 +344,12 @@ export class NumberField extends TextfieldBase { case 'ArrowUp': event.preventDefault(); this.increment(event.shiftKey ? this.stepModifier : 1); + this.setValue(); break; case 'ArrowDown': event.preventDefault(); this.decrement(event.shiftKey ? this.stepModifier : 1); + this.setValue(); break; } } @@ -359,10 +366,7 @@ export class NumberField extends TextfieldBase { this.stepBy(direction * (event.shiftKey ? this.stepModifier : 1)); clearTimeout(this.queuedChangeEvent); this.queuedChangeEvent = setTimeout(() => { - this.dispatchEvent( - new Event('change', { bubbles: true, composed: true }) - ); - this.lastCommitedValue = this.value; + this.setValue(); }, CHANGE_DEBOUNCE_MS) as unknown as number; } this.managedInput = false; @@ -404,12 +408,8 @@ export class NumberField extends TextfieldBase { return; } } - this.value = value; + this.setValue(value); this.inputElement.value = this.formattedValue; - if (this.lastCommitedValue !== this.value) { - this.lastCommitedValue = this.value; - super.handleChange(); - } } protected handleCompositionStart(): void { @@ -447,6 +447,8 @@ export class NumberField extends TextfieldBase { .map((char) => remapMultiByteCharacters[char] || char) .join(''); if (this.numberParser.isValidPartialNumber(value)) { + // Use starting value as this.value is the `input` value. + this.lastCommitedValue = this.lastCommitedValue ?? this.value; const valueAsNumber = this.convertValueToNumber(value); if (!value && this.indeterminateValue) { this.indeterminate = true; diff --git a/packages/number-field/test/number-field.test.ts b/packages/number-field/test/number-field.test.ts index c7d3a7c94e..7334bea01f 100644 --- a/packages/number-field/test/number-field.test.ts +++ b/packages/number-field/test/number-field.test.ts @@ -53,12 +53,6 @@ describe('NumberField', () => { before(async () => { const shouldPolyfillEn = shouldPolyfill('en'); const shouldPolyfillFr = shouldPolyfill('fr'); - // eslint-disable-next-line no-console - console.log({ - en: shouldPolyfillEn, - fr: shouldPolyfillFr, - shouldPolyfill, - }); if (shouldPolyfillEn || shouldPolyfillFr) { await import('@formatjs/intl-numberformat/polyfill-force.js'); } @@ -376,15 +370,15 @@ describe('NumberField', () => { changeSpy((event.target as NumberField)?.value); }); }); - it('on changing `value`', async () => { + it('except when changing `value` from the outside', async () => { el.focus(); await elementUpdated(el); expect(el.focused).to.be.true; el.value = 51; - expect(changeSpy.callCount).to.equal(1); + expect(changeSpy.callCount).to.equal(0); await elementUpdated(el); el.value = 52; - expect(changeSpy.callCount).to.equal(2); + expect(changeSpy.callCount).to.equal(0); }); it('via scroll', async () => { el.focus(); @@ -825,13 +819,23 @@ describe('NumberField', () => { let el: NumberField; let lastInputValue = 0; let lastChangeValue = 0; + const inputSpy = spy(); + const changeSpy = spy(); beforeEach(async () => { + inputSpy.resetHistory(); + changeSpy.resetHistory(); el = await getElFrom( Default({ max: 10, value: 10, - onInput: (value: number) => (lastInputValue = value), - onChange: (value: number) => (lastChangeValue = value), + onInput: (value: number) => { + inputSpy(value); + lastInputValue = value; + }, + onChange: (value: number) => { + changeSpy(value); + lastChangeValue = value; + }, }) ); expect(el.formattedValue).to.equal('10'); @@ -884,9 +888,25 @@ describe('NumberField', () => { it('dispatches onchange on setting max value', async () => { el.value = 5; await elementUpdated(el); - expect(lastChangeValue, 'last change value').to.equal(5); - el.value = 15; + expect(changeSpy.callCount).to.equal(0); + expect(el.value).to.equal(5); + el.focus(); + await sendKeys({ + press: 'Backspace', + }); + await sendKeys({ + press: '1', + }); + await sendKeys({ + press: '5', + }); + await sendKeys({ + press: 'Enter', + }); await elementUpdated(el); + expect(el.value).to.equal(10); + expect(inputSpy.callCount).to.equal(3); + expect(changeSpy.callCount).to.equal(1); expect(lastChangeValue, 'last change value').to.equal(10); }); }); @@ -894,13 +914,23 @@ describe('NumberField', () => { let el: NumberField; let lastInputValue = 0; let lastChangeValue = 0; + const inputSpy = spy(); + const changeSpy = spy(); beforeEach(async () => { + inputSpy.resetHistory(); + changeSpy.resetHistory(); el = await getElFrom( Default({ min: 10, value: 10, - onInput: (value: number) => (lastInputValue = value), - onChange: (value: number) => (lastChangeValue = value), + onInput: (value: number) => { + inputSpy(value); + lastInputValue = value; + }, + onChange: (value: number) => { + changeSpy(value); + lastChangeValue = value; + }, }) ); expect(el.formattedValue).to.equal('10'); @@ -926,9 +956,25 @@ describe('NumberField', () => { it('dispatches onchange on setting min value', async () => { el.value = 15; await elementUpdated(el); - expect(lastChangeValue, 'last change value').to.equal(15); - el.value = 5; + expect(changeSpy.callCount).to.equal(0); + expect(el.value).to.equal(15); + el.focus(); + await sendKeys({ + press: 'Backspace', + }); + await sendKeys({ + press: 'Backspace', + }); + await sendKeys({ + press: '5', + }); + await sendKeys({ + press: 'Enter', + }); await elementUpdated(el); + expect(el.value).to.equal(10); + expect(inputSpy.callCount).to.equal(3); + expect(changeSpy.callCount).to.equal(1); expect(lastChangeValue, 'last change value').to.equal(10); }); xit('manages `inputMode` in iPhone', async () => { diff --git a/packages/slider/test/index.ts b/packages/slider/test/index.ts index eb7fc5dde0..4a24bc80e6 100644 --- a/packages/slider/test/index.ts +++ b/packages/slider/test/index.ts @@ -17,11 +17,15 @@ import { Indeterminate, StoryArgs, } from '../stories/slider.stories.js'; -import { elementUpdated, expect, fixture } from '@open-wc/testing'; -import { TemplateResult } from '@spectrum-web-components/base'; +import { elementUpdated, expect, nextFrame } from '@open-wc/testing'; +import { html, TemplateResult } from '@spectrum-web-components/base'; import { sendKeys } from '@web/test-runner-commands'; import { spy } from 'sinon'; -import { testForLitDevWarnings } from '../../../test/testing-helpers.js'; +import { + fixture, + testForLitDevWarnings, +} from '../../../test/testing-helpers.js'; +import { sendMouse } from '../../../test/plugins/browser.js'; async function sliderFromFixture( sliderFixture: (args: StoryArgs) => TemplateResult @@ -103,6 +107,89 @@ export const testEditableSlider = (type: string): void => { expect(el.shadowRoot.activeElement).to.equal(el.numberField); }); + it('dispatches `input` of the animation frame', async () => { + const inputSpy = spy(); + const changeSpy = spy(); + const el = await fixture( + html` + { + inputSpy(event.target.value); + }} + @change=${(event: Event & { target: Slider }) => { + changeSpy(event.target.value); + }} + > + ` + ); + await elementUpdated(el); + expect(el.value).to.equal(50.5); + + expect(inputSpy.callCount, 'start clean').to.equal(0); + expect(changeSpy.callCount, 'start clean').to.equal(0); + + const handle = el.shadowRoot.querySelector( + '.handle' + ) as HTMLDivElement; + const rect = handle.getBoundingClientRect(); + let frames = 0; + let shouldCountFrames = true; + const countFrames = (): void => { + if (!shouldCountFrames) return; + frames += 1; + requestAnimationFrame(countFrames); + }; + countFrames(); + type Steps = { + type: 'move'; + position: [number, number]; + }[]; + const toRight: Steps = [...Array(51).keys()].map((i) => ({ + type: 'move', + position: [ + rect.left + rect.width / 2 + i, + rect.top + rect.height / 2, + ], + })); + const toLeft: Steps = toRight.slice(0, -1).reverse(); + await sendMouse({ + steps: [ + { + type: 'move', + position: [ + rect.left + rect.width / 2, + rect.top + rect.height / 2, + ], + }, + { + type: 'down', + }, + ...toRight, + ...toLeft, + { + type: 'up', + }, + ], + }); + shouldCountFrames = false; + await elementUpdated(el); + await nextFrame(); + await nextFrame(); + + expect(el.value).to.gt(50.5); + expect( + inputSpy.callCount, + 'should not have more "input"s than frames' + ).to.lte(frames); + expect(changeSpy.callCount, 'only one change').to.equal(1); + }); + it('edits via the ``', async () => { const inputSpy = spy(); const changeSpy = spy(); diff --git a/packages/slider/test/slider.test.ts b/packages/slider/test/slider.test.ts index 3b63c2de18..54c015ea37 100644 --- a/packages/slider/test/slider.test.ts +++ b/packages/slider/test/slider.test.ts @@ -384,18 +384,26 @@ describe('Slider', () => { it('dispatches `input` of the animation frame', async () => { const inputSpy = spy(); + const changeSpy = spy(); const el = await fixture( html` - inputSpy(target.value)} + @input=${(event: Event & { target: Slider }) => { + inputSpy(event.target.value); + }} + @change=${(event: Event & { target: Slider }) => { + changeSpy(event.target.value); + }} > ` ); await elementUpdated(el); + expect(inputSpy.callCount, 'start clean').to.equal(0); + expect(changeSpy.callCount, 'start clean').to.equal(0); + let frames = 0; let shouldCountFrames = true; const countFrames = (): void => { @@ -435,6 +443,7 @@ describe('Slider', () => { inputSpy.callCount, 'should not have more "input"s than frames' ).to.lte(frames); + expect(changeSpy.callCount, 'only one change').to.equal(1); }); it('manages RTL when min != 0', async () => { diff --git a/test/testing-helpers.ts b/test/testing-helpers.ts index 597c1dcc65..06af43a479 100644 --- a/test/testing-helpers.ts +++ b/test/testing-helpers.ts @@ -230,7 +230,7 @@ export async function fixture( story: TemplateResult ): Promise { const test = await owcFixture(html` - + ${story} `);