Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tracked value after resetting radio group #27394

Merged
merged 3 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/react-dom-bindings/src/client/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -388,10 +388,6 @@ export function restoreControlledInputState(element: Element, props: Object) {
);
}

// We need update the tracked value on the named cousin since the value
// was changed but the input saw no event or value set
updateValueIfChanged(otherNode);

// If this is a controlled radio button group, forcing the input that
// was previously checked to update will cause it to be come re-checked
// as appropriate.
Expand All @@ -406,6 +402,16 @@ export function restoreControlledInputState(element: Element, props: Object) {
otherProps.name,
);
}

// If any updateInput() call set .checked to true, an input in this group
// (often, `rootNode` itself) may have become unchecked
for (let i = 0; i < group.length; i++) {
const otherNode = ((group[i]: any): HTMLInputElement);
if (otherNode.form !== rootNode.form) {
continue;
}
updateValueIfChanged(otherNode);
}
}
}

Expand Down
88 changes: 76 additions & 12 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,43 @@ describe('ReactDOMInput', () => {
return copy.value === node.value;
}

function isCheckedDirty(node) {
// Return the "dirty checked flag" as defined in the HTML spec.
if (node.checked !== node.defaultChecked) {
return true;
}
const copy = node.cloneNode();
copy.type = 'checkbox';
copy.defaultChecked = !copy.defaultChecked;
return copy.checked === node.checked;
}

function getTrackedAndCurrentInputValue(elem: HTMLElement): [mixed, mixed] {
const tracker = elem._valueTracker;
if (!tracker) {
throw new Error('No input tracker');
}
return [
tracker.getValue(),
elem.nodeName === 'INPUT' &&
(elem.type === 'checkbox' || elem.type === 'radio')
? String(elem.checked)
: elem.value,
];
}

function assertInputTrackingIsCurrent(parent) {
parent.querySelectorAll('input, textarea, select').forEach(input => {
const [trackedValue, currentValue] =
getTrackedAndCurrentInputValue(input);
if (trackedValue !== currentValue) {
throw new Error(
`Input ${input.outerHTML} is currently ${currentValue} but tracker thinks it's ${trackedValue}`,
);
}
});
}

beforeEach(() => {
jest.resetModules();

Expand Down Expand Up @@ -1119,13 +1156,15 @@ describe('ReactDOMInput', () => {
name="fruit"
checked={true}
onChange={emptyFunction}
data-which="a"
/>
A
<input
ref={this.bRef}
type="radio"
name="fruit"
onChange={emptyFunction}
data-which="b"
/>
B
<form>
Expand All @@ -1135,6 +1174,7 @@ describe('ReactDOMInput', () => {
name="fruit"
defaultChecked={true}
onChange={emptyFunction}
data-which="c"
/>
</form>
</div>
Expand Down Expand Up @@ -1162,6 +1202,11 @@ describe('ReactDOMInput', () => {
expect(cNode.hasAttribute('checked')).toBe(true);
}

expect(isCheckedDirty(aNode)).toBe(true);
expect(isCheckedDirty(bNode)).toBe(true);
expect(isCheckedDirty(cNode)).toBe(true);
assertInputTrackingIsCurrent(container);

setUntrackedChecked.call(bNode, true);
expect(aNode.checked).toBe(false);
expect(cNode.checked).toBe(true);
Expand All @@ -1183,6 +1228,11 @@ describe('ReactDOMInput', () => {
// The original state should have been restored
expect(aNode.checked).toBe(true);
expect(cNode.checked).toBe(true);

expect(isCheckedDirty(aNode)).toBe(true);
expect(isCheckedDirty(bNode)).toBe(true);
expect(isCheckedDirty(cNode)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should check the correct radio when the selected name moves', () => {
Expand Down Expand Up @@ -1219,11 +1269,15 @@ describe('ReactDOMInput', () => {
const stub = ReactDOM.render(<App />, container);
const buttonNode = ReactDOM.findDOMNode(stub).childNodes[0];
const firstRadioNode = ReactDOM.findDOMNode(stub).childNodes[1];
expect(isCheckedDirty(firstRadioNode)).toBe(true);
expect(firstRadioNode.checked).toBe(false);
assertInputTrackingIsCurrent(container);
dispatchEventOnNode(buttonNode, 'click');
expect(firstRadioNode.checked).toBe(true);
assertInputTrackingIsCurrent(container);
dispatchEventOnNode(buttonNode, 'click');
expect(firstRadioNode.checked).toBe(false);
assertInputTrackingIsCurrent(container);
});

it("shouldn't get tricked by changing radio names, part 2", () => {
Expand All @@ -1246,12 +1300,13 @@ describe('ReactDOMInput', () => {
</div>,
container,
);
expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
true,
);
expect(container.querySelector('input[name="a"][value="2"]').checked).toBe(
false,
);
const one = container.querySelector('input[name="a"][value="1"]');
const two = container.querySelector('input[name="a"][value="2"]');
expect(one.checked).toBe(true);
expect(two.checked).toBe(false);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);

ReactDOM.render(
<div>
Expand All @@ -1272,12 +1327,11 @@ describe('ReactDOMInput', () => {
</div>,
container,
);
expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
true,
);
expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(
true,
);
expect(one.checked).toBe(true);
expect(two.checked).toBe(true);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should control radio buttons if the tree updates during render', () => {
Expand Down Expand Up @@ -1339,6 +1393,9 @@ describe('ReactDOMInput', () => {

expect(aNode.checked).toBe(false);
expect(bNode.checked).toBe(true);
expect(isCheckedDirty(aNode)).toBe(true);
expect(isCheckedDirty(bNode)).toBe(true);
assertInputTrackingIsCurrent(container);

setUntrackedChecked.call(aNode, true);
// This next line isn't necessary in a proper browser environment, but
Expand All @@ -1352,6 +1409,9 @@ describe('ReactDOMInput', () => {
// The original state should have been restored
expect(aNode.checked).toBe(false);
expect(bNode.checked).toBe(true);
expect(isCheckedDirty(aNode)).toBe(true);
expect(isCheckedDirty(bNode)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should warn with value and no onChange handler and readOnly specified', () => {
Expand Down Expand Up @@ -1734,6 +1794,8 @@ describe('ReactDOMInput', () => {
<input type="radio" checked={false} onChange={() => null} />,
container,
);
const input = container.querySelector('input');
expect(isCheckedDirty(input)).toBe(true);
ReactDOM.render(
<input
type="radio"
Expand All @@ -1744,6 +1806,8 @@ describe('ReactDOMInput', () => {
/>,
container,
);
expect(isCheckedDirty(input)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should warn if radio checked false changes to become uncontrolled', () => {
Expand Down