Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiFieldNumber] Invalid sign disappear for non-integer value after 7 digits #7180

Closed
dej611 opened this issue Sep 12, 2023 · 9 comments
Closed
Assignees
Labels
answered Issues answered by the EUI team - auto-closes open issues in 7 days if no follow-up response

Comments

@dej611
Copy link
Contributor

dej611 commented Sep 12, 2023

Describe the bug
If the EuiNumberField is configured with step=1 then the integer validation works up to 7 digits after the dot.
From the 8th digit onward the value becomes valid again 💥

Environment and versions

  • EUI version: latest
  • React version:
  • Kibana version (if applicable):
  • Browser: Chrome, Safari
  • Operating System: Mac

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/goofy-frog-5zxhh7
  2. Type 95.99 and the error should appear.
  3. Now type more 9s to reach 95.99999999
  4. The invalid icon is gone 💥

Expected behavior
The invalid icon remains.

Screenshots
digits_number_bug

@dej611 dej611 added bug ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Sep 12, 2023
@cee-chen

This comment was marked as off-topic.

@breehall breehall removed bug ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Sep 12, 2023
@dej611
Copy link
Contributor Author

dej611 commented Sep 12, 2023

But the problem is not on blur, in the GIF above I keep on typing 9 there and the invalid styling+markup disappears.
digits_number_bug

In your new example the styling stays on red while I'm typing:
Screenshot 2023-09-12 at 16 44 32

To me looks like a bug.

@cee-chen
Copy link
Member

cee-chen commented Sep 12, 2023

It's the number you're using, 95.99999999 does the same thing on the native input, but 1.222222222 doesn't:

screencap

The reason for this is sorta documented by MDN: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#step

Note: When the data entered by the user doesn't adhere to the stepping configuration, the user agent may round to the nearest valid value, preferring numbers in the positive direction when there are two equally close options.

So I think what Chrome/Safari is basically doing is saying "good enough this is an integer" and rounding up which makes it satisfy the whole number step requirement.

Also fun fact this behavior happens on webkit browsers but not Firefox so I was real confused for a second there trying to repro what you were describing 💀

@dej611
Copy link
Contributor Author

dej611 commented Sep 12, 2023

Ah, at first I recalled some math theory about Cantor and decimal numbers which can get approximate to integers but then I thought it wasn't that. Apparently browsers decided to have a similar approach! TIL

Thanks for looking into it @cee-chen !

@cee-chen
Copy link
Member

No problem! Just to make sure I'm on the same page as you, would you be for/against EUI smoothing over default browser step behavior?

@dej611
Copy link
Contributor Author

dej611 commented Sep 12, 2023

It would be nice to handle this, but I understand that it might be a non-trivial task. At least I have a reference now to pass over in case somebody logs an issue about that. 👍

@cee-chen
Copy link
Member

IMO, it's fairly low effort to do! Should be a basic if step is passed and is a number->onChange->check if value % step === 0 and manually set an invalid state if not.

@cee-chen cee-chen changed the title [EuiNumberField] Invalid sign disappear for non-integer value after 7 digits [EuiNumberField] Improve native browser step validation Sep 12, 2023
@cee-chen cee-chen changed the title [EuiNumberField] Improve native browser step validation [EuiFieldNumber] Improve native browser step validation Sep 12, 2023
@cee-chen cee-chen self-assigned this Sep 12, 2023
@cee-chen
Copy link
Member

cee-chen commented Sep 19, 2023

IMO, it's fairly low effort to do!

Famous last words - this was mostly trivial up until I started edge case testing (i.e., instantiating an input with an initial value/defaultValue that can't be cleanly divided by step). When that happens because the internal step gets set and can't be unset afterwards, and there's nothing I can do to override the mismatched validity state at that point (short of unmounting and remounting the entire component) because validity.stepMismatch is read-only. I opted to handle that edge casing by console warning and falling back to default browser behavior for that scenario.

In any case, new working behavior should be in #7202 / https://eui.elastic.co/pr_7202/#/forms/form-controls#number-field if you'd like to test via the playground flyout!

@cee-chen
Copy link
Member

cee-chen commented Sep 19, 2023

🤦 So, I definitely should have anticipated this happening, but it turns out all of this was for nothing lol, because even with the updated logic, with enough 9's (e.g. 1.99999999999999999999999) the number is valid in Chrome (but invalid on Firefox 💀 )

Pretty sure this is just related to the JS language itself rounding integers (e.g. try pasting 1.99999999999999999999999 in the console log), and there's not a whole lot we can do about that.

@dej611 I think I'm just going to close out my open PR - I don't love the idea of adding that much extra code for something that doesn't really work. If someone does log an issue for this, please feel free to point them at this thread. I'm going to do a little housecleaning on this issue to make it a little clearer what our decision was.

@cee-chen cee-chen changed the title [EuiFieldNumber] Improve native browser step validation [EuiFieldNumber] Invalid sign disappear for non-integer value after 7 digits Sep 19, 2023
@cee-chen cee-chen added answered Issues answered by the EUI team - auto-closes open issues in 7 days if no follow-up response and removed feature request labels Sep 19, 2023
@cee-chen cee-chen closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered Issues answered by the EUI team - auto-closes open issues in 7 days if no follow-up response
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants