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

fix(VNumberInput): avoid showing NaN #19913

Closed
wants to merge 1 commit into from
Closed

Conversation

J-Sek
Copy link
Contributor

@J-Sek J-Sek commented May 27, 2024

Description

In order to avoid NaN we need to decouple "text" model for VTextField and the "exposed" model representing value for VNumberInput's consumer. I don't see it any other way and this approach served me many years in my custom wrapper arount VTextField back in Vuetify 2, so I am pretty sure it's the only way forward.

Each time we sync between these two

  • "text" model becomes valid number or an empty string – thanks to typeof model.value === 'number' && !isNaN(...)
  • "exposed" model becomes valid number – thanks to custom aggressive parsing in extractNumber(...)

Note: there are many ways to break this implementation, so we will need a bunch of test cases as a safety net for future changes. I plan on introducing them in the following days.

fixes #19798

Markup:

<template>
  <v-app theme="dark">
    <v-container>
      <v-card class="mx-auto pa-6 my-4" style="max-width: 1200px">
        <v-row>
          <v-col cols="4">
            <small class="d-block mb-2">(clearable)</small>
            <v-number-input v-model="emptyValue" clearable />
            <code>value: {{ emptyValue }}</code>
          </v-col>
          <v-col cols="4">
            <small class="d-block mb-2">(:step=".25")</small>
            <v-number-input v-model="value1" :step=".25" />
            <code>value: {{ value1 }}</code>
          </v-col>
          <v-col cols="4">
            <small class="d-block mb-2">(:max="20" :min="-20")</small>
            <v-number-input v-model="value2" :max="20" :min="-20" />
            <div class="d-flex align-center justify-space-between">
              <code>value: {{ value2 }}</code>
              <div class="text-right">
                <v-btn @click="value2 = 5.125">Set to 5.125</v-btn>
                <v-btn @click="value2 = NaN">Set to NaN</v-btn>
              </div>
            </div>
          </v-col>
        </v-row>
      </v-card>
    </v-container>
  </v-app>
</template>

<script setup lang="ts">
  import { ref } from 'vue'

  const emptyValue = ref<number>(null)
  const value1 = ref<number>(15)
  const value2 = ref<number>(-10.25)
</script>

@MajesticPotatoe MajesticPotatoe added T: bug Functionality that does not work as intended/expected labs C: VNumberInput labels May 28, 2024
@J-Sek
Copy link
Contributor Author

J-Sek commented Jun 1, 2024

While writing tests I have noticed many edge cases related to .toString() producing totally different text that was used to parse the number. Initially I just wanted to avoid situation when component "misbehaves" in a quite critical manner - by interrupting user when he is in the middle of typing. But I learned about handful of other scenarios, most of which apply to pasting text from clipboard and raw typing.

Explanation of scenarios (I am not convinced E2E tests are self-explanatory)

  • typing - is immediately replaced with NaN (but it later works when you clear text and try again)
  • typing -0 is immediately replaced with 0 (I wanted to type -0.2 and I am basically unable to do it)
  • typing 0.0000001 is immediately replaced with 1e-7 and it only gets worse the higher the precision
  • once the mentioned problems were fixed I had to be very careful and disallow typing when one more digit would result in rounding the whole value (e.g. from 3.99... to 4) as I think it is not the intention of the user. He can still move cursor and type digits earlier, however effective digits are cut off as we prioritize digits from the left. I am OK with this tradeoff although it would be the best to display validation error stating that the field gets more than it can handle.
    • the core of this functionality is a little buried inside extractNumber(...). This function now removes digits until the condition cleanText !== Number(cleanText).toFixed(decimalDigits) is satisfied.

reasoning: accepting all digits them would affect user's input due to JS number format limitation. I would rather cut the text instead of replacing whole input with something unexpected.

The issue is related to precision. I think we would benefit from strict precision prop (i.e. fractional digits limit) that would be 0 by default. Feature request is already open #19898.

Along the way I was forced to rewrite getDecimals(...). As mentioned .toString() produces exponential notation after some point. New implementation just counts digits after the dot (if any are present) and includes value after e. I don't love the internal naming and lack of comments inside, but opted for brevity until someone complaints.

@J-Sek J-Sek marked this pull request as ready for review June 1, 2024 05:13
@johnleider johnleider requested a review from yuwu9145 June 4, 2024 20:56
Copy link
Member

@yuwu9145 yuwu9145 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did an initial testing, when typing "-" the first time, it's ignored, but when keep typing the second time, it shows up.

Also, could you provide a simplified example that demonstrate the reason to justify introducing the "text" model for VTextField? I'm confused what's the problem it's trying to resolve, so far, transforming number input's v-model based on VTextField's onUpdate:modelValue still feels enough.

