danielo515 / obsidian-modal-form

Define forms for filling data that you will be able to open from anywhere you can run JS
https://danielorodriguez.com/obsidian-modal-form/
MIT License
199 stars 17 forks source link

[BUG] Modal verification of number type incorrectly flags decimals #186

Closed andrew-m-j closed 3 months ago

andrew-m-j commented 8 months ago

Describe the bug When entering data in a form for a number type item, the entry suggests a validation error for decimal entry numbers by highlighting the entry box in red. However, the data seems to be parsed correctly to the results still, with the decimal number stored.

To Reproduce Steps to reproduce the bug:

  1. Enter a decimal number on a number type field (e.g, "123.456").
  2. Entry box is highlighted in red, suggesting an error.
  3. Submitting the form and accessing the results indicate that the decimal is stored correctly.

Expected behavior

Form validation of the number type should not flag a decimal as an error. The text entry already seems to prevent characters from being entered. Other fringe cases that might need to be handled are if there are two decimals entered, which is an error (e.g., "123.456.789"), or for comma entry instead of a decimal as in some countries.

fetwar commented 5 months ago

+1

This bug is preventing decimals from being input for me in the same manner. I am able to reproduce the bug by following the instructions from @andrew-m-j. With the same result

Having a very brief look at the code though... I can't actually find where the validators for Number are.

Do you mind pointing me in the right direction for this one and I will attempt a fix @danielo515

danielo515 commented 5 months ago

There is no number validation, that is why you don't see any @fetwar If the form were validating the value, and you input an invalid value, you will not be able to submit the form, but, if you try, you will see you will be able to submit the form without problem. My guess is that this is either a problem with obsidian itself, or a problem in a theme that is incorrectly applying the wrong styles. I just tested any random obsidian input, changed it to be of type number and then it exhibits the same behaviour. So this is either a but in Obsidian core or a theme.

fetwar commented 5 months ago

Hmmm, sounds like it might be a css style that is being apply for generic field "Number".

Could this perhaps be changed to a custom field type?

I will have a look at the CSS styles being applied and see if we find anything

fetwar commented 5 months ago

CSS styles applied on app.css:

input[type='number'] {
  -webkit-app-region: no-drag;
  background: var(--background-modifier-form-field);
  border: var(--input-border-width) solid var(--background-modifier-border);
  color: var(--text-normal);
  font-family: inherit;
  padding: var(--size-4-1) var(--size-4-2);
  font-size: var(--font-ui-small);
  border-radius: var(--input-radius);
  outline: none;
}

These are consistent whether in error state or not, ~the var values themselves are changing~ - No they aren't lol, new styles are being applied that I apparently missed

fetwar commented 5 months ago

The color and border-color attributes are being applied by the css classes here:

https://github.com/danielo515/obsidian-modal-form/blob/9ac9049121bbd0419dee9edff8b5ff9fe7f4b19b/styles.css#L77-L81

Which I am guessing means Obsidian is applying the "invalid" attribute as you said.

What are your thoughts on the idea of a non-number input or only showing the error for this css class if modal-form-invalid is applied to specifically number?

Those are both workarounds though

danielo515 commented 5 months ago

What are your thoughts on the idea of a non-number input or only showing the error for this css class if modal-form-invalid is applied to specifically number?

The first one is a no-go. I started this project with the idea of using the standard Obsidian inputs as much as possible, and I also like to stick to standard html inputs for accessibility reasons. The second one, may be a viable workaround, but I would like to see what the obsidian team has to say if we report this error before we do anything on our side

danielo515 commented 5 months ago

https://forum.obsidian.md/t/inputs-of-type-number-are-being-flagged-as-invalid-if-they-contain-decimals/80235

danielo515 commented 5 months ago

Interestingly, this is standard HTML behaviour: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#allowing_decimal_values So the workaround is simple, add a step of 0.001

fetwar commented 5 months ago

I 100% agree, it seems odd that standard "number" following HTML standards is in an "invalid" state. Integer sure, number definitely not

fetwar commented 5 months ago

Sounds like a great workaround, I appreciate your investigations on this

fetwar commented 5 months ago

I think a step of any would be a more reasonable step to not unnecessarily constrain users who may want smaller decimal granularity.

From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#step

A string value of any means that no stepping is implied, and any value is allowed (barring other constraints, such as min and max).

danielo515 commented 5 months ago

The problem with that is that, as you pointed out, it wipes min and max constraints. So at least, as a minimum, a mix and max should be checked for existence.

On Mon, Apr 15, 2024 at 1:34 AM Marc @.***> wrote:

I think a step of any would be a more reasonable step to not unnecessarily constrain users who may want smaller decimal granularity.

From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#step

A string value of any means that no stepping is implied, and any value is allowed (barring other constraints, such as min https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#min and max https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#max ).

— Reply to this email directly, view it on GitHub https://github.com/danielo515/obsidian-modal-form/issues/186#issuecomment-2054221482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARKJWN6F344I4OWCL2JWGTY5MG75AVCNFSM6AAAAABBG2XZEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGIZDCNBYGI . You are receiving this because you were mentioned.Message ID: @.***>

--

https://danielorodriguez.com

fetwar commented 4 months ago

The wording around min and max constraints is a bit unclear, but validation of other constraints such as min, max and required are still applied.

I have created this codepen for testing the validation, and I can't see any issue with the form input working in this way

fetwar commented 4 months ago

PR opened

danielo515 commented 4 months ago

Thank you for the testing @fetwar ! that was very useful! Will check the PR