bug(number-field): update number-field value on pressing "enter" #3638

TarunAdobe · 2023-09-12T11:39:00Z

Description

If you type in a number larger than the max attribute and press "return", it should change the number to the max.
i.e. The value inside the number-field should update on pressing "return" however right now it only happens when the input is blurred out.

Related issue(s)

[Bug]: sp-number-field doesn't always limit to max value upon return #3000

Motivation and context

If you type in a number larger than the max attribute and press "return", it doesn't always change the number. However, the number is always changed when the input is blurred. In addition, the number is never changed when format-options is used, and the number is only changed the first time you press "return" even without format-options.

How has this been tested?

Test case 1
1. Go here
2. Do this
Test case 2
1. Go here
2. Do this

Screenshots (if appropriate)

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 · 2023-09-12T11:43:09Z

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 · 2023-09-12T11:50:39Z

Tachometer results

Chrome

number-field permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	622 kB	270.12ms - 275.32ms	-	unsure 🔍 -1% - +2% -1.88ms - +4.82ms
branch	618 kB	269.14ms - 273.36ms	unsure 🔍 -2% - +1% -4.82ms - +1.88ms	-

slider permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	461 kB	214.69ms - 218.65ms	-	unsure 🔍 -2% - +1% -4.65ms - +1.31ms
branch	457 kB	216.11ms - 220.57ms	unsure 🔍 -1% - +2% -1.31ms - +4.65ms	-

Firefox

number-field permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	622 kB	422.96ms - 444.36ms	-	unsure 🔍 -2% - +5% -8.41ms - +20.17ms
branch	618 kB	418.31ms - 437.25ms	unsure 🔍 -5% - +2% -20.17ms - +8.41ms	-

slider permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	461 kB	347.83ms - 362.09ms	-	unsure 🔍 -6% - +1% -23.18ms - +4.78ms
branch	457 kB	352.14ms - 376.18ms	unsure 🔍 -1% - +7% -4.78ms - +23.18ms	-

blunteshwar · 2023-09-14T15:05:51Z

@TarunAdobe some VRTs are failing. Can you look at them

Westbrook · 2023-09-20T11:19:13Z

packages/number-field/src/NumberField.ts

@@ -340,6 +340,13 @@ export class NumberField extends TextfieldBase {
                    new Event('change', { bubbles: true, composed: true })
                );
                break;
+            case 'Enter':
+                event.preventDefault();


What is the default that you're preventing here?

We haven't gotten to Form Association for these Custom Elements, but in the future, were this associated to a form, there would be some expectation that Enter would submit the form by default.

good point! I have removed the event.preventDefault() and I've updated the few tests with the current behaviour expectations

Westbrook · 2023-09-20T11:19:36Z

packages/number-field/src/NumberField.ts

+                this.inputElement.value = this.formattedValue;
+                this.dispatchEvent(
+                    new Event('change', { bubbles: true, composed: true })
+                );


This looks good are there any tests that would be beneficial to add in support of this going forward?

Westbrook

Really like what you've settled on here! A couple of extra cases to confirm and tests to duplicate before we can merge this, but it's really starting to shape up.

packages/number-field/src/NumberField.ts

Westbrook · 2023-09-28T00:24:46Z

packages/number-field/src/NumberField.ts

@@ -400,7 +405,11 @@ export class NumberField extends TextfieldBase {
            }
        }
        this.value = value;


This line will trigger the setter. Will the following lines never accidentally dispatch a second change event?

no because in the setter we are updated the lastCommittedValue which will cause the check on the line 409 to fail making sure there is not second change event

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

Westbrook

This looks good to go. I'm gonna rebase this to main and make sure CI is still happy before merging.

One thing to keep an eye on, the tests that you touched were a tiny bit flaky before and this may increase the negative effect of that flakiness in future work. If we see a jump in transient failures, let's make time in a team sync to discuss ways to make these tests more stable.

…input

TarunAdobe force-pushed the bug/number-field-max-value branch from 2360ef8 to 5a426e4 Compare September 13, 2023 09:32

