vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
620 stars 167 forks source link

Ignore white space when evaluating `isEmpty()` for String-typed `HasValue` implementations #19847

Open Legioth opened 2 months ago

Legioth commented 2 months ago

Describe your motivation

The default isEmpty() implementation performs an equality check against getEmptyValue() which is "" for typical field implementations with a string value. The means that a required field can be satisfied by typing a single space. This is almost universally not the desired outcome of marking a textual field as required.

Describe the solution you'd like

We should make any string-typed field considered as empty if it only contains whitespace characters. I suggest using Character.isWhitespace(int) but also explicitly rejecting the three non-breaking space code points mentioned in the documentation of isWhitespace (\u00A0, \u2007 and \u202F) since I assume many users try using them to bypass the validation.

This could be implemented directly in the default implementation of HasValue::isEmpty so that it it uses different logic if getEmptyValue() is "" and getValue() is a String.

Describe alternatives you've considered

Changing this might lead to a breaking change in some special cases so we might want to wait with this change until a major release or alternatively make it configurable in some way that is still more convenient than using the asRequired(Validator) overload for each binding.

guttormvik2 commented 2 months ago

As a developer, I want to have control. As a user, I want to get stuff done. I want the system to prevent me from doing mistakes, like forgetting to provide a required value, but I don't want the system to get in my way. If I think the field should not be required, I will enter some junk. If whitespace is blocked, I'll just enter "." instead, or "-" or "NA" etc. There is little you can do to stop me. You can only annoy me.

So, I think the current behavior is the right one for "required" as such.

However.. I have a related suggestion: Let me turn on trimming of all text fields, not just required ones. Then you should trim all whitespace, and together with "required" you'd get the result you advocate, but for a different and less user-hostile reason.

Btw: Displaying data should never change it, so this trimming should only be done when user changes the value. If, for some reason, the data from the database has trailing spaces, they should be left alone.

Legioth commented 2 months ago

I want the system to prevent me from doing mistakes, like forgetting to provide a required value, but I don't want the system to get in my way.

I would say that regular spaces (or newlines in a text area) might still be an accident if you initially typed something into a field and then moved the text to some other place and accidentally omitted a trailing invisible character. But I do also agree that anything beyond that is not meaningful for forcing the user to provide some input since they can just type in some actual text that just doesn't carry any meaning. There might be some security reasons for why various whitespace characters should be avoided but those cases are probably sensitive to those characters in any location and not only as the only characters in the value.

However.. I have a related suggestion: Let me turn on trimming of all text fields, not just required ones.

I can see the benefit from automatic trimming but I'm not sure exactly where it should be applied. As you point out, it should preserve whitespace that comes from the database but there are also some aspects to how whitespace from the user would be trimmed e.g. to avoid moving around the cursor while the field is focused. Is anyone aware of some specific example out there that handles this well?

guttormvik2 commented 2 months ago

avoid moving around the cursor while the field is focused

Didn't follow that. Are you thinking about newlines? "\nText" initially displaying as "", and "Text" appearing when user presses down or right arrow? If so, I don't necessarily see that as a problem; It is reflecting the actual data.

knoobie commented 2 months ago

While those are the "actual" data - this is normally done by accident and creates problems in systems down the line. For example the most promient problem comes from copy pasting something from Excel into a web page.. sometimes you get trailing spaces, tabs or new lines additionally added. All those characters make operations on the submitted data way harder (database searches for example) - therefore, we have a strict policy for allowed character and enforce them when filling out our forms to ensure all systems that consume our data can do it safely.

Legioth commented 2 months ago

I'm thinking of the value in the field being Text and the cursor at index 3 i.e. after the T. After trimming, that index would put the cursor after the x instead which might be confusing if it happens while typing.

guttormvik2 commented 2 months ago

Legioth:

might be confusing if it happens while typing

I was thinking about trimming on blur, and/or on valueChanged on the server-side but if you want eager submits then that is a concern. On the other hand, this is supposed to be an exception, so maybe it is acceptable to confuse a little bit?

Wait a minute.. You can't trim while writing anyway. You have to be able to type space!

knoobie:

While those are the "actual" data - this is normally done by accident and creates problems in systems down the line

Agreed. We want clean data, but we don't always have that luxury. My concern is with old data, either imported from a legacy system, or from before we introduced trimming. I want those data to continue to work, and I don't want a user that only views the data to be asked "Do you want to save?" (or auto-save, and then later be asked "why did you change this record?)

One fun little wrinkle: That text field might be part of a a key or unique constraint. Say the user opens up such a record, does one proper change and the system auto-trims some other value. On save the user might get exceptions from the DB that he or she can't do anything about, and are thus prevented from doing the proper change as well.