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: always synchronize invalid attribute regardless of required #8884

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Mar 31, 2025

Description

Currently, FormItem synchronizes the invalid attribute only when the field is marked as required. However, this behavior seems incorrect, as required is not the only case where users may want to apply styles based on the invalid state. The current implementation also conflicts with server-side validation, as it sets the invalid state based on the result of the client-side checkValidity() method. This doesn't work in the Flow component, where the invalid state is managed by the server.

This PR refactors FormItem to synchronize the invalid attribute based exclusively on the presence of the invalid attribute on the field, without calling checkValidity() and regardless of the required state.

Fixes vaadin/flow-components#7246

Type of change

  • Bugfix

@vursen vursen changed the title fix: always synchronize invalid attribute fix: synchronize invalid attribute regardless of required Mar 31, 2025
@vursen vursen changed the title fix: synchronize invalid attribute regardless of required fix: always synchronize invalid attribute regardless of required Mar 31, 2025
@vursen vursen marked this pull request as ready for review April 1, 2025 08:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue with FormItem by ensuring that the invalid attribute is synchronized independently from the required state, which is essential for consistent styling and proper integration with server-side validation.

  • Updated tests in form-item.test.js to validate the new synchronization behavior for invalid and required attributes.
  • Refactored vaadin-form-item-mixin.js to remove reliance on checkValidity and introduce new event handling and attribute synchronization logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/form-layout/test/form-item.test.js Added tests for initial invalid state and for synchronizing attributes on field changes.
packages/form-layout/src/vaadin-form-item-mixin.js Refactored field event handling, removed outdated validation function, and implemented a unified attribute synchronization.
Comments suppressed due to low confidence (2)

packages/form-layout/src/vaadin-form-item-mixin.js:230

  • The logic in __synchronizeAttributes does not check for the manualValidation flag, which may lead to the 'invalid' attribute being toggled unexpectedly when manual validation is enabled. Consider adding an explicit check for field.manualValidation before toggling 'invalid'.
field.hasAttribute('invalid') || (field.matches(':invalid') && this.__isFieldDirty),

packages/form-layout/src/vaadin-form-item-mixin.js:194

  • [nitpick] The field selection logic might not cover cases where a field uses a custom validation mechanism without defining 'validate' or 'checkValidity'. Please verify that this condition accurately captures all valid field types intended for synchronization.
const newFieldNode = fieldNodes.find((field) => field.validate || field.checkValidity);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[formlayout] invalid property is not properly reflected from the component
2 participants