-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix: make defaultValue
work with spread
#14640
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'svelte': patch | ||
--- | ||
|
||
fix: make `defaultValue` work with spread |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -347,16 +347,31 @@ export function set_attributes( | |
} else if (key === '__value' || (key === 'value' && value != null)) { | ||
// @ts-ignore | ||
element.value = element[key] = element.__value = value; | ||
} else if (key === 'selected' && is_option_element) { | ||
set_selected(/** @type {HTMLOptionElement} */ (element), value); | ||
} else { | ||
var name = key; | ||
if (!preserve_attribute_case) { | ||
name = normalize_attribute(name); | ||
} | ||
let is_default_value_or_checked = name === 'defaultValue' || name === 'defaultChecked'; | ||
|
||
if (value == null && !is_custom_element) { | ||
if (value == null && !is_custom_element && !is_default_value_or_checked) { | ||
attributes[key] = null; | ||
// if we remove the value/checked attributes this also for some reasons reset | ||
// the default value so we need to keep track of it and reassign it after the remove | ||
let default_value_reset = /**@type {HTMLInputElement}*/ (element).defaultValue; | ||
let default_checked_reset = /**@type {HTMLInputElement}*/ (element).defaultChecked; | ||
element.removeAttribute(key); | ||
} else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) { | ||
if (key === 'value') { | ||
/**@type {HTMLInputElement}*/ (element).defaultValue = default_value_reset; | ||
} else if (key === 'checked') { | ||
/**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset; | ||
dummdidumm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} else if ( | ||
is_default_value_or_checked || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this check? AFAIK the setters should include defaultValue/defaultChecked There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this it was not working at all so I assumed no (tbh I didn't check) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
(setters.includes(name) && (is_custom_element || typeof value !== 'string')) | ||
) { | ||
// @ts-ignore | ||
element[name] = value; | ||
} else if (typeof value !== 'function') { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,263 @@ | ||
import { test } from '../../test'; | ||
import { flushSync } from 'svelte'; | ||
|
||
export default test({ | ||
async test({ assert, target }) { | ||
/** | ||
* @param {NodeListOf<any>} inputs | ||
* @param {string} field | ||
* @param {any | any[]} value | ||
*/ | ||
function check_inputs(inputs, field, value) { | ||
for (let i = 0; i < inputs.length; i++) { | ||
assert.equal(inputs[i][field], Array.isArray(value) ? value[i] : value, `field ${i}`); | ||
} | ||
} | ||
|
||
/** | ||
* @param {any} input | ||
* @param {string} field | ||
* @param {any} value | ||
*/ | ||
function set_input(input, field, value) { | ||
input[field] = value; | ||
input.dispatchEvent( | ||
new Event(typeof value === 'boolean' ? 'change' : 'input', { bubbles: true }) | ||
); | ||
} | ||
|
||
/** | ||
* @param {HTMLOptionElement} option | ||
*/ | ||
function select_option(option) { | ||
option.selected = true; | ||
option.dispatchEvent(new Event('change', { bubbles: true })); | ||
} | ||
|
||
const after_reset = []; | ||
|
||
const reset = /** @type {HTMLInputElement} */ (target.querySelector('input[type=reset]')); | ||
const [test1, test2, test3, test4, test5, test6, test7, test14] = | ||
target.querySelectorAll('div'); | ||
const [test8, test9, test10, test11] = target.querySelectorAll('select'); | ||
const [ | ||
test1_span, | ||
test2_span, | ||
test3_span, | ||
test4_span, | ||
test5_span, | ||
test6_span, | ||
test7_span, | ||
test8_span, | ||
test9_span, | ||
test10_span, | ||
test11_span | ||
] = target.querySelectorAll('span'); | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement | HTMLTextAreaElement>} */ | ||
const inputs = test1.querySelectorAll('input, textarea'); | ||
check_inputs(inputs, 'value', 'x'); | ||
assert.htmlEqual(test1_span.innerHTML, 'x x x x'); | ||
|
||
for (const input of inputs) { | ||
set_input(input, 'value', 'foo'); | ||
} | ||
flushSync(); | ||
check_inputs(inputs, 'value', 'foo'); | ||
assert.htmlEqual(test1_span.innerHTML, 'foo foo foo foo'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'value', 'x'); | ||
assert.htmlEqual(test1_span.innerHTML, 'x x x x'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement | HTMLTextAreaElement>} */ | ||
const inputs = test2.querySelectorAll('input, textarea'); | ||
check_inputs(inputs, 'value', 'x'); | ||
assert.htmlEqual(test2_span.innerHTML, 'x x x x'); | ||
|
||
for (const input of inputs) { | ||
set_input(input, 'value', 'foo'); | ||
} | ||
flushSync(); | ||
check_inputs(inputs, 'value', 'foo'); | ||
assert.htmlEqual(test2_span.innerHTML, 'foo foo foo foo'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'value', 'x'); | ||
assert.htmlEqual(test2_span.innerHTML, 'x x x x'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement | HTMLTextAreaElement>} */ | ||
const inputs = test3.querySelectorAll('input, textarea'); | ||
check_inputs(inputs, 'value', 'y'); | ||
assert.htmlEqual(test3_span.innerHTML, 'y y y y'); | ||
|
||
for (const input of inputs) { | ||
set_input(input, 'value', 'foo'); | ||
} | ||
flushSync(); | ||
check_inputs(inputs, 'value', 'foo'); | ||
assert.htmlEqual(test3_span.innerHTML, 'foo foo foo foo'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'value', 'x'); | ||
assert.htmlEqual(test3_span.innerHTML, 'x x x x'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement>} */ | ||
const inputs = test4.querySelectorAll('input'); | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test4_span.innerHTML, 'true true'); | ||
|
||
for (const input of inputs) { | ||
set_input(input, 'checked', false); | ||
} | ||
flushSync(); | ||
check_inputs(inputs, 'checked', false); | ||
assert.htmlEqual(test4_span.innerHTML, 'false false'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test4_span.innerHTML, 'true true'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement>} */ | ||
const inputs = test5.querySelectorAll('input'); | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test5_span.innerHTML, 'true true'); | ||
|
||
for (const input of inputs) { | ||
set_input(input, 'checked', false); | ||
} | ||
flushSync(); | ||
check_inputs(inputs, 'checked', false); | ||
assert.htmlEqual(test5_span.innerHTML, 'false false'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test5_span.innerHTML, 'true true'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement>} */ | ||
const inputs = test6.querySelectorAll('input'); | ||
check_inputs(inputs, 'checked', false); | ||
assert.htmlEqual(test6_span.innerHTML, 'false false'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test6_span.innerHTML, 'true true'); | ||
}); | ||
} | ||
{ | ||
/** @type {NodeListOf<HTMLInputElement>} */ | ||
const inputs = test7.querySelectorAll('input'); | ||
check_inputs(inputs, 'checked', true); | ||
assert.htmlEqual(test7_span.innerHTML, 'true'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(inputs, 'checked', false); | ||
assert.htmlEqual(test7_span.innerHTML, 'false'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLOptionElement>} */ | ||
const options = test8.querySelectorAll('option'); | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test8_span.innerHTML, 'b'); | ||
|
||
select_option(options[2]); | ||
flushSync(); | ||
check_inputs(options, 'selected', [false, false, true]); | ||
assert.htmlEqual(test8_span.innerHTML, 'c'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test8_span.innerHTML, 'b'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLOptionElement>} */ | ||
const options = test9.querySelectorAll('option'); | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test9_span.innerHTML, 'b'); | ||
|
||
select_option(options[2]); | ||
flushSync(); | ||
check_inputs(options, 'selected', [false, false, true]); | ||
assert.htmlEqual(test9_span.innerHTML, 'c'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test9_span.innerHTML, 'b'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLOptionElement>} */ | ||
const options = test10.querySelectorAll('option'); | ||
check_inputs(options, 'selected', [false, false, true]); | ||
assert.htmlEqual(test10_span.innerHTML, 'c'); | ||
|
||
select_option(options[0]); | ||
flushSync(); | ||
check_inputs(options, 'selected', [true, false, false]); | ||
assert.htmlEqual(test10_span.innerHTML, 'a'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test10_span.innerHTML, 'b'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLOptionElement>} */ | ||
const options = test11.querySelectorAll('option'); | ||
check_inputs(options, 'selected', [false, false, true]); | ||
assert.htmlEqual(test11_span.innerHTML, 'c'); | ||
|
||
select_option(options[0]); | ||
flushSync(); | ||
check_inputs(options, 'selected', [true, false, false]); | ||
assert.htmlEqual(test11_span.innerHTML, 'a'); | ||
|
||
after_reset.push(() => { | ||
check_inputs(options, 'selected', [false, true, false]); | ||
assert.htmlEqual(test11_span.innerHTML, 'b'); | ||
}); | ||
} | ||
|
||
{ | ||
/** @type {NodeListOf<HTMLInputElement | HTMLTextAreaElement>} */ | ||
const inputs = test14.querySelectorAll('input, textarea'); | ||
assert.equal(inputs[0].value, 'x'); | ||
assert.equal(/** @type {HTMLInputElement} */ (inputs[1]).checked, true); | ||
// this is still missing...i have no idea how to fix this lol | ||
// assert.equal(inputs[2].value, 'x'); | ||
|
||
after_reset.push(() => { | ||
assert.equal(inputs[0].value, 'y'); | ||
assert.equal(/** @type {HTMLInputElement} */ (inputs[1]).checked, false); | ||
assert.equal(inputs[2].value, 'y'); | ||
}); | ||
} | ||
|
||
reset.click(); | ||
await Promise.resolve(); | ||
flushSync(); | ||
after_reset.forEach((fn) => fn()); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, yeah that's why - now I'm wondering if we should model this differently: Check if the dom element is an input or textarea, and if so, if we come across a
value
orchecked
attribute, skip to the setters. Not sure which solution ends up being less code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need to remove the attribute, we just need to make sure to restore the default value