null initial values break number inputs

[vsgoulart]

Describe the Bug

When initializing input of type numbers with the value null, these inputs show a NaN error. Value null should be accepted since they are valid output of a completed form.

Steps to Reproduce

1. Render a form with a input type number with initial value null
2. The form is rendered with an error in the number input (like in the image below)

Expected Behavior

The input should show an empty field (since null is a valid output on form submissions)

Additional information

[Skaiir]

@vsgoulart What version was this on? Can't replicate on master atm. Maybe a tasklist only issue? Maybe something fixed by accident.

[vsgoulart]

@vsgoulart What version was this on? Can't replicate on master atm. Maybe a tasklist only issue? Maybe something fixed by accident.

@Skaiir There's a problem with the issue the description, but the root cause of this is indeed on Tasklist (to reproduce we need to pass the value "null" and not a pure null).

But investigating this highlights another potential issue on form-js which is that the initial validation is inconsistent across components like in the image below (some things are validated on the first render and some aren't):

[Skaiir]

Well so the way this works, there's first and sanitation that runs on every field on start, and then there's validation. Number has also a special validation that checks for NaN. Yeah it's inconsistent, I think we should always just sanitize and report that sanitation happened. But as of now, we don't have any mechanism for reporting that, so we would just sanitize and forget about "NaN" entirely. Also text null should not parse to "null".

@christian-konrad does that make sense to you? We have a ticket tracking sanitation reporting somewhere that we can get to some other time.

[Skaiir]

Just wanted to be thorough here, I checked all the conversion behaviors

I'm thinking we get rid of NaN, it's a very unique way of doing things compared to all the other components / types. Just sanitation should be enough. It may still make sense to display NaN in the number field when pasting in text into it, but then again there are better ways of doing this, like short toast notifications or something, and we don't know how frequent of a problem that is. I don't know, this can be discussed.

Null for text entry components should sanitize to "", not "null".

Then there's a couple things I feel like may be implicitly converted for checkboxes. But maybe keeping it bool only is the way to go. Then again that's a bit of the opposite approach to text components where we convert to string pretty much every time.

Any thoughts?

[pinussilvestrus]

Thanks for sharing the table, I like that we tackle it!

I agree that we should avoid mixtures of sanitized values as much as possible, and should be consistent across the components (cf. no surprises. That means I agree with removing "NaN" and sanitize to null here 👍

The same should go for text fields, that we sanitize to "" which equalizes the empty value for strings. Also for input data like null. I have the feeling we should also do the same for Boolean values into text fields, this is a wrong data type and we should not try to magically fix this. Same goes for check boxes, wrong data type follows to an empty, sanitized value.

[vsgoulart]

Also text null should not parse to "null".

Yeah, this was the Tasklist issue

Yeah, I also agree with should avoid NaN

The same should go for text fields, that we sanitize to "" which equalizes the empty value for strings. Also for input data like null. I have the feeling we should also do the same for Boolean values into text fields, this is a wrong data type and we should not try to magically fix this. Same goes for check boxes, wrong data type follows to an empty, sanitized value.

👍 I also agree

[Skaiir]

Mhhh okay, I think it makes sense. The only issue with text fields is that we have it documented that types will be converted to string. So it would be a breaking change.

[pinussilvestrus]

In that case let's wait for @christian-konrad and make it a conscious decision.

[Skaiir]

The obvious tasks from this issue have been covered, we're awaiting some feedback for what we want to do with the textfield and textarea components however, hence not yet closed.

[marcosgvieira]

@Skaiir can we move this to "Done" or is it still waiting for something?

[Skaiir]

Yeah sure, I like the state that it's at let's not overthink it.