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

fix: ensure validation between value-changed and change events #6256

Merged
merged 1 commit into from
Jul 27, 2023
Merged

fix: ensure validation between value-changed and change events #6256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 27 additions & 9 deletions packages/select/src/vaadin-select-base-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@ export const SelectBaseMixin = (superClass) =>
menuElement.addEventListener(
'click',
() => {
this.__userInteraction = true;
this.opened = false;
Copy link
Contributor Author

@vursen vursen Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a bit more context. The issue emerged when I prevented validation on close when working on the dirty state. Validation didn't get triggered by this line anymore and should validate before change event on Enter started to fail when running with the Lit version of the component. It turned out that the change event was fired synchronously while all validation was performed asynchronously (in the value change observer which was delayed until the next update). To fix this, I had to do a refactoring that ensures the correct sequence of events for both Polymer and Lit, which resulted in this PR.

Copy link
Contributor Author

@vursen vursen Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note, this line appears to be unnecessary after all, as the overlay is closed anyway here. However, this may change the sequence of events: opened-changed may be fired after value-changed now. Let me know if you think it's safer to keep this line.

this.__dispatchChangePending = true;
},
true,
);
Expand All @@ -281,8 +280,10 @@ export const SelectBaseMixin = (superClass) =>
_valueChanged(value, oldValue) {
this.toggleAttribute('has-value', Boolean(value));

// Validate only if `value` changes after initialization.
if (oldValue !== undefined) {
// Skip validation during initialization and when
// a change event is scheduled, as validation will be
// triggered by `__dispatchChange()` in that case.
if (oldValue !== undefined && !this.__dispatchChangePending) {
this.validate();
}
}
Expand Down Expand Up @@ -322,7 +323,7 @@ export const SelectBaseMixin = (superClass) =>
const currentIdx = selected !== undefined ? selected : -1;
const newIdx = this._menuElement._searchKey(currentIdx, e.key);
if (newIdx >= 0) {
this.__userInteraction = true;
this.__dispatchChangePending = true;

// Announce the value selected with the first letter shortcut
this._updateAriaLive(true);
Expand Down Expand Up @@ -371,7 +372,12 @@ export const SelectBaseMixin = (superClass) =>
if (this._openedWithFocusRing) {
this.setAttribute('focus-ring', '');
}
this.validate();

// Skip validation when a change event is scheduled, as validation
// will be triggered by `__dispatchChange()` in that case.
if (!this.__dispatchChangePending) {
this.validate();
}
}
}

Expand Down Expand Up @@ -508,10 +514,9 @@ export const SelectBaseMixin = (superClass) =>
if (!this._valueChanging) {
this._selectedChanging = true;
this.value = selected.value || '';
if (this.__userInteraction) {
if (this.__dispatchChangePending) {
this.opened = false;
this.dispatchEvent(new CustomEvent('change', { bubbles: true }));
this.__userInteraction = false;
this.__dispatchChange();
}
delete this._selectedChanging;
}
Expand Down Expand Up @@ -600,4 +605,17 @@ export const SelectBaseMixin = (superClass) =>
listBox.appendChild(this.__createItemElement(item));
});
}

/** @private */
async __dispatchChange() {
// Wait for the update complete to guarantee that value-changed is fired
// before validated and change events when using the Lit version of the component.
if (this.updateComplete) {
await this.updateComplete;
}

this.validate();
this.dispatchEvent(new CustomEvent('change', { bubbles: true }));
this.__dispatchChangePending = false;
}
};
3 changes: 2 additions & 1 deletion packages/select/test/select.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,10 +634,11 @@ describe('vaadin-select', () => {
expect(changeSpy.called).to.be.false;
});

it('should fire `change` event when value changes when alphanumeric keys are pressed', () => {
it('should fire `change` event when value changes when alphanumeric keys are pressed', async () => {
keyDownChar(valueButton, 'o');
keyDownChar(valueButton, 'p');
keyDownChar(valueButton, 't');
await nextUpdate(select);
expect(changeSpy.callCount).to.equal(1);
});

Expand Down
10 changes: 7 additions & 3 deletions packages/select/test/validation.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import '@vaadin/item/vaadin-item.js';
import '@vaadin/list-box/vaadin-list-box.js';

describe('validation', () => {
let select, validateSpy, changeSpy;
let select, validateSpy, valueChangedSpy, changeSpy;

describe('basic', () => {
beforeEach(async () => {
Expand All @@ -19,6 +19,8 @@ describe('validation', () => {
validateSpy = sinon.spy(select, 'validate');
changeSpy = sinon.spy();
select.addEventListener('change', changeSpy);
valueChangedSpy = sinon.spy();
select.addEventListener('value-changed', valueChangedSpy);
});

it('should pass validation by default', () => {
Expand Down Expand Up @@ -48,15 +50,17 @@ describe('validation', () => {
expect(validateSpy.called).to.be.true;
});

it('should validate before change event on Enter', async () => {
it('should validate between value-changed and change events on Enter', async () => {
select.focus();
select.click();
await nextRender();

await sendKeys({ press: 'Enter' });
await nextUpdate(select);
expect(valueChangedSpy.calledOnce).to.be.true;
expect(validateSpy.calledOnce).to.be.true;
expect(changeSpy.calledOnce).to.be.true;
expect(validateSpy.called).to.be.true;
expect(validateSpy.calledAfter(valueChangedSpy)).to.be.true;
expect(validateSpy.calledBefore(changeSpy)).to.be.true;
});

Expand Down