vaadin / flow-components

Java counterpart of Vaadin Web Components
102 stars 65 forks source link

Combobox not invalid if required-Flag is set and no value is set #2051

Open nittka opened 3 years ago

nittka commented 3 years ago

Description

We want the required-indicator to be shown on labels also in a form layout and work on a prototype based on the enhanced form layout. But even without the form layout the ComboBox seems not to work as expected (required-validation).

Expected outcome

When the value for a component marked as required (e.g. TextField or Combobox) is set, the indicator disappears. When the value is removed again, the component becomes invalid. This is indicated by the background turning red and a red required-indicator to appear.

Actual outcome

For the TextField both background and indicator are red. But for the ComboBox the indicator is green and the background does not turn read.

I may be wrong, but the isInvalid-implementation of the combobox does not correctly handle the case "required but empty" or it does not propagate the result correctly to its internal components or the html. Wheras the vaadin-text-field of the TextField has both the required and the invalid-attribute set, the vaadin-combo-box is missing these attributes. The vaadin-text-field of the ComboBox has the required-attribute, but the invalid-attribute is missing.

Minimal reproducible example

        ComboBox<String> labelComboBox = new ComboBox<String>();
        labelComboBox.setLabel("ComboBox");
        labelComboBox.setItems("Option one", "Option two");
        labelComboBox.setRequired(true);
        labelComboBox.setRequiredIndicatorVisible(true);

        TextField textField = new TextField();
        textField.setLabel("TextField");
        textField.setRequired(true);
        textField.setRequiredIndicatorVisible(true);

Steps to reproduce

  1. Add TextField and ComboBox as above to a page.
  2. Enter/select a valid value for both components.
  3. Remove the values for both components again.
  4. The TextField behaves as expected, the ComboBox does not properly indicate the missing value.

Environment

Browsers Affected

As the html already looks suspicious, I assume all browsers are affected.

nittka commented 3 years ago

I am sorry, I have to revise the error description. Without any extra styling the required indicator of the combobox does indeed become red as expected.

However, for custom styling (based on the attributes set), the combobox suffers from the issue mentioned. An underlying problem is discussed in the linked ticket.

yuriy-fix commented 3 years ago

Dear @nittka, could you please provide a reproduction with the custom styling (based on the attributes set) you've mentioned?

nittka commented 3 years ago

After looking at our code again, I think the problem really only occurs in combination with the form layout, i.e. when trying to evaluate the state on the java-side. Adding the following to the above code, the combobox is never "invalid", even if the UI styling indicates an invalid state.

labelComboBox.addValueChangeListener(e -> {
    System.out.println("*" + labelComboBox.getValue() + "* " + labelComboBox.isInvalid());
});

So with respect to the styling this ticket can really be closed. Whether the inconcsistent isInvalid-behaviour of the different base components is an error depends on the discussion around the linked ticket.

yuriy-fix commented 3 years ago

Could it be related to this one: https://github.com/vaadin/web-components/issues/835?

nittka commented 3 years ago

Yes, it is related, but I guess the focus of this ticket has changed.

We started off with the EnhancedFormLayout, but it did not meet all our (potential) requirements. For example, the indicator disappears if a value is present (without the possibility to turn this behaviour off). So in a filled out form, the user cannot distinguish between required and optional fields, anymore. Further, for text fields the indicator does not appear again, if the field is cleared.

So instead of using the EnhancedFormLayout, we tried to adapt its implementation pattern for showing/hiding/changing the color of the indicator according to our needs. And now the problem is, that the components' isInvalid-implementations are inconsistent. For the combobox the nested TextField is invalid but this state is not propagated to the combobox itself, so HasValidation-components cannot be handled generically.

At the moment, I consider this ticket "depends on #1531". As I stated there, setRequired and setRequiredIndicatorVisible are both needed and should be handled consistently by all base components. If the related attributes (including invalid) were available to the form item, all styling could be done via css.

TatuLund commented 3 years ago

For example, the indicator disappears if a value is present

Actually that is intentional behavior in EnhancedFormLayout to mimic exactly how the indicator works normally in the fields. It would be very easy to introduce API to turn this off, i.e. have sticky indicators.

nittka commented 3 years ago

I did not mean to imply that the EnhancedFormLayout does not behave as inteded. We just could not used it (as is) for our purposes.

(I may be wrong!! or our demo application uses some custom styling I overlooked. If I enter a value in a standard text field (required and requiredIndicator true), the indicator disappears. If I then remove the value again, the indicator appears in red. The EnhancedFormLayout would not mimic this behaviour.)

mrgreywater commented 3 years ago

Workaround until it's fixed:

combobox.addAttachListener(evt -> {
    if (combobox.isRequired() && combobox.getValue() == null) {
        combobox.setInvalid(true);
    }
});