Keep value of controlled number fields upon action

[alexdln]

trafficstars

Summary

The second attempt to fix the problem. The first one (#30737) is simpler and just an addition of a workaround. Now I've approached it more comprehensively.

Improved the system for inserting default values. Previously, they were inserted sometimes on change, sometimes on blur, sometimes never. I've implemented logic for determining default values for number and email inputs before submit (it's strange that there was a comment about the email input problem, but no condition).

I've expanded checks for other input types (it's strange that, for example, a defaultValue could theoretically be set for radio)

In doing so, I removed the test "should properly control 0.0 for a number input". I don't understand why it even exists - why is it expected that a controlled field can be overwritten in this way? It feels like it was working by sheer miracle. But we can bring back the ChangeEventPlugin logic. However, as I've come to understand, it's also full of problems and seems quite prone to bugs. Plus, it's unnecessary actions

Test plan

• added tests
• https://codesandbox.io/p/sandbox/react-keep-value-of-controlled-number-fields-upon-action-3sd2ly?file=%2Fsrc%2Findex.js

Closes https://github.com/facebook/react/issues/29862

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 30, 2024 4:05pm

[react-sizebot]

Comparing: dc32c7f35ed6699e302dc7dbae17804555c669c6...e1d61650a1b94fd5c95a2608d4687445c855dab1

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	+0.05%	1.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	+0.07%	500.37 kB	500.75 kB	+0.09%	89.80 kB	89.88 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	+0.07%	507.50 kB	507.87 kB	+0.09%	90.96 kB	91.05 kB
facebook-www/ReactDOM-prod.classic.js	+0.06%	595.24 kB	595.62 kB	+0.09%	105.55 kB	105.64 kB
facebook-www/ReactDOM-prod.modern.js	+0.07%	571.54 kB	571.91 kB	+0.09%	101.75 kB	101.84 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by :no_entry_sign: dangerJS against 65e58d17ae6d21d9b9d2a56c80baba72148f4f97

[eps1lon]

Should we verify this also works in an actual browser not just JSDOM? You can use https://react.new and a build from that PR via https://ci.codesandbox.io/status/facebook/react/pr/30752 to create a repro highlighting the fix.

[eps1lon]

Seems like this is failing with yarn test --variant=true

[alexdln]

While making the demo, I decided to check how checkable inputs work. And as it turned out, they don't work. I'll try to deal with them separately later. It's strange that there are no bugs reported for this... For clarity, of course, I checked in the version without my changes - https://codesandbox.io/p/devbox/silly-chebyshev-rshrhl-forked-g68h27.

And here's the version with my changes - https://codesandbox.io/p/devbox/656qfj. The numeric field works well, but the checkable fields are still being cleared:

https://github.com/user-attachments/assets/82c1c48e-1869-4d59-8852-cdf14af4b955

[eps1lon]

FYI you can just use URLs in the package.json in the sandboxes. This helps to confirm the correct version is used.

Can you summarize in the PR description what is being fixed and attach a before/after video? I can't seem to spot a difference but maybe I'm using the wrong browser?

[alexdln]

Can you summarize in the PR description what is being fixed and attach a before/after video? I can't seem to spot a difference but maybe I'm using the wrong browser?

This PR is specifically for fixing the number input. When submitting from the number input itself (i.e., changing the value without leaving it and pressing Enter) - the value won't reset to the initial one. In the previous version, it was resetting because it was saved on blur.

https://github.com/user-attachments/assets/f17a8e3f-26f8-4aee-9b86-da6de6a05a70

Now it is saved before submit:

https://github.com/user-attachments/assets/a6eb0153-1908-4341-aea7-610f843aecf3

[eps1lon]

Gotcha, thank you for the clarification. I was submitting via click and then it didn't reset. That's also probably why we haven't gotten any reports yet because most devs test with a mouse or with an explicit submit. Not by an implicit submit via {enter} while focusing a form element.

[alexdln]

Gotcha, thank you for the clarification. I was submitting via click and then it didn't reset. That's also probably why we haven't gotten any reports yet because most devs test with a mouse or with an explicit submit. Not by an implicit submit via {enter} while focusing a form element.

Yes, but at the same time this opens up some very unpleasant artifacts. And as I wrote above, I also caught the same error with checkable inputs (which will try to fix a bit later [after merging the current PR])

Video without current fix (https://codesandbox.io/p/devbox/silly-chebyshev-rshrhl-forked-g68h27):

https://github.com/user-attachments/assets/fbdaf95e-323e-467a-8d6d-3bf5ba87feb0

[eps1lon]

Yes, but at the same time this opens up some very unpleasant artifacts.

You mean this PR introduces new bugs or during testing you noticed existing bugs? Controlled <select> being subject to automatic form reset is already known: https://github.com/facebook/react/issues/30580

[alexdln]

During testing noticed existing bugs (the same, but with checkbox and radio inputs)

[alexdln]

Just thoughts out loud, so as not to lose all these points and conclusions.

It turns out:

1. Inputs can be controlled or uncontrolled. Controlled ones are those that have a value. However, those that have a value but no change event will report an warning;
2. defaultValue is the base value of the input. It should be set as the value during rendering and will be restored when the field is reset (if it's uncontrolled);
3. If the field is controlled, it's expected that defaultValue will repeat the value for using the logic above;
4. Chrome has a bug where changing defaultValue can trigger validation (for number and email inputs), so defaultValue for number input is set on blur (after change it will be on submit);

Details:

• All this logic works on every change in the field;
• All this logic should work for number, text, email, date, tel and other text-like fields;
• For checkbox, everything should work the same, but with checked and defaultChecked props;
• For radio, the logic should track all fields with the current name in the form and control attributes considering the others;
• For the select element, it's the same as for radio, but with selected and defaultSelected props;
• For button, reset, image, file and other fields without input possibility - nothing should happen (or is the value attribute removed?)

What's strange

• In some situations, during input, the value attribute itself changes, in others it doesn't;
• Value changes work differently (see above - why a text field can't change 0 to 0.0, but a number field can);
• The logic still doesn't work on checkbox, radio, select even though there are separate conditions for these fields in the code and, for example, defaultChecked processing;
• The logic on defaultValue works on all field types, including checkbox, which supposedly doesn't need it;

All this logic is definitely needed on reset. But judging by the tests, it somehow participates in re-renders. But how? What kind of situation is it that I was in a field, entered a value into it, then a re-render occurred and the value was cleared. Apparently, the field was completely recreated, but can this happen for the same element in the same place?

Is all this really the right way? Maybe there's a possibility to leave native field handling? But the question is - where exactly is this logic needed. If in fact it's only needed during reset - we can look towards customizing the reset itself (resetting uncontrolled fields to the original defaultValue, leaving controlled fields unchanged).

[alexdln]

@eps1lon @sophiebits

Is there anything else that needs to be done? I'm not closing the threads in case you have any more questions or a different perspective (you know all the details better)

So from the active question - should we bring back the logic with blur? At first glance, it seems unnecessary and redundant. But if you prefer more cautious changes, we can restore it and try to remove it in subsequent tasks.

I think I've answered the rest of the questions, and I've also detailed all the issues and the current principle (might come in handy)

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[sophiebits]

Apologies this is still open — removing the stale label.

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[github-actions[bot]]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!