fix(input-number): allow editing of numbers that start with zeros

[josercarcamo]

Related Issue: #9611

Summary

Allows editing of numbers that start with zeros

[josercarcamo]

@benelan could you please review?

[benelan]

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!

[benelan]

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

[josercarcamo]

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.

[benelan]

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):

calcite-design-system/packages/calcite-components/src/components/input-number/input-number.tsx

Lines 823 to 824 in b9624f9

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

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

calcite-design-system/packages/calcite-components/src/components/input-number/input-number.tsx

Lines 840 to 846 in b9624f9

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

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

calcite-design-system/packages/calcite-components/src/utils/number.ts

Line 261 in b9624f9

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.

[github-actions]

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.

[benelan]

I suggested code that simplifies your implementation, which will hopefully make things easier to troubleshoot. Can you run with the suggested changes and see if you can finish it off from there?

Please reach out tomorrow if the suggestions don't unblock you, and I'll take another look. Thanks!

[benelan]

The regex here is unnecessarily complex, which is likely the cause of the test failures. We just need to know the following things:

• are there leading zeros, and if so, how many?
• is there a minus sign?

So the lines could be simplified to something like this:

    const hasLeadingMinusSign = valueHandleInteger.charAt(0) === "-";
    const hasLeadingZeros = valueHandleInteger.match(/^-?(0+)\d/);

[benelan]

With the regex change I suggested above, lines 851-871 could be changed to something like this:

    // adds localized trailing decimal separator
    if (hasTrailingDecimalSeparator && isValueDeleted) {
      newLocalizedValue = `${newLocalizedValue}${numberStringFormatter.decimal}`;
    }

    // adds localized leading zeros
    if (hasLeadingZeros) {
      newLocalizedValue = `${
        hasLeadingMinusSign ? newLocalizedValue.charAt(0) : ""
      }${numberStringFormatter.localize("0").repeat(hasLeadingZeros[1].length)}${
        hasLeadingMinusSign ? newLocalizedValue.slice(1) : newLocalizedValue
      }`;
    }

    this.displayedValue = newLocalizedValue;

Diff

-    //adds localized trailing decimal separator
-    this.displayedValue =
-      hasTrailingDecimalSeparator && isValueDeleted
-        ? `${newLocalizedValue}${numberStringFormatter.decimal}`
-        : newLocalizedValue;
- 
-    if (reResult) {
-      this.displayedValue = reResult[1]
-        ? `-${this.displayedValue
-            .substring(1, this.displayedValue.length)
-            .padStart(
-              valueHandleInteger.length - 1 - (hasTrailingDecimalSeparator ? 1 : 0),
-              numberStringFormatter.localize("0"),
-            )}`
-        : this.displayedValue.padStart(
-            valueHandleInteger.length,
-            numberStringFormatter.localize("0"),
-          );
-      this.displayedValue += hasTrailingDecimalSeparator ? numberStringFormatter.decimal : "";
-    }
+    // adds localized trailing decimal separator
+    if (hasTrailingDecimalSeparator && isValueDeleted) {
+      newLocalizedValue = `${newLocalizedValue}${numberStringFormatter.decimal}`;
+    }
+ 
+    // adds localized leading zeros
+    if (hasLeadingZeros) {
+      newLocalizedValue = `${
+        hasLeadingMinusSign ? newLocalizedValue.charAt(0) : ""
+      }${numberStringFormatter.localize("0").repeat(hasLeadingZeros[1].length)}${
+        hasLeadingMinusSign ? newLocalizedValue.slice(1) : newLocalizedValue
+      }`;
+    }
+ 
+    this.displayedValue = newLocalizedValue;

[benelan]

We may want to use the isValueDeleted boolean for the hasLeadingZeros conditional too. Can you investigate?

[josercarcamo]

@benelan could you please review?

[benelan]

Glad my suggestions worked, thanks for testing! The e2e tests need some changes and then this will be good to go 🚀

[benelan]

FYI the attribute to change the locale is lang, not locale

[benelan]

I don't think we need to test this feature for every locale.

Can you remove this test and add a new one on line 1544 that does the following:

• uses the arab numbering system
• uses the ar locale
• uses a minus sign
• uses a decimal

[benelan]

This test says it is using a group separator, but the attribute is missing from the component

Suggested change

	await page.setContent(html`<calcite-input-number></calcite-input-number>`);
	await page.setContent(html`<calcite-input-number group-separator></calcite-input-number>`);

[benelan]

Based on the modified expected value here, it looks like you are deleting the group separator instead of the 7. You will need to add another ArrowLeft keypress now that there is an additional character.

Alternatively you can do this, which doesn't depend on the length of the number:

    await page.keyboard.press("Home");
    await page.keyboard.press("Delete");

The other expected values below will also need to be updated to ensure they are testing the leading zero fix.

[benelan]

Suggested change

	await page.setContent(html`<calcite-input-number lang="ar"></calcite-input-number>`);
	await page.setContent(html`<calcite-input-number lang="ar" numbering-system="arab"></calcite-input-number>`);

You need to use the numbering-system attribute in order to change the numbering system. It doesn't change automatically when setting lang because some users want the latin numbering system and arab locale, for example.

[benelan]

I pushed some changes to fix the arabic test, LGTM now!

[jcfranco]

@josercarcamo for consistency, could you restore the previous HTML and either:

• move this particular Arabic configuration to a new section or
• omit it for now?