Fix should be in onModelUpdate where you could properly transform what's being typed from VTextField to the correct number

@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 6, 2024

Did an initial testing, when typing "-" the first time, it's ignored, but when keep typing the second time, it shows up.

Could you share the code you've used? I cannot reproduce this behavior with the simplest case I can think of:

// value = ref(null)
<v-number-input v-model="value" />

Input behaves correctly on Chrome and Firefox (on Linux). And there are test cases in the typing values group that should probably fail if such bug exists.


Also, could you provide a simplified example that demonstrate the reason to justify introducing the "text" model for VTextField?

  1. Type 0.001, Backspace - results in 0
    • intention was to replace 1 with non-zero character, but user is interrupted and forced to type .00 again
    • if there is a single decimal digit, replacement is interrupted as well but in more subtle way which might be even more frustrating
  2. Type or paste 0.0000001 - results in 1e-7

There are only 2 ways to solve it that I see:

  • defer onModelUpdate until the field is blurred
    • although it would not solve exponential notation
    • could introduce inconsistency into validation experience
  • or split text model from exposed numeric model and synchronize them as suggested in this PR

@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 8, 2024

There is one more argument for the internal model split - we could support grouping and precision enforcement. I have already implemented this on top of this PR, so it is blocking me from posting PR for #19898. I hope we could deliver these fixes and functionalities before this component gets out of Labs :)

@johnleider johnleider requested a review from yuwu9145 July 9, 2024 16:04
@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 17, 2024

Hey @yuwu9145, how can I help push this PR forward? This component will soon be out of labs and I think it would not look good if we let it show "NaN" or ship suboptimal UX (i.e. interruptions while typing).

@yuwu9145
Copy link
Member

Hey @yuwu9145, how can I help push this PR forward? This component will soon be out of labs and I think it would not look good if we let it show "NaN" or ship suboptimal UX (i.e. interruptions while typing).

I'll be available to check it again tomorrow

Copy link
Member

@yuwu9145 yuwu9145 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+/- buttons become useless when displaying exponent number.

Instead, numberInput should only support certain max/min/fractionDigits ranges without converting to exponent format or loosing precision. If users want to use number in advanced level (e.g. bigInt), they should use string with v-text-field where advanced numbers can be processed by third-party libraries (e.g. decimal.js). This can be shown in the doc once we figure out what are the exact ranges it supports.

Screenshot 2024-07-18 at 1 40 08 pm

NumberInput should be kept simple and focus on BASIC NUMERICAL INPUTS:

  • Only allow "numbers", "-" , "."
  • "-", "." are only allowed once, AND "-" only allowed at the start

@johnleider could you confirm if this was the right direction for numberInput?

I think solution to resolve NaN issue should be around onModelUpdate

Internal model split opens the door for more advanced usages which I personally don't think suitable in number input

@J-Sek J-Sek force-pushed the fix-19798 branch 2 times, most recently from 2c10687 to 5e0eb0d Compare July 18, 2024 13:35
@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 18, 2024

I think solution to resolve NaN issue should be around onModelUpdate

I am trying to follow... we need to suppress emits that lead to NaN auto-replacing value when we anticipate user to continue typing.

Edit: previously listed downsides are not relevant

All the relevant E2E tests still pass.

@J-Sek J-Sek force-pushed the fix-19798 branch 4 times, most recently from f1b5f08 to 7069595 Compare July 18, 2024 18:33
@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 18, 2024

@yuwu9145, I have polished the PR a little after following your guide. I don't necessarily like the fact we are closing the doors for grouping separators and formatting locales, but maybe it's better to ship a scalper than swiss army knife (for the first version out of labs).

