🔥 Stop syncing the value attribute on inputs (behind a feature flag) #13526

nhunzaker · 2018-08-31T22:37:41Z

This PR revives #10150, my original PR to remove value attribute syncing.

TODOs:

value set value, defaultValue set defaultValue
checked sets checked, defaultChecked sets defaultChecked
Remove warning if defaultValue changes (if it exists)
Remove warning if defaultChecked changes (if it exists)
Can we get rid of wrapper state alltogether?
Could we only apply value to text inputs?
Could we apply checked to check/radios?
Could submit/reset just use standard attribute assignment?
SSR mismatch?

This fixture build demonstrates the behavior:

http://react-dom-no-sync-value.surge.sh/

See #11896 and #13525

🔥 🔥 🔥

nhunzaker · 2018-08-31T22:40:59Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

@@ -1605,15 +1605,15 @@ describe('ReactDOMInput', () => {
      };
    }

-    it('always sets the attribute when values change on text inputs', function() {
+    it('retains the initial value attribute when values change on text inputs', function() {


It's always been this way, but I don't like how React stores an initial value:
https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberInput.js#L25

I think the value should just be blank when it becomes null, or revert to the defaultValue prop (which could be null).

pull-bot · 2018-08-31T22:48:42Z

ReactDOM: size: -0.2%, gzip: -0.5%

Details of bundled changes.

Comparing: 2d5c590...5bb2e39

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	-0.1%	-0.6%	645.08 KB	644.23 KB	151.15 KB	150.23 KB	UMD_DEV
react-dom.production.min.js	-0.2%	-0.5%	92.16 KB	91.95 KB	30.08 KB	29.92 KB	UMD_PROD
react-dom.development.js	-0.1%	-0.7%	640.32 KB	639.47 KB	149.75 KB	148.72 KB	NODE_DEV
react-dom.production.min.js	-0.2%	-0.6%	92.1 KB	91.9 KB	29.67 KB	29.51 KB	NODE_PROD
ReactDOM-dev.js	-0.1%	-0.7%	662.65 KB	661.77 KB	152.09 KB	150.97 KB	FB_WWW_DEV
ReactDOM-prod.js	-0.1%	-0.9%	286.89 KB	286.53 KB	53.15 KB	52.68 KB	FB_WWW_PROD
react-dom.profiling.min.js	-0.2%	-0.4%	94.96 KB	94.75 KB	30.33 KB	30.21 KB	NODE_PROFILING
ReactDOM-profiling.js	-0.1%	-0.9%	293.28 KB	292.92 KB	54.51 KB	54.03 KB	FB_WWW_PROFILING

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

gaearon · 2018-09-02T11:24:25Z

Is there any other logic we can remove or simplify now? I was hoping some of the things we added over time might be unnecessary now. Although I'm aware some of your fixes were for issues that predated #6406.

gaearon · 2018-09-02T11:45:14Z

My original PR also removed synchronization for the checked attribute. However it isn't as problematic, and wasn't mentioned in #13525, so I didn't remove the behavior.

I talked to @sebmarkbage, and the guiding principles we came up with are:

React DOM should do what you'd do if you were writing mutations manually.
We only support mutations on uncontrolled components.

Our goal is to simplify the system and reduce the edge cases. So if we only set it out of courtesy for e.g. CSS selectors, I think we should stop doing that, and keep it minimal. Just let value set value, defaultValue set defaultValue, and same for checked / defaultChecked.

Regarding form.reset(). We should support it on uncontrolled components. For them, reverting to last supplied defaultValue makes sense. Which would happen anyway. We don't need to fire a change event for that. If you use a reset button, put an onReset on it if you want.

We should not support form.reset() on a controlled component. It is a mutation, and if you want it, just use an uncontrolled component — or change your resetting logic to reset the state instead. So we'll want to warn if you reset a form, and it contains a controlled component inside.

nhunzaker · 2018-09-02T19:24:29Z

Is there any other logic we can remove or simplify now? I was hoping some of the things we added over time might be unnecessary now. Although I'm aware some of your fixes were for issues that predated

I was too. I'll take a second pass through.

For everything else, I've started a checklist in the description of things I need to do.

gaearon · 2018-09-02T20:05:20Z

You might also want to start by adding a feature flag for this. Because we'll want to keep both versions around for a while.

It might be reasonable to fork some files like ReactDOMFiberInput completely later. Let's see which ones have the most feature flag checks after a bit of work.

nhunzaker · 2018-09-04T22:58:38Z

I did some more work today that I believe gets us most of the way there. I shouldn't continue to refine this anymore, next I need to:

Break down the changes into distinct feature flags
Test a few assumptions about defaultValue since we wrote the Chromium fix for value detachment
Follow up on some server side mismatching

I'm really curious about the final item. How should SSR work when value has no influence over the value attribute client-side?

I'm also starting to think that the rules are simple enough now that we can start to think about this more as a spec. I think I'd like to write up some rules, even if it's just for myself.

nhunzaker · 2018-09-04T23:01:49Z

One idea for SSR is to very strictly adhere to "value sets the property, defaultValue sets the attribute". For this to work, we'd need to allow a user to set both properties at once without warning.

I don't necessarily sponsor this idea 😸. This would be a breaking change for just about every server-side app, but it would keep things consistent in both environments.

It also requires a user to be intentional about setting the value attribute for sensitive information, like passwords, which could be a desired side-effect for some users.

gaearon

Please introduce a new feature flag for this. Let's call it something like disableInputAttributeSyncing. We need to be granular about changes because we might want to try them separately at FB, so I don't want to have just one feature flag.

We want to run relevant tests in both modes. But I think we don't want to run the whole bundle in two modes just yet.

So I propose that for now you can create ReactFire-test.internal.js and copy paste any tests where behavior differs into it, and adjust the expectations. And set the feature flag at the top of this file.

Then we can get it in and start testing at FB. It doesn't have to be 100% complete for this — since it won't be enabled by default. For example SSR stuff can wait.

nhunzaker · 2018-09-12T14:53:19Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+      // manual testing in the fixtures shows that the active element
+      // is no longer the input, however blur() + a blur event seem to
+      // be the only way to remove focus in JSDOM
+      node.blur()


@gaearon this was the false positive case mentioned earlier. JSDOM doesn't update the active element on blur, only after blur() + a blur event.

I have also walked through the number fixtures to verify that things continue to work as expected manually.

gaearon · 2018-09-12T15:05:12Z

packages/react-dom/src/client/ReactDOMInput.js

@@ -57,13 +58,14 @@ function isControlled(props) {

 export function getHostProps(element: Element, props: Object) {
  const node = ((element: any): InputWithWrapperState);
-  const checked = props.checked;
+  const checked =
+    props.checked != null ? props.checked : node._wrapperState.initialChecked;


You're doing props.checked access twice now. Let's avoid changes like this.

Removed in 14dab53

gaearon · 2018-09-12T15:06:07Z

packages/react-dom/src/client/ReactDOMInput.js

+    const currentValue = node.value;
+    const value = disableInputAttributeSyncing
+      ? getToStringValue(props.value)
+      : initialValue;


Seems like previous we didn't have to call this earlier than next condition. What changed? Let's avoid adding more eager computation.

getToStringValue is called when assigning initialValue in the wrapper state:

react/packages/react-dom/src/client/ReactDOMInput.js

Lines 120 to 122 in 7204b63

initialValue: getToStringValue(

props.value != null ? props.value : defaultValue,

),

Something can be done here. Working on it.

I was! I did this in addffb8

nhunzaker · 2018-09-12T15:25:45Z

packages/react-dom/src/client/ReactDOMInput.js

@@ -63,7 +64,7 @@ export function getHostProps(element: Element, props: Object) {
    defaultChecked: undefined,
    defaultValue: undefined,
    value: undefined,
-    checked: checked != null ? checked : node._wrapperState.initialChecked,
+    checked: props.checked != null ? props.checked : node._wrapperState.initialChecked


Ah, I'm being silly. I'll revert this too

nhunzaker · 2018-09-12T15:59:25Z

packages/react-dom/src/client/ReactDOMInput.js

      return;
    }

-    const initialValue = toString(node._wrapperState.initialValue);
-    const currentValue = node.value;


I inlined this value because it is only needed in one place in both feature flag branches.

nhunzaker · 2018-09-12T16:06:16Z

I believe all comments have been addressed and, reading through the code, I feel good about this change.

old

gaearon · 2018-09-12T17:02:32Z

packages/react-dom/src/client/ReactDOMInput.js

+        //   1. The value React property when present
+        //   2. The defaultValue React property when present
+        //   3. An empty string
+        if (initialValue !== node.value) {


This used to compare toString(node._wrapperState.initialValue) !== node.value.

But now it compares node._wrapperState.initialValue !== node.value.

Is that change safe?

You are correct. This should call toString()

Needs a regression test?

Hard. We only need to call toString for value comparison so that we do not reassign the existing default value and trigger validation. null and undefined are already skipped. This also happens on the post mount step, and not during hydration. I'm not sure how to write a test for it.

We have a manual test fixture for this reason (sorry, I know the formatting isn't ideal):

react/fixtures/dom/src/components/fixtures/text-inputs/index.js

Lines 65 to 99 in a7bd7c3

<Fixture>

<form className="control-box">

<fieldset>

<legend>Empty value prop string</legend>

<input value="" required={true} />

</fieldset>

<fieldset>

<legend>No value prop</legend>

<input required={true} />

</fieldset>

<fieldset>

<legend>Empty defaultValue prop string</legend>

<input required={true} defaultValue="" />

</fieldset>

<fieldset>

<legend>No value prop date input</legend>

<input type="date" required={true} />

</fieldset>

<fieldset>

<legend>Empty value prop date input</legend>

<input type="date" value="" required={true} />

</fieldset>

</form>

</Fixture>

<p className="footnote">

Checking the date type is also important because of a prior fix for

iOS Safari that involved assigning over value/defaultValue

properties of the input to prevent a display bug. This also triggers

input validation.

</p>

<p className="footnote">

The date inputs should be blank in iOS. This is not a bug.

</p>

</TestCase>

gaearon · 2018-09-12T17:05:17Z

packages/react-dom/src/client/ToStringValue.js

-export function toString(value: ToStringValue): string {
-  return '' + (value: any);
+export function toString(value: ?ToStringValue): string {
+  return value == null ? '' : '' + (value: any);


Why this change? Did we check every toString callsite to ensure it doesn't break them?

There were two places in postMount where it could stringify null or undefined. Otherwise this can stay the same. Updating now.

My priority here is that the old code path stays as similar as it can be. So that we can merge this without worrying.

Totally. I've done this in e4f68fe

nhunzaker · 2018-09-12T18:03:20Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js

+
+    if (ReactFeatureFlags.disableInputAttributeSyncing) {
+      // TODO: figure out why. This might be a bug.
+      expect(called).toBe(1);


It's possible that this happens because value isn't present, so no operation occurs on value, so value tracking is never triggered. A solution could be to set the value from defaultValue or manually call tracking.

gaearon

Old code paths look legit.

I'll need to take a closer look at new code paths later but we can get this in and iterate. Thank you!

…acebook#13526) * 🔥 Stop syncing the value attribute on inputs * Eliminate some additional checks * Remove initialValue and initialWrapper from wrapperState flow type * Update tests with new sync logic, reduce some operations * Update tests, add some caveats for SSR mismatches * Revert newline change * Remove unused type * Call toString to safely type string values * Add disableInputAttributeSyncing feature flag Reverts tests to original state, adds attribute sync feature flag, then moves all affected tests to ReactFire-test.js. * Revert position of types in toStringValues * Invert flag on number input blur * Add clarification why double blur is necessary * Update ReactFire number cases to be more explicite about blur * Move comments to reduce diff size * Add comments to clarify behavior in each branch * There is no need to assign a different checked behavior in Fire * Use checked reference * Format * Avoid precomputing stringable values * Revert getToStringValue comment * Revert placement of undefined in getToStringValue * Do not eagerly stringify value * Unify Fire test cases with normal ones * Revert toString change. Only assign unsynced values when not nully

burka · 2019-02-25T12:21:16Z

Is there a way to toggle this feature flag without recompiling react?
When is this feature scheduled?

kripod · 2019-12-17T10:28:16Z

Is there a reason this hasn't been enabled by default? It seems to be a cause for the bug #17609.

gaearon · 2019-12-17T13:34:31Z

No particular reason other than it's a breaking change so we can't enable it until 17.

facebook-github-bot added the CLA Signed label Aug 31, 2018

nhunzaker changed the title ~~🔥 Stop syncing the value attribute on inputs~~ 🔥 Stop syncing the value attribute on inputs Aug 31, 2018

gaearon self-assigned this Aug 31, 2018

nhunzaker commented Aug 31, 2018

View reviewed changes

nhunzaker force-pushed the nh-no-input-value-attribute branch from 10473d0 to ffb38e6 Compare August 31, 2018 22:53

This was referenced Sep 4, 2018

[RFC] Enabling form.reset() on controlled inputs, make value and defaultValue more transparent #8011

Closed

Password inputs do not synchronize the value attribute #12722

Closed

nhunzaker force-pushed the nh-no-input-value-attribute branch from ffb38e6 to 0715051 Compare September 4, 2018 16:41

gaearon previously requested changes Sep 11, 2018

View reviewed changes

nhunzaker force-pushed the nh-no-input-value-attribute branch from 5bb2e39 to f8c710c Compare September 12, 2018 02:25

nhunzaker commented Sep 12, 2018

View reviewed changes

gaearon mentioned this pull request Sep 12, 2018

Run tests for Fire feature flags #13628

Merged

gaearon reviewed Sep 12, 2018

View reviewed changes

nhunzaker commented Sep 12, 2018

View reviewed changes

nhunzaker added 7 commits September 12, 2018 17:43

🔥 Stop syncing the value attribute on inputs

46e6bc8

Eliminate some additional checks

7ce3884

Remove initialValue and initialWrapper from wrapperState flow type

9097f42

Update tests with new sync logic, reduce some operations

c520ff9

Update tests, add some caveats for SSR mismatches

ff926df

Revert newline change

d4eba96

Remove unused type

154c846

nhunzaker and others added 13 commits September 12, 2018 17:43

Invert flag on number input blur

ad3befc

Add clarification why double blur is necessary

69304e4

Update ReactFire number cases to be more explicite about blur

d79aca2

Move comments to reduce diff size

04f85c1

Add comments to clarify behavior in each branch

44d2204

There is no need to assign a different checked behavior in Fire

19cc9e7

Use checked reference

2e4868a

Format

2727f1e

Avoid precomputing stringable values

31fa167

Revert getToStringValue comment

1a37a43

Revert placement of undefined in getToStringValue

30752e1

Do not eagerly stringify value

7bc5f44

Unify Fire test cases with normal ones

fa10f5f

gaearon force-pushed the nh-no-input-value-attribute branch from 520c127 to fa10f5f Compare September 12, 2018 16:44

gaearon changed the title ~~🔥 Stop syncing the value attribute on inputs~~ 🔥 Stop syncing the value attribute on inputs (behind a feature flag) Sep 12, 2018

gaearon added Type: Breaking Change and removed Type: Breaking Change labels Sep 12, 2018

gaearon reviewed Sep 12, 2018

View reviewed changes

Revert toString change. Only assign unsynced values when not nully

e4f68fe

nhunzaker commented Sep 12, 2018

View reviewed changes

gaearon approved these changes Sep 12, 2018

View reviewed changes

gaearon merged commit a079011 into facebook:master Sep 12, 2018

Jessidhia mentioned this pull request Oct 4, 2018

Stop syncing value attribute for controlled inputs #11896

Open

vcanales mentioned this pull request Dec 17, 2019

Controlled numeric input gets cleared when unfocused #17609

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔥 Stop syncing the value attribute on inputs (behind a feature flag) #13526

🔥 Stop syncing the value attribute on inputs (behind a feature flag) #13526

nhunzaker commented Aug 31, 2018 •

edited

Loading

nhunzaker Aug 31, 2018

pull-bot commented Aug 31, 2018 •

edited

Loading

react-dom

schedule

gaearon commented Sep 2, 2018

gaearon commented Sep 2, 2018

nhunzaker commented Sep 2, 2018

gaearon commented Sep 2, 2018

nhunzaker commented Sep 4, 2018

nhunzaker commented Sep 4, 2018

gaearon left a comment •

edited

Loading

nhunzaker Sep 12, 2018

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018

nhunzaker Sep 12, 2018

nhunzaker Sep 12, 2018

nhunzaker Sep 12, 2018

nhunzaker Sep 12, 2018

nhunzaker commented Sep 12, 2018

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018 •

edited

Loading

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018

gaearon Sep 12, 2018

nhunzaker Sep 12, 2018 •

edited

Loading

nhunzaker Sep 12, 2018 •

edited

Loading

gaearon left a comment

burka commented Feb 25, 2019

kripod commented Dec 17, 2019

gaearon commented Dec 17, 2019

	initialValue: getToStringValue(
	props.value != null ? props.value : defaultValue,
	),

	<Fixture>
	<form className="control-box">
	<fieldset>
	<legend>Empty value prop string</legend>
	<input value="" required={true} />
	</fieldset>
	<fieldset>
	<legend>No value prop</legend>
	<input required={true} />
	</fieldset>
	<fieldset>
	<legend>Empty defaultValue prop string</legend>
	<input required={true} defaultValue="" />
	</fieldset>
	<fieldset>
	<legend>No value prop date input</legend>
	<input type="date" required={true} />
	</fieldset>
	<fieldset>
	<legend>Empty value prop date input</legend>
	<input type="date" value="" required={true} />
	</fieldset>
	</form>
	</Fixture>

	<p className="footnote">
	Checking the date type is also important because of a prior fix for
	iOS Safari that involved assigning over value/defaultValue
	properties of the input to prevent a display bug. This also triggers
	input validation.
	</p>
	<p className="footnote">
	The date inputs should be blank in iOS. This is not a bug.
	</p>
	</TestCase>

🔥 Stop syncing the value attribute on inputs (behind a feature flag) #13526

🔥 Stop syncing the value attribute on inputs (behind a feature flag) #13526

Conversation

nhunzaker commented Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

pull-bot commented Aug 31, 2018 • edited Loading

react-dom

schedule

gaearon commented Sep 2, 2018

gaearon commented Sep 2, 2018

nhunzaker commented Sep 2, 2018

gaearon commented Sep 2, 2018

nhunzaker commented Sep 4, 2018

nhunzaker commented Sep 4, 2018

gaearon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

nhunzaker Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

burka commented Feb 25, 2019

kripod commented Dec 17, 2019

gaearon commented Dec 17, 2019

nhunzaker commented Aug 31, 2018 •

edited

Loading

pull-bot commented Aug 31, 2018 •

edited

Loading

gaearon left a comment •

edited

Loading

nhunzaker Sep 12, 2018 •

edited

Loading

nhunzaker Sep 12, 2018 •

edited

Loading

nhunzaker Sep 12, 2018 •

edited

Loading