Number field fires spurious value change events when assigning a value that cannot be parsed

[Legioth]

Description

If I assign a value that cannot be parsed as a number to the number field, then then the actual is set as the empty string. In the process, the field fires two value-changed events where event.detail.value is the empty string. This happens even if the field was previously empty which means that there's no user-observable change to the field.

This is particularly problematic when binding the field to a state variable typed as number. A trivial implementation would use parseInt(event.detail.value) which for an empty input gives NaN as the value. This is in itself a bit weird but still reasonable. The problem comes when assigning the value back to the field as '' + value since that assigns 'NaN' as the value and this triggers those spurious events. This might in turn lead to an infinite loop if the value change event is in turn used to update the state in a way that doesn't take into account that NaN == NaN evaluates to false in JavaScript (e.g. with a Preact signal: preactjs/signals#614).

Expected outcome

No event should fired since the effective value is '' both before and after the change.

I could also understand if the first event would have the assigned unparseable string as event.detail.value and then there would be a second value for changing back to '' but that would not be practical since it leads to even worse event loops.

Minimal reproducible example

https://vaadin.com/docs/latest/components/number-field

Steps to reproduce

1. Open https://vaadin.com/docs/latest/components/number-field
2. Select any number field from the page in the browser's inspector (be careful to select the web component and not the <input>)
3. Add an event listener through the JS console: $0.addEventListener('value-changed', event => console.log(event.detail.value))
4. Change the value to any unparseable string: $0.value = "NaN"
5. Note the two events logged to the console

Environment

Vaadin version(s): 24.6.0-alpha2

Browsers

No response

[vursen]

Related #4184

[TatuLund]

A bit more distant cousin here: vaadin/hilla#2185

[rolfsmeds]

@vursen, could this be prevented without doing #4184?

[vursen]

could this be prevented without doing #4184?

Yes, seems to be doable by moving the type conversion logic from the value observer to the value setter:

_valueChanged(newVal, oldVal) {
-  // Validate value to be numeric
-  if (newVal && isNaN(parseFloat(newVal))) {
-    this.value = '';
-  } else if (typeof this.value !== 'string') {
-    this.value = String(this.value);
-  }

  super._valueChanged(this.value, oldVal);
}

+ get value() {
+  return super.value;
+ }

+ set value(value) {
+   if (isNaN(parseFloat(value))) {
+     super.value = '';
+   } else if (typeof value !== 'string') {
+     super.value = String(value);
+  } else {
+     super.value = value;
+  }
+ }

[vursen]

Though, there is still a question of whether we should show a warning when there is an attempt to set an unparsable value. Right now, it's inconsistent: integer-field shows a warning while number-field doesn't.

web-components/packages/number-field/src/vaadin-number-field-mixin.js

Lines 367 to 373 in 78967d4

	_valueChanged(newVal, oldVal) {
	// Validate value to be numeric
	if (newVal && isNaN(parseFloat(newVal))) {
	this.value = '';
	} else if (typeof this.value !== 'string') {
	this.value = String(this.value);
	}

web-components/packages/integer-field/src/vaadin-integer-field.js

Lines 78 to 83 in 78967d4

	_valueChanged(newVal, oldVal) {
	if (newVal !== '' && !this.__isInteger(newVal)) {
	console.warn(`Trying to set non-integer value "${newVal}" to <vaadin-integer-field>. Clearing the value.`);
	this.value = '';
	return;
	}

I would personally prefer seeing a warning, but I got the impression that setting 'NaN' is planned to be used as a feature.

[Legioth]

My use of 'NaN' was accidental since I wasn't taking into account that value might not always be an actual number. Logging a warning would have been acceptable and would have made it easier for me to understand the mistake.