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(input-number): allow editing of numbers that start with zeros #11705

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

josercarcamo
Copy link
Contributor

Related Issue: #9611

Summary

Allows editing of numbers that start with zeros

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Mar 6, 2025
@josercarcamo josercarcamo added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 6, 2025
@josercarcamo josercarcamo requested a review from benelan March 6, 2025 20:46
@josercarcamo
Copy link
Contributor Author

@benelan could you please review?

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

There are some cases that don't work with this regex solution, can you take another look? Please reach out if you have any questions or if you'd like some implementation ideas!

hasTrailingDecimalSeparator && isValueDeleted
? `${newLocalizedValue}${numberStringFormatter.decimal}`
: newLocalizedValue;
const re = new RegExp(`^-?0+\\d*${numberStringFormatter.decimal}?$`);
Copy link
Member

Choose a reason for hiding this comment

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

There are some issues with this regex:

  • The - won't work in some locales, you can use numberStringFormatter.minusSign instead
  • The 0 won't work for other numbering systems
  • The \d won't work for other numbering systems
  • There will likely be issues when the groupSeparator property is enabled. You can access the locale's group separator with numberStringFormatter.group
  • There can be an e in the number, e.g. 10e2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the code is currently using this exact notation in lines 133-137 of number.ts:

const allLeadingZerosOptionallyNegative = /^([-0])0+(?=\d)/; const decimalOnlyAtEndOfString = /(?!^\.)\.$/; const allHyphensExceptTheStart = /(?!^-)-/g; const isNegativeDecimalOnlyZeros = /^-\b0\b\.?0*$/; const hasTrailingDecimalZeros = /0*$/;

Plus all current e2e tests pass.

Copy link
Member

@benelan benelan Mar 11, 2025

Choose a reason for hiding this comment

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

Plus all current e2e tests pass.

The existing tests don't cover this leading zero bug, which is why they aren't failing. However, the test you added wouldn't pass if you modified it to use the group-separator attribute. Same goes for the other cases I mentioned in my previous comment.

But the code is currently using this exact notation in lines 133-137 of number.ts:

My bad, the use of numberStringFormatter.decimal in the regex had me confused for a second. With input there are two different values:

  1. the value property, which is always in the en locale and latn numbering system
  2. the displayValue state, which is in the user-specified locale and numbering system

The regex you referenced are part of the number sanitation for the value property, which why it doesn't use the locale utils. The leading zeros get trimmed during number sanitation. After the number sanitation, the value is localized for the displayValue.

The trimmed, leading zeros need to be re-added to the displayValue, so they should be in the specified numbering system. The value property doesn't need the zeros re-added.

So the steps that need to be done for this issue are:

  1. Check for leading zeros before the number sanitation (like how we check for trailing decimals here):

const hasTrailingDecimalSeparator =
valueHandleInteger.charAt(valueHandleInteger.length - 1) === ".";

  1. Add back trimmed, leading zeros after the localization (like we are adding localized trailing decimal zeros here):

if (origin !== "connected" && !hasTrailingDecimalSeparator) {
newLocalizedValue = addLocalizedTrailingDecimalZeros(
newLocalizedValue,
newValue,
numberStringFormatter,
);
}

Notice how the 0 is being localized into the specified numbering system in the util:

return localizedValue.padEnd(localizedValue.length + trailingDecimalZeros.length, formatter.localize("0"));

The trailing, decimal zeros logic was added in #7159, I recommend taking a look that PR's implementation and discussion.

Please don't hesitate to reach out on Teams if you have any follow up questions. The input-number logic has a lot of features and edge cases, so it can be difficult to navigate at first.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants