Skip to content

Fix/numberfield format options input reset#9905

Merged
snowystinger merged 5 commits into
adobe:mainfrom
Wonchang0314:fix/numberfield-format-options-input-reset
Apr 22, 2026
Merged

Fix/numberfield format options input reset#9905
snowystinger merged 5 commits into
adobe:mainfrom
Wonchang0314:fix/numberfield-format-options-input-reset

Conversation

@Wonchang0314
Copy link
Copy Markdown
Contributor

@Wonchang0314 Wonchang0314 commented Apr 10, 2026

Closes #1893
Related to #9899

Summary

When formatOptions is passed as an inline object literal (e.g. formatOptions={{ style: 'currency',
currency: 'USD' }}), every parent re-render creates a new object reference. This caused
useNumberFieldState to detect a "change" in formatOptions and overwrite the current inputValue with
the last committed number value — resetting whatever the user was typing mid-input.

This fix stabilizes the formatOptions reference inside useNumberFieldState using a useRef and a
shallow equality check. If the new formatOptions has the same key-value pairs as the previous one,
the old reference is reused, preventing a spurious inputValue reset. I though this approach is appropriate
because all values in Intl.NumberFormatOptions are primitives (strings, numbers, booleans), so
shallow equality is equivalent to deep equality.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Run the existing NumberField test suites to verify nothing is broken:
    yarn jest packages/react-aria-components/test/NumberField.test.js
    yarn jest packages/@adobe/react-spectrum/test/numberfield/NumberField.test.js
  2. The two new regression tests specifically cover this fix:
    - "does not reset input value when parent re-renders with same formatOptions content" — a parent
    component with its own state re-renders while formatOptions is an inline object literal; the typed
    value must be preserved.
    - "does not reset input during typing when re-rendered with same formatOptions content" — same
    scenario via the rerender helper.
  3. To manually verify: render a NumberField with formatOptions={{ style: 'currency', currency: 'USD'
    }} as an inline literal inside a component that re-renders on some other state change. Type a value
    without committing (no blur/Enter) and trigger the parent re-render — the typed value should no
    longer be reset.

🧢 Your Project:

@github-actions github-actions Bot added the RAC label Apr 10, 2026
@LFDanLu
Copy link
Copy Markdown
Member

LFDanLu commented Apr 15, 2026

@Wonchang0314 thanks for the PR! Mind signing the CLA?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I removed the changes made in useNumberFieldState and ran the tests, they still passed. Can you double check these and make sure they fail without the changes and pass with the fix? If that is proving difficult, feel free to add a storybook reproduction so we can more easily verify these changes,

Copy link
Copy Markdown
Contributor Author

@Wonchang0314 Wonchang0314 Apr 16, 2026

Choose a reason for hiding this comment

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

@LFDanLu Yes, I'm happy to sign the CLA. Should I just sign and submit the CLA form as an individual?

Copy link
Copy Markdown
Contributor Author

@Wonchang0314 Wonchang0314 Apr 16, 2026

Choose a reason for hiding this comment

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

@LFDanLu I found that the existing tests were not effectively covering this case, so I removed them. The issue here involves a state update triggered during render, which is not reliably reproduced in the jsdom environment.

Instead, I added tests using renderHook to directly test useNumberFieldState. I verified the behavior both before and after the fix to ensure that the issue is reproducible without the fix and resolved with it.

Thanks for taking a look!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Wonchang0314 Yep, you can just sign it as an individual. You might need to close and re-open the PR for it to run the check

Copy link
Copy Markdown
Contributor Author

@Wonchang0314 Wonchang0314 Apr 20, 2026

Choose a reason for hiding this comment

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

Hi @yihuiliao, I reopened the PR and it seems like all set. Would you mind taking a quick look when you have time?


// Stabilize formatOptions reference to avoid unnecessary re-renders
// when consumers pass inline object literals with the same content.
let formatOptionsRef = useRef(formatOptions);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like the lint is breaking here due to this change: Reading from refs during rendering is not allowed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct, this is not allowed as it can break React concurrency

Comment thread packages/react-stately/src/numberfield/useNumberFieldState.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe we should update the equality check here instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with your suggestion to update the equality check here. I've applied the fix and pushed the changes.

@github-actions github-actions Bot removed the RAC label Apr 16, 2026
@Wonchang0314 Wonchang0314 force-pushed the fix/numberfield-format-options-input-reset branch from fe21549 to 885f69d Compare April 16, 2026 02:08
@github-actions github-actions Bot added the RAC label Apr 16, 2026
describe('useNumberFieldState', () => {
describe('formatOptions stability', () => {
it('does not reset inputValue when re-rendered with same formatOptions content', () => {
let initialFormatOptions = {unit: 'millisecond', useGrouping: false};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let initialFormatOptions = {unit: 'millisecond', useGrouping: false};
let initialFormatOptions = {style: 'unit', unit: 'millisecond', useGrouping: false};

expect(result.current.inputValue).toBe('2');
});

it('resets inputValue when formatOptions content actually changes', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

im assuming this and the test below passed before these change were made and we just didn't have test coverage for them?

Copy link
Copy Markdown
Contributor Author

@Wonchang0314 Wonchang0314 Apr 20, 2026

Choose a reason for hiding this comment

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

yes, those two tests cover behaviors that already worked before this fix. If you think tests 2 and 3 are unnecessary, I can remove them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

happy to have more test coverage, just wanted to make sure i was understanding things correctly!

Copy link
Copy Markdown
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

lgtm

@snowystinger snowystinger added this pull request to the merge queue Apr 22, 2026
Merged via the queue into adobe:main with commit b0bfcb8 Apr 22, 2026
28 checks passed
@yihuiliao yihuiliao added the no testing Does not require manual testing during testing session label May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no testing Does not require manual testing during testing session

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useNumberField's formatOptions must be memo'ized

5 participants