Skip to content

Commit

Permalink
Fix checkbox DOM sync (#50991)
Browse files Browse the repository at this point in the history
# Fix checkbox DOM sync

Unbreaks checkboxes with custom value attributes.

## Description

There was a trivial logic error in the DOM syncing code, causing the `value` attribute on checkboxes to get wrongly overwritten in a certain case (if there was a custom value attribute and `checked` was unchanged).

Fixes #50995

## Customer Impact

In some cases, checkbox values would get corrupted after an enhanced navigation. This would break form functionality.

The specific scenario was "checkboxes with a custom value attribute, when an enhanced nav leaves the `checked` state unchanged".

## Regression?

- [ ] Yes
- [x] No

[If yes, specify the version the behavior has regressed from]

## Risk

- [ ] High
- [ ] Medium
- [x] Low

Very low because it's a tiny code edit, and only affects a very specific scenario, and the old logic was (in retrospect) definitely incorrect.

## Verification

- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A
  • Loading branch information
SteveSandersonMS authored Sep 28, 2023
1 parent 07963de commit e242869
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.server.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.web.js

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions src/Components/Web.JS/src/Rendering/DomMerging/DomSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,10 @@ function ensureEditableValueSynchronized(destination: Element, value: any) {
} else if (destination instanceof HTMLSelectElement && destination.selectedIndex !== value) {
destination.selectedIndex = value as number;
} else if (destination instanceof HTMLInputElement) {
if (destination.type === 'checkbox' && destination.checked !== value) {
destination.checked = value as boolean;
if (destination.type === 'checkbox') {
if (destination.checked !== value) {
destination.checked = value as boolean;
}
} else if (destination.value !== value) {
destination.value = value as string;
}
Expand Down
16 changes: 16 additions & 0 deletions src/Components/Web.JS/test/DomSync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,22 @@ describe('DomSync', () => {
expect(selectElem.selectedIndex).toBe(2);
});

test('should handle checkboxes with value attribute', () => {
// Checkboxes require even more special-case handling because their 'value' attribute
// has to be handled as a regular attribute, and 'checked' must be handled similarly
// to 'value' on other inputs

const destination = makeExistingContent(`<input type='checkbox' value='first' checked />`);
const newContent = makeNewContent(`<input type='checkbox' value='second' checked />`);

const checkboxElem = destination.startExclusive.nextSibling as HTMLInputElement;

// Act/Assert
synchronizeDomContent(destination, newContent);
expect(checkboxElem.checked).toBeTruthy();
expect(checkboxElem.value).toBe('second');
});

test('should treat doctype nodes as unchanged', () => {
// Can't update a doctype after the document is created, nor is there a use case for doing so
// We just have to skip them, as it would be an error to try removing or inserting them
Expand Down

0 comments on commit e242869

Please sign in to comment.