Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(number-field): update number-field value on pressing "enter" #3638

Merged
merged 3 commits into from
Sep 29, 2023
Merged

bug(number-field): update number-field value on pressing "enter" #3638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c16cb7
fix(number-field): updated onchange logic
TarunAdobe Sep 27, 2023
65ac8c0
fix(number-field): added a variable to track last committed value in …
TarunAdobe Sep 27, 2023
0e21010
chore(number-field): added test for onchange events
TarunAdobe Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions packages/number-field/src/NumberField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,12 @@ export class NumberField extends TextfieldBase {
}
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 @@ -175,6 +181,7 @@ export class NumberField extends TextfieldBase {

public override _value = NaN;
private _trackingValue = '';
private lastCommitedValue = NaN;

/**
* Retreive the value of the element parsed to a Number.
Expand Down Expand Up @@ -280,9 +287,12 @@ export class NumberField extends TextfieldBase {
this.buttons.releasePointerCapture(event.pointerId);
cancelAnimationFrame(this.nextChange);
clearTimeout(this.safty);
this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
if (this.lastCommitedValue !== this.value) {
this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
this.lastCommitedValue = this.value;
}
this.managedInput = false;
}

Expand Down Expand Up @@ -329,16 +339,10 @@ export class NumberField extends TextfieldBase {
case 'ArrowUp':
event.preventDefault();
this.increment(event.shiftKey ? this.stepModifier : 1);
this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
break;
case 'ArrowDown':
event.preventDefault();
this.decrement(event.shiftKey ? this.stepModifier : 1);
this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
break;
}
}
Expand All @@ -358,6 +362,7 @@ export class NumberField extends TextfieldBase {
this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
this.lastCommitedValue = this.value;
}, CHANGE_DEBOUNCE_MS) as unknown as number;
}
this.managedInput = false;
Expand Down Expand Up @@ -400,7 +405,11 @@ export class NumberField extends TextfieldBase {
}
}
this.value = value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will trigger the setter. Will the following lines never accidentally dispatch a second change event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no because in the setter we are updated the lastCommittedValue which will cause the check on the line 409 to fail making sure there is not second change event

super.handleChange();
this.inputElement.value = this.formattedValue;
if (this.lastCommitedValue !== this.value) {
this.lastCommitedValue = this.value;
super.handleChange();
}
}

protected handleCompositionStart(): void {
Expand Down
107 changes: 104 additions & 3 deletions packages/number-field/test/number-field.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,16 @@ describe('NumberField', () => {
changeSpy((event.target as NumberField)?.value);
});
});
it('on changing `value`', async () => {
el.focus();
await elementUpdated(el);
expect(el.focused).to.be.true;
el.value = 51;
expect(changeSpy.callCount).to.equal(1);
await elementUpdated(el);
el.value = 52;
expect(changeSpy.callCount).to.equal(2);
});
it('via scroll', async () => {
el.focus();
await elementUpdated(el);
Expand Down Expand Up @@ -507,6 +517,72 @@ describe('NumberField', () => {
expect(el.value).to.equal(50);
});
it('many input, but one change', async () => {
const buttonUp = el.shadowRoot.querySelector(
'.step-up'
) as HTMLElement;
const buttonUpRect = buttonUp.getBoundingClientRect();
const buttonUpPosition: [number, number] = [
buttonUpRect.x + buttonUpRect.width / 2,
buttonUpRect.y + buttonUpRect.height / 2,
];
const buttonDown = el.shadowRoot.querySelector(
'.step-down'
) as HTMLElement;
const buttonDownRect = buttonDown.getBoundingClientRect();
const buttonDownPosition: [number, number] = [
buttonDownRect.x + buttonDownRect.width / 2,
buttonDownRect.y + buttonDownRect.height / 2,
];
sendMouse({
steps: [
{
type: 'move',
position: buttonUpPosition,
},
{
type: 'down',
},
],
});
await oneEvent(el, 'input');
expect(el.value).to.equal(51);
expect(inputSpy.callCount).to.equal(1);
expect(changeSpy.callCount).to.equal(0);
await oneEvent(el, 'input');
expect(el.value).to.equal(52);
expect(inputSpy.callCount).to.equal(2);
expect(changeSpy.callCount).to.equal(0);
await oneEvent(el, 'input');
expect(el.value).to.equal(53);
expect(inputSpy.callCount).to.equal(3);
expect(changeSpy.callCount).to.equal(0);
sendMouse({
steps: [
{
type: 'move',
position: buttonDownPosition,
},
],
});
let framesToWait = FRAMES_PER_CHANGE * 2;
while (framesToWait) {
// input is only processed onces per FRAMES_PER_CHANGE number of frames
framesToWait -= 1;
await nextFrame();
}
expect(inputSpy.callCount).to.equal(5);
expect(changeSpy.callCount).to.equal(0);
await sendMouse({
steps: [
{
type: 'up',
},
],
});
expect(inputSpy.callCount).to.equal(5);
expect(changeSpy.callCount).to.equal(1);
});
it('no change in committed value - using buttons', async () => {
const buttonUp = el.shadowRoot.querySelector(
'.step-up'
) as HTMLElement;
Expand Down Expand Up @@ -566,7 +642,10 @@ describe('NumberField', () => {
],
});
expect(inputSpy.callCount).to.equal(4);
expect(changeSpy.callCount).to.equal(1);
expect(
changeSpy.callCount,
'value does not change from initial value so no "change" event is dispatched'
).to.equal(0);
});
});
it('accepts pointer interactions with the stepper UI', async () => {
Expand Down Expand Up @@ -751,7 +830,10 @@ describe('NumberField', () => {
await sendKeys({ press: 'Enter' });
await elementUpdated(el);
expect(lastInputValue, 'last input value').to.equal(10);
expect(lastChangeValue, 'last change value').to.equal(10);
expect(lastChangeValue, 'last change value').to.equal(
0,
'value does not change from initial value so no "change" event is dispatched'
);
expect(el.formattedValue).to.equal('10');
expect(el.valueAsString).to.equal('10');
expect(el.value).to.equal(10);
Expand Down Expand Up @@ -785,6 +867,14 @@ describe('NumberField', () => {
expect(el.valueAsString).to.equal('5');
expect(el.value).to.equal(5);
});
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;
await elementUpdated(el);
expect(lastChangeValue, 'last change value').to.equal(10);
});
});
describe('min', () => {
let el: NumberField;
Expand All @@ -811,11 +901,22 @@ describe('NumberField', () => {
await sendKeys({ press: 'Enter' });
await elementUpdated(el);
expect(lastInputValue, 'last input value').to.equal(10);
expect(lastChangeValue, 'last change value').to.equal(10);
expect(lastChangeValue, 'last change value').to.equal(
0,
'value does not change from initial value so no "change" event is dispatched'
);
expect(el.formattedValue).to.equal('10');
expect(el.valueAsString).to.equal('10');
expect(el.value).to.equal(10);
});
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;
await elementUpdated(el);
expect(lastChangeValue, 'last change value').to.equal(10);
});
xit('manages `inputMode` in iPhone', async () => {
// setUserAgent is not currently supported by Playwright
await setUserAgent(
Expand Down