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

Binder does not always invalidate bad input #2702

Open
vursen opened this issue Sep 5, 2024 · 5 comments · Fixed by #2816 · May be fixed by #2902
Open

Binder does not always invalidate bad input #2702

vursen opened this issue Sep 5, 2024 · 5 comments · Fixed by #2816 · May be fixed by #2902

Comments

@vursen
Copy link
Contributor

vursen commented Sep 5, 2024

Describe the bug

With Hilla Binder, DatePicker doesn't show as invalid when I enter "foo" and press Enter or leave the field. However, it does become invalid if I gradually remove characters from a valid date.

Screen.Recording.2024-09-05.at.13.06.25.mov

Expected-behavior

Fields should invalidate when bad input is applied.

Reproduction

https://vaadin.com/docs/latest/components/date-picker#custom-validation

System Info

Latest version

@vursen vursen added bug Something isn't working hilla Issues related to Hilla labels Sep 5, 2024
@vursen
Copy link
Contributor Author

vursen commented Sep 5, 2024

Made a quick investigation and from what I understand, Binder currently listens to input, change, and blur events to update its state and trigger validation:

element.addEventListener('input', updateValueFromElement);
const changeBlurHandler = () => {
updateValueFromElement();
binderNode.visited = true;
};
element.addEventListener('blur', changeBlurHandler);
element.addEventListener('change', changeBlurHandler);

However, this is apparently not always sufficient. I'm also not sure if running validation on input makes sense for all components, as not all of them update their value on input.

So, assuming that different components can have different validation triggers, it might be better to delegate the validation triggering to the component itself by listening to the validated event instead. For text fields, it would trigger validation on change and additionally on input if the field is already invalid (since this is the current behavior of the web component). For date picker, validation would be triggered on change, unparsable-change, overlay close, blur, etc; there are plenty of triggers.

@platosha
Copy link
Contributor

platosha commented Oct 8, 2024

Possibly related to #2341

@krissvaa krissvaa self-assigned this Oct 9, 2024
platosha pushed a commit that referenced this issue Oct 17, 2024
…2816)

Invalidate input if bad input found by component
Fixes: #2702
vaadin-bot pushed a commit that referenced this issue Oct 17, 2024
…2816)

Invalidate input if bad input found by component
Fixes: #2702
taefi pushed a commit that referenced this issue Oct 21, 2024
…2816) (CP: 24.5) (#2845)

fix: invalidate binder node if bad input found by component already (#2816)

Invalidate input if bad input found by component
Fixes: #2702

Co-authored-by: Kriss Seglins <[email protected]>
@vursen
Copy link
Contributor Author

vursen commented Oct 23, 2024

I’m reopening this issue as it still persists. Based on the tests, the referenced fix seems to address the case where an unparsable value is set by the developer via the value property, which is a slightly different issue. The issue described in this ticket occurs when the user enters an unparsable value and, for example, presses Enter. In this scenario, neither change nor blur are fired, so Binder isn't notified about the validation.

In our discussion with Anton, we agreed that a reasonable solution would be to replace the input, change, and blur event subscriptions with a subscription to the validated event when the field is a Vaadin component.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 24.5.1.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 24.6.0.alpha3 and is also targeting the upcoming stable 24.6.0 version.

platosha added a commit that referenced this issue Nov 12, 2024
@platosha platosha linked a pull request Nov 12, 2024 that will close this issue
platosha added a commit that referenced this issue Nov 29, 2024
platosha added a commit that referenced this issue Dec 3, 2024
platosha added a commit that referenced this issue Dec 3, 2024
platosha added a commit that referenced this issue Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment