[DO NO MERGE]: fix(number-field): separator interpreted as decimal #4552

mizgaionutalexandru · 2024-06-10T07:03:42Z

Description

Updated the special convertValueToNumber case for iPhones to correctly identify user's intent of using a decimal point.
Also changed the , or . that's being replaced with the website language specific decimal character from the first occurrence left-to-right to right-to-left. (e.g. for 1,234,56 previously the value would've been 1.23456, now it is 1,234.56 to better reflect user's intent).

Related issue(s)

[Bug]: Inconsistent behavior of sp-number-field in Safari on iOS #4531

Motivation and context

This solves the problem of a comma used as a separator in the number-field being interpreted as a decimal point when the user didn't want that. These changes came as a necessity since iOS keyboard shows either , or . based on device's settings.

How has this been tested?

Test case 1
1. Open storybook on an iPhone
2. Select the number-field component
3. Enter a value that would have a separator, such as 1000
4. Blur the number-field
5. Remove a digit from the input's value
6. Blur the number-field (the value should be 100)
Test case 2
1. Open storybook on an iPhone
2. Select the number-field component
3. Enter a value that would have a separator and a decimal, such as 1000.5
4. Blur the number-field
5. Remove/Add a digit from/to the input's value
6. Blur the number-field
Test case 3
1. Open storybook on an iPhone
2. Select the number-field component
3. Enter a value that would have a separator and a decimal, such as 1000.5
4. Blur the number-field
5. Remove/Add a digit and a comma/dot from/to the input's value
6. Blur the number-field

Below you can find videos of some of these testings done on two different types of keyboard.

comma.keyboard.dot.website.mov

dot.keyboard.dot.website.mov

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2024-06-10T07:06:20Z

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2024-06-10T07:11:42Z

Tachometer results

Chrome

number-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	524 kB	65.89ms - 67.77ms	-	faster ✔ 9% - 14% 6.57ms - 11.02ms
branch	511 kB	73.61ms - 77.64ms	slower ❌ 10% - 17% 6.57ms - 11.02ms	-

slider permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	480 kB	74.20ms - 75.87ms	-	faster ✔ 2% - 5% 1.50ms - 3.94ms
branch	467 kB	76.86ms - 78.64ms	slower ❌ 2% - 5% 1.50ms - 3.94ms	-

Firefox

number-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	524 kB	145.33ms - 150.99ms	-	faster ✔ 8% - 12% 12.70ms - 20.66ms
branch	511 kB	162.03ms - 167.65ms	slower ❌ 8% - 14% 12.70ms - 20.66ms	-

slider permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	480 kB	166.70ms - 175.10ms	-	unsure 🔍 -6% - -0% -11.21ms - +0.01ms
branch	467 kB	172.79ms - 180.21ms	unsure 🔍 -0% - +7% -0.01ms - +11.21ms	-

Rajdeepc · 2024-06-10T10:23:59Z

Can you please add a test here!

mizgaionutalexandru · 2024-06-12T06:52:44Z

packages/number-field/test/number-field.test.ts

@@ -117,6 +117,40 @@ describe('NumberField', () => {
            expect(el.focusElement.value).to.equal('13 377 331');
        });
    });
+    xit('correctly interprets decimal point on iPhone', async () => {


I created a test with some common usecases but haven't been able to run it since I couldn't find a way to mock isIphone.

* chore(overlay): overlay-trigger adds receives focus attribute * chore(overlay): added storybook * chore(overlay): updated golden image cache --------- Co-authored-by: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com> Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>

Co-authored-by: rmanole <rmanole@adobe.com>

mizgaionutalexandru · 2024-06-17T14:21:15Z

This approach seems to fix the issue in most cases. That being said, I've found a corner case caused by the following flow:

user clicks on number-field with a non-decimal value and with groupings (e.g. 3,000,000)
user types ,/. (based on iOS device language settings) and a number - boolean isIntentDecimal is set to true
user deletes said ,/. and number - boolean still true
user blurs number-field, now the value becomes 3,000.0

Maybe there is a comparison to be made so that we can reset isIntentDecimal? We should also consider the position of said ,/. to verify that the character deleted was the one added by the user, not a grouping/decimal separator that already existed in the string value.
I think this PR solves most problems with this caveat. Maybe we should consider a long-term solution in another issue/PR, be the approach explained above or others. How can we best approach this?

blunteshwar · 2024-06-18T04:14:11Z

Some tests are failing. Can you have a look at them. Other than that LGTM!

The merge-base changed after approval.

mizgaionutalexandru · 2024-06-18T10:48:19Z

Looks like the Lighthouse CI check problem is similar with this one: #4269 (review)

I think the performance check is also related given the fact that the storybook link is not generated correctly.

Rajdeepc · 2024-06-18T10:51:15Z

@mizgaionutalexandru I can see you have polluted the commit history. I see 4 commits from main are being logged here! Can you please close this PR and create a fresh PR with these changes?
Please create a new branch from main
Thank you!

mizgaionutalexandru requested a review from Rajdeepc June 10, 2024 07:04

mizgaionutalexandru and others added 2 commits June 11, 2024 16:39

fix(number-field): separator interpreted as decimal

631681f

chore: release new versions #publish

dd5179a

mizgaionutalexandru force-pushed the mizgaionutalexandru/4531-ios-number-field branch from d7184b6 to 0d57c2c Compare June 12, 2024 06:17

fix(number-field): fixed multiple separators usecase in decimal inputs

a04ef28

mizgaionutalexandru force-pushed the mizgaionutalexandru/4531-ios-number-field branch from 0d57c2c to a04ef28 Compare June 12, 2024 06:38

mizgaionutalexandru commented Jun 12, 2024

View reviewed changes

mizgaionutalexandru requested a review from TarunAdobe June 12, 2024 10:15

Rajdeepc and others added 2 commits June 13, 2024 15:04

feat: add missing --mod-* tokens for number-field (#4565)

9703c43

Co-authored-by: rmanole <rmanole@adobe.com>

Rajdeepc requested a review from blunteshwar June 17, 2024 07:24

Merge branch 'main' into mizgaionutalexandru/4531-ios-number-field

18ed53e

blunteshwar previously approved these changes Jun 18, 2024

View reviewed changes

mizgaionutalexandru force-pushed the main branch from 9703c43 to 0cab113 Compare June 18, 2024 06:21

Merge branch 'main' into mizgaionutalexandru/4531-ios-number-field

1c74531

blunteshwar requested review from blunteshwar June 18, 2024 10:43

Rajdeepc changed the title ~~fix(number-field): separator interpreted as decimal~~ [DO NO MERGE]: fix(number-field): separator interpreted as decimal Jun 18, 2024

mizgaionutalexandru mentioned this pull request Jun 18, 2024

fix(number-field): multiple separators use-cases in decimal inputs in iOS devices #4571

Merged

14 tasks

mizgaionutalexandru closed this Jun 18, 2024

mizgaionutalexandru deleted the mizgaionutalexandru/4531-ios-number-field branch June 18, 2024 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NO MERGE]: fix(number-field): separator interpreted as decimal #4552

[DO NO MERGE]: fix(number-field): separator interpreted as decimal #4552

mizgaionutalexandru commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024 •

edited

Loading

number-field permalink

basic-test

slider permalink

test-basic

number-field permalink

basic-test

slider permalink

test-basic

Rajdeepc commented Jun 10, 2024

mizgaionutalexandru Jun 12, 2024

mizgaionutalexandru commented Jun 17, 2024

blunteshwar commented Jun 18, 2024

mizgaionutalexandru commented Jun 18, 2024

Rajdeepc commented Jun 18, 2024 •

edited

Loading

[DO NO MERGE]: fix(number-field): separator interpreted as decimal #4552

[DO NO MERGE]: fix(number-field): separator interpreted as decimal #4552

Conversation

mizgaionutalexandru commented Jun 10, 2024 • edited Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

Types of changes

Checklist

Best practices

github-actions bot commented Jun 10, 2024

Branch preview

github-actions bot commented Jun 10, 2024 • edited Loading

Tachometer results

number-field permalink

basic-test

slider permalink

test-basic

number-field permalink

basic-test

slider permalink

test-basic

Rajdeepc commented Jun 10, 2024

mizgaionutalexandru Jun 12, 2024

Choose a reason for hiding this comment

mizgaionutalexandru commented Jun 17, 2024

blunteshwar commented Jun 18, 2024

mizgaionutalexandru commented Jun 18, 2024

Rajdeepc commented Jun 18, 2024 • edited Loading

mizgaionutalexandru commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

Rajdeepc commented Jun 18, 2024 •

edited

Loading