{ text: '0.000010', expected: '0.00001' },
{ text: '0.012340', expected: '0.01234' },
{ text: '099999990.000000010', expected: '99999990.00000001' },
{ text: '99999999999999999999999999999', expected: '999999999999999' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 99999999999999999999999999999 becomes 999999999999999 ?

Copy link
Member

@yuwu9145 yuwu9145 Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS number has a quirk:

Precision loss when it's beyond MAX_SAFE_INTEGER

Screenshot 2024-07-19 at 11 48 20 pm

Perhaps our number input should only support between Number.MIN_SAFE_INTEGER and Number.MAX_SAFE_INTEGER

Copy link
Contributor Author

@J-Sek J-Sek Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the end goal is the same. We want to respect whatever user typed and avoid replacing already typed characters because of JS quirks. Using integer limit/range with inputs (that can be float values) seems like something that will leave tons of scenarios uncovered.

Suggested implementation shortens input string (removing characters from the end) until condition is satisfied

cleanText !== Number(cleanText).toFixed(decimalDigits)

Original question was about a very long number. You can paste or type to any example from documentation. Pasted it becomes 1e+29, typed... well breaks in more spectacular way and can lead to NaN, hence test case should be a great fit.

Copy link
Member

@yuwu9145 yuwu9145 Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using integer limit/range with inputs (that can be float values) seems like something that will leave tons of scenarios uncovered.

The primary purpose of number-input is BASIC NUMERICAL INPUTS, for example, money/weight etc. It is designed to AVOID JS number quirks rather than RESOLVE all precision & unsafe JS quirks.

As per my understanding, there are three number challenges:

  • Unsafe integer (9007199254740993 -> 9007199254740992) - use MAX_SAFE_INTEGER & MIN_SAFE_INTEGER range
  • JS floating error (0.1 + 0.2 === 0.30000000000000004 ) - almost impossible to resolve perfectly, so we make it clear in doc that we use toFixed(Math.max(modelDecimals, stepDecimals)) to cope with it internally
  • Exponent format - not supported because they invalidate +/- buttons

These would cover the most of the use cases.

If users want to deal with crazy numbers (bigInt or long decimals), they should not use number-input but operate on STRING in text-field. These cases do not require +/- buttons anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always considered my implementation as a way to avoid JS quirks rather than resolving them :)

Scenario: For whatever reason user types (or pastes) 555555... (very long string of digits). What would we like to show:

  • NaN --- exposing JS quirk
  • 9007199254740991 (MAX_SAFE_INTEGER) --- exposing JS quirk
  • 5555555555555555 (digits from user, but only up until they can be displayed)

I am leaning towards the last and I still did not hear any argument against it. In my opinion it is not contrary to any of your points.

Imagine a developer that just placed v-number-input in the form and never expected end-user to have a "heavy-finger" or a slightly broken keyboard (I am used to both). But in some of the scenarios, he might unnecessarily surprise end-user. I think the unsurprising result is the best outcome and it it's implementation is quite unsophisticated.

I also don't want to fight over it. It is much more important to close this PR and proceed to strict precision implementation. If you feel like Math.max(Math.min(Number(cleanText), Number.MAX_SAFE_INTEGER), Number.MIN_SAFE_INTEGER) should replace the while-loop, I won't have anything against adjusting the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_SAFE_INTEGER will be used as the max prop.max, so any input bigger than it will be restricted/changed to MAX_SAFE_INTEGER.

describe('typing values', () => {
it('should ignore invalid and duplicated characters', () => {
const scenarios = [
{ text: '+=1234... abc e12', expected: '1234.12' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid input should be enforced to empty undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted a test case for number extraction, but this example does not convey that we wish to accept numbers copy/pasted with any format (as long as decimal separator is dot). I will update this part.

@johnleider
Copy link
Member

@johnleider could you confirm if this was the right direction for numberInput?

Confirmed.

@yuwu9145
Copy link
Member

yuwu9145 commented Jul 22, 2024

Thanks for your time, but it isn't heading to the right direction:

JS built-in numbers do have quirks, but what we should do is clearly figure out a safe range they can work within and document floating limitations to prevent confusing users. MAX_SAFE_INTEGER is large enough to allow the number-input to serve basic numeric uses, Any number beyond MAX_SAFE_INTEGER isn't supported by the number input.

Therefore, there is no value in having extractNumber that performs custom string-to-number conversion. Additionally, a new text model isn't necessary as the beforeinput event works well for preprocessing and pre-validating input.

I'm going to push forward #20211 for your reference

@yuwu9145 yuwu9145 closed this Jul 22, 2024
@J-Sek
Copy link
Contributor Author

J-Sek commented Jul 22, 2024

MAX_SAFE_INTEGER is large enough to allow the number-input to serve basic numeric uses, isn't supported by the number input.

After playing a bit with MAX_SAFE_INTEGER I think you will get have a hard time making it work with floats. The more fractional digits, the less decimal digits you can use to reliably display the value.

Fraction digits Example with issues Decimal part limit
0 9_999_999_999_999_999 » 10000000000000000 15
1 1_999_999_999_999_999.1 » 1999999999999999 15
2 99_999_999_999_999.01 » 99999999999999.02 13
3 9_999_999_999_999.001 » 9999999999999.002 12
4 1_999_999_999_999.0001 » 1999999999999 12

MAX_SAFE_INTEGER has 16 digits, so it becomes useless once there are any floats. It will be better in preventing NaN than Infinity, but that's basically all we can say about it. Which is good enough to resolve the issue :)

I think the docs could include something like "this field supports numbers up to 14 digits long. Use explicit min/max and precision to avoid JS-related issues".

@J-Sek J-Sek deleted the fix-19798 branch July 30, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VNumberInput labs T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report][3.6.5] (VNumberInput): interpretes negative numbers as NaN
4 participants