From bd1bff8ec0fa26df6357272922d9f7868e4cc134 Mon Sep 17 00:00:00 2001 From: Theodore Brown Date: Mon, 20 Nov 2023 00:01:26 -0600 Subject: [PATCH] Fix select binding when matching enabled option has null value Resolves #9545 --- .../3-transform/client/visitors/template.js | 15 +++- packages/svelte/src/internal/client/render.js | 74 ++++++------------- .../_config.js | 2 +- .../_config.js | 28 +++++++ .../main.svelte | 11 +++ 5 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/main.svelte diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 57743a202c..5d6d9a02b2 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -567,7 +567,11 @@ function serialize_element_special_value_attribute(element, node_id, attribute, const inner_assignment = b.assignment( '=', b.member(node_id, b.id('value')), - b.assignment('=', b.member(node_id, b.id('__value')), value) + b.conditional( + b.binary('==', b.literal(null), b.assignment('=', b.member(node_id, b.id('__value')), value)), + b.literal(''), // render null/undefined values as empty string to support placeholder options + value + ) ); const is_reactive = attribute.metadata.dynamic; const needs_selected_call = @@ -582,9 +586,12 @@ function serialize_element_special_value_attribute(element, node_id, attribute, b.call('$.selected', node_id) ]) : needs_option_call - ? // This ensures a one-way street to the DOM in case it's - b.call('$.select_option', node_id, inner_assignment) + ? b.sequence([ + inner_assignment, + // This ensures a one-way street to the DOM in case it's + b.call('$.select_option', node_id, value) + ]) : inner_assignment ); diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index d992199d8d..c1f03a5114 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -444,20 +444,22 @@ export function class_toggle(dom, class_name, value) { * @template V * @param {HTMLSelectElement} select * @param {V} value + * @param {boolean} [mounting] */ -export function select_option(select, value) { +export function select_option(select, value, mounting) { if (select.multiple) { return select_options(select, value); } - for (let i = 0; i < select.options.length; i += 1) { - const option = select.options[i]; + for (const option of select.options) { const option_value = get_option_value(option); if (option_value === value) { option.selected = true; return; } } - select.value = ''; + if (!mounting || value !== undefined) { + select.value = ''; + } } /** @@ -466,8 +468,7 @@ export function select_option(select, value) { * @param {V} value */ function select_options(select, value) { - for (let i = 0; i < select.options.length; i += 1) { - const option = select.options[i]; + for (const option of select.options) { // @ts-ignore option.selected = ~value.indexOf(get_option_value(option)); } @@ -897,20 +898,10 @@ export function selected(dom) { } select = select.parentNode; } - if (select != null) { - // @ts-ignore - const select_value = select.__value; - // @ts-ignore - const option_value = dom.__value; - const selected = select_value === option_value; - dom.selected = selected; - dom.value = option_value; - // Handle the edge case of new options being added to a select when its state is "nothing selected" - // and keeping the selection state in sync (the DOM auto-selects the first option on insert) - // @ts-ignore - if (select.__value === null) { - /** @type {HTMLSelectElement} */ (select).value = ''; - } + // @ts-ignore + if (select != null && dom.__value === select.__value) { + // never set to false, since this causes browser to select default option + dom.selected = true; } }); } @@ -949,7 +940,7 @@ export function bind_value(dom, get_value, update) { * @returns {void} */ export function bind_select_value(dom, get_value, update) { - let mounted = false; + let mounting = true; dom.addEventListener('change', () => { /** @type {unknown} */ let value; @@ -964,40 +955,19 @@ export function bind_select_value(dom, get_value, update) { }); // Needs to be an effect, not a render_effect, so that in case of each loops the logic runs after the each block has updated effect(() => { - const value = get_value(); - if (value == null && !mounted) { + let value = get_value(); + select_option(dom, value, mounting); + if (mounting && value === undefined) { /** @type {HTMLOptionElement | null} */ - let selected_option = value === undefined ? dom.querySelector(':checked') : null; - if (selected_option === null) { - dom.value = ''; - // @ts-ignore - dom.__value = null; + let selected_option = dom.querySelector(':checked'); + if (selected_option !== null) { + value = get_option_value(selected_option); + update(value); } - const options = dom.querySelectorAll('option'); - for (const option of options) { - if (get_option_value(option) === value || option.hasAttribute('selected')) { - if (option.disabled) { - option.value = ''; - } - option.selected = true; - selected_option = option; - break; - } - } - if (selected_option != null) { - const non_null_value = get_option_value(selected_option); - update(non_null_value); - if (selected_option.hasAttribute('selected')) { - selected_option.removeAttribute('selected'); - selected_option.selected = true; - } - } - } else { - select_option(dom, value); - // @ts-ignore - dom.__value = value; } - mounted = true; + // @ts-ignore + dom.__value = value; + mounting = false; }); } diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-initial-value-undefined-2/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-initial-value-undefined-2/_config.js index b7ddbb7d3c..0590c6eef1 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-select-initial-value-undefined-2/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-initial-value-undefined-2/_config.js @@ -8,7 +8,7 @@ export default test({ diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/_config.js new file mode 100644 index 0000000000..e8d70675de --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/_config.js @@ -0,0 +1,28 @@ +import { ok, test } from '../../test'; + +const items = [{ id: 'a' }, { id: 'b' }]; + +export default test({ + get props() { + return { + /** @type {{ id: string } | null} */ + foo: null, + items + }; + }, + + test({ assert, component, target }) { + const select = target.querySelector('select'); + ok(select); + + const options = target.querySelectorAll('option'); + + assert.equal(options[0].selected, true); + assert.equal(options[1].selected, false); + assert.equal(options[0].value, ''); + + component.foo = items[0]; + assert.equal(options[0].selected, false); + assert.equal(options[1].selected, true); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/main.svelte b/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/main.svelte new file mode 100644 index 0000000000..56219ef284 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-null-placeholder-2/main.svelte @@ -0,0 +1,11 @@ + + +