Add assertions about <input> value dirty state #26626

sophiebits · 2023-04-14T03:59:33Z

Since this is an observable behavior and is hard to think about, seems good to have tests for this.

The expected value included in each test is the behavior that existed prior to #26546.

sophiebits · 2023-04-14T04:00:38Z

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

    expect(node.value).toBe('0');
+    expect(isValueDirty(node)).toBe(true);


Interestingly, this sole test fails on main with disableInputAttributeSyncing off:

● ReactDOMInput › should properly transition from an empty value to 0 expect(received).toBe(expected) // Object.is equality Expected: true Received: false 825 | 826 | expect(node.value).toBe('0'); > 827 | expect(isValueDirty(node)).toBe(true); | ^ 828 | 829 | if (disableInputAttributeSyncing) { 830 | expect(node.hasAttribute('value')).toBe(false);

(It passes with the flag on.) So it regressed recently. Possibly related to the bug @rickhanlonii was debugging.

react-sizebot · 2023-04-14T04:06:40Z

Comparing: b433c37...1eb923f

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.min.js	=	164.49 kB	164.49 kB	=	51.69 kB	51.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	166.96 kB	166.96 kB	=	52.37 kB	52.37 kB
facebook-www/ReactDOM-prod.classic.js	=	565.31 kB	565.31 kB	=	99.48 kB	99.48 kB
facebook-www/ReactDOM-prod.modern.js	=	549.10 kB	549.10 kB	=	96.79 kB	96.79 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 1eb923f

In facebook@2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in facebook#26626.

sebmarkbage · 2023-04-15T01:06:17Z

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

+  function isValueDirty(node) {
+    // Return the "dirty value flag" as defined in the HTML spec. Cast to text
+    // input to sidestep complicated value sanitization behaviors.
+    const copy = node.cloneNode();


Interesting that cloning clones the dirty flag.

In facebook@2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in facebook#26626.

In 2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in #26626.

Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.

In 2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in #26626. DiffTrain build for [b433c37](b433c37)

sebmarkbage · 2023-04-18T20:14:46Z

I'm very excited about this. Does something like this make sense on Textarea too?

sophiebits · 2023-04-18T20:16:23Z

Yes, and dirty checked for checkbox/radio.

Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to #26546.

In facebook/react@2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in facebook/react#26626. DiffTrain build for [b433c379d55d9684945217c7d375de1082a1abb8](facebook/react@b433c37)

In facebook@2019ddc, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in facebook#26626.

Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.

Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to #26546. DiffTrain build for commit 1b4a0da.

sophiebits requested review from sebmarkbage and rickhanlonii April 14, 2023 03:59

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 14, 2023

sophiebits commented Apr 14, 2023

View reviewed changes

sophiebits force-pushed the dirty-value branch from 3032480 to 77683a1 Compare April 14, 2023 05:03

sophiebits mentioned this pull request Apr 14, 2023

Fix input tracking bug #26627

Merged

This comment was marked as off-topic.

Sign in to view

sebmarkbage approved these changes Apr 15, 2023

View reviewed changes

sebmarkbage reviewed Apr 15, 2023

View reviewed changes

Add assertions about <input> value dirty state

1eb923f

Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.

sophiebits force-pushed the dirty-value branch from 77683a1 to 1eb923f Compare April 18, 2023 17:51

sophiebits merged commit 1b4a0da into facebook:main Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assertions about <input> value dirty state #26626

Add assertions about <input> value dirty state #26626

sophiebits commented Apr 14, 2023

sophiebits Apr 14, 2023

This comment was marked as off-topic.

react-sizebot commented Apr 14, 2023 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

sebmarkbage Apr 15, 2023

sebmarkbage commented Apr 18, 2023

sophiebits commented Apr 18, 2023

		expect(node.value).toBe('0');
		expect(isValueDirty(node)).toBe(true);

Add assertions about <input> value dirty state #26626

Add assertions about <input> value dirty state #26626

Conversation

sophiebits commented Apr 14, 2023

sophiebits Apr 14, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

react-sizebot commented Apr 14, 2023 • edited Loading

Critical size changes

Significant size changes

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

sebmarkbage Apr 15, 2023

Choose a reason for hiding this comment

sebmarkbage commented Apr 18, 2023

sophiebits commented Apr 18, 2023

react-sizebot commented Apr 14, 2023 •

edited

Loading