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.

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(labs): allow validators to report customError #5310

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion labs/behaviors/constraint-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export function mixinConstraintValidation<
const {validity, validationMessage: nonCustomValidationMessage} =
this[privateValidator].getValidity();

const customError = !!this[privateCustomValidationMessage];
const customError = !!this[privateCustomValidationMessage] || validity.customError;
const validationMessage =
this[privateCustomValidationMessage] || nonCustomValidationMessage;

Expand Down
70 changes: 70 additions & 0 deletions labs/behaviors/constraint-validation_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import {
import {mixinElementInternals} from './element-internals.js';
import {getFormValue, mixinFormAssociated} from './form-associated.js';
import {CheckboxValidator} from './validators/checkbox-validator.js';
import {Validator} from './validators/validator.js';
import {SelectState} from './validators/select-validator.js';

describe('mixinConstraintValidation()', () => {
const baseClass = mixinConstraintValidation(
Expand Down Expand Up @@ -45,6 +47,52 @@ describe('mixinConstraintValidation()', () => {
}
}

/**
* A validator that set customError flag to true
*/
class CustomErrorValidator extends Validator<SelectState> {
private control?: HTMLInputElement;

protected override computeValidity(state: SelectState) {
if (!this.control) {
this.control = document.createElement('input');
}
this.control.setCustomValidity('validator custom error');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs this.control.required = state.required to support the multi-validation test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above: we only care about customError as this is not working properly with the current codebase.

return {
validity: this.control.validity,
validationMessage: this.control.validationMessage,
};
}

protected override equals(prev: SelectState, next: SelectState) {
return prev.value === next.value
}

protected override copy({ value, required }: SelectState) {
return { value, required };
}
}

@customElement('test-custom-error-constraint-validation')
class TestCustomErrorConstraintValidation extends baseClass {
@property() value = '';
@property({ type: Boolean }) required = false;
override render() {
return html`<div id="root"></div>`;
}
[createValidator]() {
return new CustomErrorValidator(() => this);
}

[getValidityAnchor]() {
return this.shadowRoot?.querySelector<HTMLElement>('#root') ?? null;
}

[getFormValue]() {
return String(this.value);
}
}

describe('validity', () => {
it('should return a ValidityState value', () => {
const control = new TestConstraintValidation();
Expand Down Expand Up @@ -174,4 +222,26 @@ describe('mixinConstraintValidation()', () => {
.toBe('Error');
});
});

describe('customError', () => {
it('should set customError to true when validator has customError', () => {
const control = new TestCustomErrorConstraintValidation();
expect(control.validity.customError)
.withContext('validity.customError')
.toBeTrue();
});
it('should dispatch invalid event when validator has customError', () => {
const control = new TestCustomErrorConstraintValidation();
const invalidListener = jasmine.createSpy('invalidListener');
control.addEventListener('invalid', invalidListener);
control.reportValidity();
expect(invalidListener).toHaveBeenCalledWith(jasmine.any(Event));
});
it('should report custom validation message over other validation messages', () => {
const control = new TestCustomErrorConstraintValidation();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this test needs control.required = true to ensure other validation is running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @asyncLiz

I think this is covered in other tests above in the same file. Here we are just making sure that a customError is properly reported as an error (the new test validator in the test file just does that: this.control.setCustomValidity('validator custom error');).

Those 3 new tests fail without the fix in labs/behaviors/constraint-validation.ts. They pass after changing it to:

const customError = !!this[privateCustomValidationMessage] || validity.customError;

expect(control.validationMessage)
.withContext('validationMessage')
.toBe('validator custom error');
});
})
});