[combo-box] Inconsistency when using allow-custom-values and binding undefined to value

[robrez]

Description

When binding undefined to the value property while using allow-custom-value, the string "undefined" is visible inside the input-container.

By contrast, the input-container becomes empty whenever, allow-custom-value is not being used.

Attached image demonstrates some example bindings and the resulting value which was bound internally by combo-box

Note that null works as expected. Note that in all cases above, the resulting value when allow-custom-value is not being used is always an empty string

Expected outcome

I noticed this when upgrading from v14 and would expect for the input-field to not display the string "undefined" when binding undefined

Minimal reproducible example

<vaadin-combo-box
  allow-custom-value
></vaadin-combo-box>

<script>
const items = [ 'a', 'b', 'c' ];
const comboBox = document.querySelector('vaadin-combo-box');
comboBox.items = items;
comboBox.value = undefined;
</script>

Steps to reproduce

1. Add the snippet above to HTML page
2. observe that the string "undefined" can be found in the combobox's input-container

Environment

Vaadin version(s): 24.x, 23.x OS: windows

Browsers

Chrome

[robrez]

After debugging a bit, I came up w/ a proposed solution for v23.x... I actually think that this was fixed in >v24.0.0 via https://github.com/vaadin/web-components/commit/3a5b00c6ba8dc84ba3777feace31499926b85f32

The observations I had regard v23 are below

    set _inputElementValue(value) {
      if (this.inputElement) {
        this.inputElement[this._propertyForValue] = value;
      }
    }

• https://github.com/vaadin/web-components/blob/23.3/packages/combo-box/src/vaadin-combo-box-mixin.js#L279

The logic used while setting the value downward to the internal inputElement differs from input-mixin...

      _forwardInputValue(value) {
        // Value might be set before an input element is initialized.
        // This case should be handled separately by a component that
        // implements this mixin, for example in `connectedCallback`.
        if (!this.inputElement) {
          return;
        }

        if (value != null) {
          this.inputElement.value = value;
        } else {
          this.inputElement.value = '';
        }
      }

• https://github.com/vaadin/web-components/blob/23.3/packages/field-base/src/input-mixin.js#LL111-L124C8

Note that the value != null condition will fall into the else whenever the value is null or undefined.

The proposed fix for v23.x is to change combo-box-mixin...

     set _inputElementValue(value) {
       if (this.inputElement) {
-        this.inputElement[this._propertyForValue] = value;
+        this.inputElement[this._propertyForValue] = value != null ? value : '';
       }
     }

Thoughts?

[vursen]

Hey @robrez! Thanks for your report.

The combo-box component doesn't officially support null and undefined as a value. It only accepts string values, so the right way would be to use an empty string when you need to reset the value. Otherwise, I would ask you to clarify the use case a bit more and why it cannot be accomplished with the current API.

[robrez]

Sure, here is how I noticed this. I have some code that looks like this..

class MyThing extends LitElement {
  @property() value: string | undefined | null;
  @property() items: string[] = [ 'a', 'b', 'c' ];
  
  render(): TemplateResult {
    return html`<vaadin-combo-box allow-custom-value .items=${this.items} .value=${this.value} />`
  }
}

I recently upgraded from vaadin 14 (really combo-box ^5.x.x) to >=23.x.x. combo-box started rendering undefined where it previously rendered "" for the initial state (value is undefined).

The initial state of undefined was obvious, but it is also common for various devs to set this.value = null or this.value = undefined when clearing/resetting a form, for example. At scale this will be challenging to track down

All of that works fine w/o allow-custom-value because of the internal coercion to '' for invalid values. Here are stackblitzs

• v5.x.x
• v24.x.x

[vursen]

We had a meeting internally that we concluded with the following proposal:

Only strings are valid values for the value property. That said, we'd like the field components to automatically convert null and undefined to an empty string to facilitate migration from older versions.

When passing null or undefined as a value to a field component, a warning will be shown, advising against using non-string values and informing that the automatic conversion results in one extra value-changed event.

[web-padawan]

Only strings are valid values for the value property. That said, we'd like the field components to automatically convert null and undefined to an empty string to facilitate migration from older versions.

This is also how I see it. IIRC, even with Polymer this could be achieved using getter / setter pair. Lit provides much more flexible API that is well suitable for handling cases such as this one.

When passing null or undefined as a value to a field component, a warning will be shown, advising against using non-string values and informing that the automatic conversion results in one extra value-changed event.

Let me disagree. While warnings make a lot of sense when it comes to Vaadin-specific concepts such as dataProvider, it sounds redundant in case of value property, especially as native HTML elements do not produce any warnings.

I will try to see if we can mitigate extra value-changed event using Polymer API.

[nioe]

It has been a year already since the last comment. Any updates?