Skip to content

Commit

Permalink
fix(number-field): prevent over excited "change" events
Browse files Browse the repository at this point in the history
  • Loading branch information
@Westbrook
Westbrook committed Oct 18, 2023
1 parent da9d58b commit 7b93724
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 48 deletions.
52 changes: 27 additions & 25 deletions packages/number-field/src/NumberField.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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 })
);
Expand All @@ -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;
}
}
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
80 changes: 63 additions & 17 deletions packages/number-field/test/number-field.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -884,23 +888,49 @@ 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);
});
});
describe('min', () => {
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');
Expand All @@ -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 () => {
Expand Down
93 changes: 90 additions & 3 deletions packages/slider/test/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Slider>(
html`
<sp-slider
editable
hide-stepper
min="1"
max="100"
step="1"
label="Slider label"
@input=${(event: Event & { target: Slider }) => {
inputSpy(event.target.value);
}}
@change=${(event: Event & { target: Slider }) => {
changeSpy(event.target.value);
}}
></sp-slider>
`
);
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 `<sp-number-field>`', async () => {
const inputSpy = spy();
const changeSpy = spy();
Expand Down
Loading

0 comments on commit 7b93724

Please sign in to comment.