TarunAdobe self-assigned this Sep 13, 2023

TarunAdobe requested review from Westbrook, Rajdeepc, blunteshwar and najikahalsema September 13, 2023 09:32

TarunAdobe added the ready-for-review label Sep 13, 2023

TarunAdobe marked this pull request as ready for review September 13, 2023 09:33

TarunAdobe force-pushed the bug/number-field-max-value branch 2 times, most recently from 7808521 to bc7ffca Compare September 20, 2023 05:33

Rajdeepc previously approved these changes Sep 20, 2023

View reviewed changes

Rajdeepc added Component: Slider a11y Issues related to accessibility labels Sep 20, 2023

Rajdeepc linked an issue Sep 20, 2023 that may be closed by this pull request

[Bug]: NumberField does not reflect the correct value in the UI when value is a negative floating number and step is a floating number #3508

Closed

1 task

Westbrook reviewed Sep 20, 2023

View reviewed changes

TarunAdobe dismissed Rajdeepc’s stale review via fc8b4e1 September 21, 2023 07:33

TarunAdobe force-pushed the bug/number-field-max-value branch 2 times, most recently from 1f861d7 to de0544f Compare September 21, 2023 11:16

TarunAdobe linked an issue Sep 25, 2023 that may be closed by this pull request

[Bug]: sp-number-field doesn't always limit to max value upon return #3000

Closed

1 task

blunteshwar removed a link to an issue Sep 26, 2023

[Bug]: NumberField does not reflect the correct value in the UI when value is a negative floating number and step is a floating number #3508

Closed

1 task

TarunAdobe closed this Sep 27, 2023

TarunAdobe force-pushed the bug/number-field-max-value branch from 20c79f0 to 61aa8cc Compare September 27, 2023 10:19

TarunAdobe reopened this Sep 27, 2023

TarunAdobe requested review from Rajdeepc and Westbrook September 27, 2023 11:46

Westbrook reviewed Sep 28, 2023

View reviewed changes

TarunAdobe requested a review from Westbrook September 29, 2023 05:49

Westbrook approved these changes Sep 29, 2023

View reviewed changes

TarunAdobe added 3 commits September 29, 2023 08:41

fix(number-field): updated onchange logic

8c16cb7

fix(number-field): added a variable to track last committed value in …

65ac8c0

…input

chore(number-field): added test for onchange events

0e21010

Westbrook force-pushed the bug/number-field-max-value branch from 65bc5ae to 0e21010 Compare September 29, 2023 12:42

Westbrook merged commit 649eb2f into main Sep 29, 2023
8 checks passed

Westbrook deleted the bug/number-field-max-value branch September 29, 2023 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug(number-field): update number-field value on pressing "enter" #3638

bug(number-field): update number-field value on pressing "enter" #3638

TarunAdobe commented Sep 12, 2023

github-actions bot commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

number-field permalink

slider permalink

number-field permalink

slider permalink

blunteshwar commented Sep 14, 2023

Westbrook Sep 20, 2023

TarunAdobe Sep 21, 2023

Westbrook Sep 20, 2023

Westbrook left a comment

Westbrook Sep 28, 2023

TarunAdobe Sep 28, 2023

Westbrook left a comment

bug(number-field): update number-field value on pressing "enter" #3638

bug(number-field): update number-field value on pressing "enter" #3638

Conversation

TarunAdobe commented Sep 12, 2023

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Sep 12, 2023 • edited Loading

Branch Preview

github-actions bot commented Sep 12, 2023 • edited Loading

Tachometer results

number-field permalink

slider permalink

number-field permalink

slider permalink

blunteshwar commented Sep 14, 2023

Westbrook Sep 20, 2023

Choose a reason for hiding this comment

TarunAdobe Sep 21, 2023

Choose a reason for hiding this comment

Westbrook Sep 20, 2023

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

Westbrook Sep 28, 2023

Choose a reason for hiding this comment

TarunAdobe Sep 28, 2023

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading