fix: binding group with if block (#8373)

Fixes #8372

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>

Author: tanhauhau

src/compiler/compile/render_dom/Block.ts

@@ -505,7 +505,7 @@ export default class Block {
 
 	render_binding_groups() {
 		for (const binding_group of this.binding_groups) {
-			binding_group.render();
+			binding_group.render(this);
 		}
 	}
 }

src/compiler/compile/render_dom/Renderer.ts

@@ -27,8 +27,8 @@ export interface BindingGroup {
 	contexts: string[];
 	list_dependencies: Set<string>;
 	keypath: string;
-	elements: Identifier[];
-	render: () => void;
+	add_element: (block: Block, element: Identifier) => void;
+	render: (block: Block) => void;
 }
 
 export default class Renderer {

src/compiler/compile/render_dom/wrappers/Element/Binding.ts

@@ -153,7 +153,7 @@ export default class BindingWrapper {
 			case 'group':
 			{
 				block.renderer.add_to_context('$$binding_groups');
-				this.binding_group.elements.push(this.parent.var);
+				this.binding_group.add_element(block, this.parent.var);
 
 				if ((this.parent as ElementWrapper).has_dynamic_value) {	
 					update_or_condition = (this.parent as ElementWrapper).dynamic_value_condition;	
@@ -323,7 +323,11 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B
 			parent = parent.parent;
 		}
 
-		const elements = [];
+		/**
+		 * When using bind:group with logic blocks, the inputs with bind:group may be scattered across different blocks.
+		 * This therefore keeps track of all the <input> elements that have the same bind:group within the same block.
+		 */
+		const elements = new Map<Block, any>();
 
 		contexts.forEach(context => {
 			renderer.add_to_context(context, true);
@@ -343,8 +347,13 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B
 			contexts,
 			list_dependencies,
 			keypath,
-			elements,
-			render() {
+			add_element(block, element) {
+				if (!elements.has(block)) {
+					elements.set(block, []);
+				}
+				elements.get(block).push(element);
+			},
+			render(block) {
 				const local_name = block.get_unique_name('binding_group');
 				const binding_group = block.renderer.reference('$$binding_groups');
 				block.add_variable(local_name);
@@ -362,7 +371,7 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B
 					);
 				}
 				block.chunks.hydrate.push(
-					b`${local_name}.p(${elements})`
+					b`${local_name}.p(${elements.get(block)})`
 				);
 				block.chunks.destroy.push(
 					b`${local_name}.r()`

src/runtime/internal/dom.ts

@@ -359,7 +359,7 @@ export function get_binding_group_value(group, __value, checked) {
 	return Array.from(value);
 }
 
-export function init_binding_group(group) {
+export function init_binding_group(group: HTMLInputElement[]) {
 	let _inputs: HTMLInputElement[];
 	return {
 		/* push */ p(...inputs: HTMLInputElement[]) {

test/runtime-puppeteer/index.ts

@@ -56,9 +56,7 @@ async function launchPuppeteer() {
 
 const assert = fs.readFileSync(`${__dirname}/assert.js`, 'utf-8');
 
-describe('runtime (puppeteer)', function() {
-	// Note: Increase the timeout in preparation for restarting Chromium due to SIGSEGV.
-	this.timeout(10000);
+describe('runtime (puppeteer)', () => {
 	before(async () => {
 		svelte = loadSvelte(false);
 		console.log('[runtime-puppeteer] Loaded Svelte');
@@ -75,7 +73,7 @@ describe('runtime (puppeteer)', function() {
 
 	const failed = new Set();
 
-	function runTest(dir, hydrate) {
+	function runTest(dir, hydrate, is_first_run) {
 		if (dir[0] === '.') return;
 		// MEMO: puppeteer can not execute Chromium properly with Node8,10 on Linux at GitHub actions.
 		const { version } = process;
@@ -254,11 +252,14 @@ describe('runtime (puppeteer)', function() {
 					prettyPrintPuppeteerAssertionError(err.message);
 					assertWarnings();
 				});
-		});
+		}).timeout(is_first_run ? 20000 : 10000);
 	}
 
+	// Increase the timeout on the first run in preparation for restarting Chromium due to SIGSEGV.
+	let first_run = true;
 	fs.readdirSync(`${__dirname}/samples`).forEach(dir => {
-		runTest(dir, false);
-		runTest(dir, true);
+		runTest(dir, false, first_run);
+		runTest(dir, true, first_run);
+		first_run = false;
 	});
 });

test/runtime/samples/binding-input-group-if-gh-8372-1/_config.js

@@ -0,0 +1,40 @@
+export default {
+	async test({ assert, target, component, window }) {
+		const button = target.querySelector('button');
+		const clickEvent = new window.Event('click');
+		const changeEvent = new window.Event('change');
+
+		const [input1, input2] = target.querySelectorAll('input[type="checkbox"]');
+		function validate_inputs(v1, v2) {
+			assert.equal(input1.checked, v1);
+			assert.equal(input2.checked, v2);
+		}
+
+		assert.deepEqual(component.test, []);
+		validate_inputs(false, false);
+
+		component.test = ['a', 'b'];
+		validate_inputs(true, true);
+
+		input1.checked = false;
+		await input1.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, ['b']);
+
+		input2.checked = false;
+		await input2.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, []);
+
+		input1.checked = true;
+		input2.checked = true;
+		await input1.dispatchEvent(changeEvent);
+		await input2.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, ['b', 'a']);
+
+		await button.dispatchEvent(clickEvent);
+		assert.deepEqual(component.test, ['b', 'a']); // should it be ['a'] only? valid arguments for both outcomes
+
+		input1.checked = false;
+		await input1.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, []);
+	}
+};

test/runtime/samples/binding-input-group-if-gh-8372-1/main.svelte

@@ -0,0 +1,14 @@
+<script>
+	export let test = [];
+	let hidden = false
+</script>
+
+<button on:click={() => hidden = !hidden}>
+	{hidden ? "show" : "hide"} b
+</button>
+
+<label>a <input type="checkbox" bind:group={test} value="a" /></label>
+{#if !hidden}
+<label>b <input type="checkbox" bind:group={test} value="b" /></label>
+{/if}
+<label>c <input value="just here, so b is not the last input" /></label>

test/runtime/samples/binding-input-group-if-gh-8372-2/_config.js

@@ -0,0 +1,34 @@
+export default {
+	async test({ assert, target, component, window }) {
+		const button = target.querySelector('button');
+		const clickEvent = new window.Event('click');
+		const changeEvent = new window.Event('change');
+
+		const [input1, input2] = target.querySelectorAll('input[type="radio"]');
+		function validate_inputs(v1, v2) {
+			assert.equal(input1.checked, v1);
+			assert.equal(input2.checked, v2);
+		}
+
+		component.test = 'a';
+		validate_inputs(true, false);
+
+		component.test = 'b';
+		validate_inputs(false, true);
+
+		input1.checked = true;
+		await input1.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, 'a');
+
+		input2.checked = true;
+		await input2.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, 'b');
+
+		await button.dispatchEvent(clickEvent);
+		assert.deepEqual(component.test, 'b'); // should it be undefined? valid arguments for both outcomes
+
+		input1.checked = true;
+		await input1.dispatchEvent(changeEvent);
+		assert.deepEqual(component.test, 'a');
+	}
+};

test/runtime/samples/binding-input-group-if-gh-8372-2/main.svelte

@@ -0,0 +1,14 @@
+<script>
+	export let test;
+	let hidden = false
+</script>
+
+<button on:click={() => hidden = !hidden}>
+	{hidden ? "show" : "hide"} b
+</button>
+
+<label>a <input type="radio" bind:group={test} value="a" /></label>
+{#if !hidden}
+<label>b <input type="radio" bind:group={test} value="b" /></label>
+{/if}
+<label>c <input value="just here, so b is not the last input" /></label>