Proper child reconciliation for web components with slots. (#762)

mesop/examples/web_component/slot/counter_component.js

@@ -22,6 +22,7 @@ class CounterComponent extends LitElement {
         <button id="decrement-btn" @click="${this._onDecrement}">
           Decrement
         </button>
+        <input type="text" />
       </div>
     `;
   }

mesop/examples/web_component/slot/outer_component.js

@@ -19,7 +19,7 @@ class OuterComponent extends LitElement {
   render() {
     return html`
       <div class="container">
-        <span>Value: ${this.value}</span>
+        <span id="outer-value">Value: ${this.value}</span>
         <button id="increment-btn" @click="${this._onIncrement}">
           increment
         </button>

mesop/examples/web_component/slot/slot_app.py

@@ -10,6 +10,10 @@
 )
 
 
+def on_add_checkbox_slot(e: me.ClickEvent):
+  me.state(State).checkbox_slot = True
+
+
 @me.page(
   path="/web_component/slot/slot_app",
   security_policy=me.SecurityPolicy(
@@ -19,6 +23,8 @@
   ),
 )
 def page():
+  s = me.state(State)
+  me.button("add checkbox slot", on_click=on_add_checkbox_slot)
   with outer_component(
     value=me.state(State).value,
     on_increment=on_increment,
@@ -27,26 +33,32 @@ def page():
       me.text(
         "You can use built-in components inside the slot of a web component."
       )
-      #
-      me.checkbox(
-        # Need to set |checked| because of https://github.com/google/mesop/issues/449
-        checked=me.state(State).checked,
-        label="Checked?",
-        on_change=on_checked,
-      )
+      if s.checkbox_slot:
+        me.checkbox(
+          label="Checked?",
+          on_change=on_checked,
+        )
+      me.input(key="input", label="input slot", on_input=on_input)
       counter_component(
+        key="counter",
         value=me.state(State).value,
         on_decrement=on_decrement,
       )
   me.text(f"Checked? {me.state(State).checked}")
 
 
+def on_input(e: me.InputEvent):
+  me.state(State).input = e.value
+
+
 def on_checked(e: me.CheckboxChangeEvent):
   me.state(State).checked = e.checked
 
 
 @me.stateclass
 class State:
+  checkbox_slot: bool = False
+  input: str
   checked: bool
   value: int = 10
 

mesop/tests/e2e/web_components/slot_test.ts

@@ -1,39 +1,61 @@
 import {test, expect, Page} from '@playwright/test';
 
-test.describe(() => {
-  // All tests in this describe group will get 2 retry attempts.
-  test.describe.configure({retries: 2});
-  test('web components - slot', async ({page}) => {
-    await page.goto('http://localhost:32123/web_component/slot/slot_app');
-    // Make sure the page has loaded:
-    expect(page.getByRole('button', {name: 'Decrement'})).toBeVisible();
-
-    assertValue(10);
-    await page.getByRole('button', {name: 'increment'}).click();
-    assertValue(11);
-    await page.getByRole('button', {name: 'increment'}).click();
-    assertValue(12);
-    await page.getByRole('button', {name: 'Decrement'}).click();
-    assertValue(11);
-    await page.getByRole('button', {name: 'Decrement'}).click();
-    assertValue(10);
-
-    async function assertValue(value: number) {
-      // Check that the outer component is displaying the right value.
-      expect(
-        await page
-          .locator('div')
-          .filter({hasText: `Value: ${value} increment Start of`})
-          .textContent(),
-      ).toContain(value.toString());
-
-      // Check that the inner component is displaying the right value.
-      expect(
-        await page
-          .locator('slot-counter-component')
-          .getByText('Value:')
-          .textContent(),
-      ).toContain(value.toString());
-    }
-  });
+test('web components - slot', async ({page}) => {
+  await page.goto('/web_component/slot/slot_app');
+
+  // Make sure the page has loaded:
+  expect(page.getByRole('button', {name: 'Decrement'})).toBeVisible();
+
+  await assertValue(10);
+  await page.getByRole('button', {name: 'increment'}).click();
+  await assertValue(11);
+  await page.getByRole('button', {name: 'increment'}).click();
+  await assertValue(12);
+  await page.getByRole('button', {name: 'Decrement'}).click();
+  await assertValue(11);
+  await page.getByRole('button', {name: 'Decrement'}).click();
+  await assertValue(10);
+
+  async function assertValue(value: number) {
+    // Check that the outer component is displaying the right value.
+    expect(
+      await page
+        .locator('#outer-value')
+        .filter({hasText: `Value: ${value}`})
+        .textContent(),
+    ).toContain(value.toString());
+
+    // Check that the inner component is displaying the right value.
+    expect(
+      await page
+        .locator('slot-counter-component')
+        .getByText('Value:')
+        .textContent(),
+    ).toContain(value.toString());
+  }
+});
+
+test('web components - slot child reconciliation', async ({page}) => {
+  await page.goto('/web_component/slot/slot_app');
+  await page.getByLabel('input slot').click();
+  await page.getByLabel('input slot').fill('abc');
+
+  await page.locator('slot-counter-component').getByRole('textbox').click();
+  await page.locator('slot-counter-component').getByRole('textbox').fill('def');
+
+  await page.getByRole('button', {name: 'Decrement'}).click();
+  await page.getByRole('button', {name: 'add checkbox slot'}).click();
+
+  await page.getByRole('checkbox').click();
+
+  // Assertions
+  await expect(page.getByRole('checkbox')).toBeChecked();
+  await expect(page.getByText('Checked? true')).toBeVisible();
+  expect(await page.getByLabel('input slot').inputValue()).toEqual('abc');
+  expect(
+    await page
+      .locator('slot-counter-component')
+      .getByRole('textbox')
+      .inputValue(),
+  ).toEqual('def');
 });

mesop/web/src/component_renderer/component_renderer.ts

@@ -159,28 +159,11 @@ export class ComponentRenderer {
 
   ngOnChanges() {
     if (this.customElement) {
-      // This is a naive way to apply changes by removing all the children
-      // and creating new children. In the future, this can be optimized
-      // to be more performant, but this naive approach should have the
-      // correct behavior, albeit inefficiently.
-      // See: https://github.com/google/mesop/issues/449
-
-      // Clear existing children
-      for (const element of Array.from(
-        this.customElement.querySelectorAll(COMPONENT_RENDERER_ELEMENT_NAME),
-      )) {
-        this.customElement.removeChild(element);
-      }
-
-      // Update the custom element and its children
+      // Update the custom element properties and events
       this.updateCustomElement(this.customElement);
-      for (const child of this.component.getChildrenList()) {
-        const childElement = document.createElement(
-          COMPONENT_RENDERER_ELEMENT_NAME,
-        );
-        (childElement as any)['component'] = child;
-        this.customElement.appendChild(childElement);
-      }
+
+      // Efficiently update children
+      this.updateCustomElementChildren();
       return;
     }
     if (isRegularComponent(this.component)) {
@@ -233,6 +216,34 @@ export class ComponentRenderer {
     }
   }
 
+  private updateCustomElementChildren() {
+    const existingChildren = Array.from(
+      this.customElement!.querySelectorAll(COMPONENT_RENDERER_ELEMENT_NAME),
+    );
+    const newChildren = this.component.getChildrenList();
+
+    // Update or add children
+    for (let i = 0; i < newChildren.length; i++) {
+      const child = newChildren[i];
+      if (i < existingChildren.length) {
+        // Update existing child
+        (existingChildren[i] as any)['component'] = child;
+      } else {
+        // Add new child
+        const childElement = document.createElement(
+          COMPONENT_RENDERER_ELEMENT_NAME,
+        );
+        (childElement as any)['component'] = child;
+        this.customElement!.appendChild(childElement);
+      }
+    }
+
+    // Remove excess children
+    for (let i = newChildren.length; i < existingChildren.length; i++) {
+      this.customElement!.removeChild(existingChildren[i]);
+    }
+  }
+
   ngDoCheck() {
     // Only need to re-compute styles in editor mode to properly
     // show focused component highlight.

playwright.config.ts

@@ -10,7 +10,7 @@ import {defineConfig, devices} from '@playwright/test';
  * See https://playwright.dev/docs/test-configuration.
  */
 export default defineConfig({
-  timeout: process.env.CI ? 75_000 : 30_000, // Budget more time for CI since tests run slower there.
+  timeout: process.env.CI ? 75_000 : 40_000, // Budget more time for CI since tests run slower there.
   testDir: '.',
   // Use a custom snapshot path template because Playwright's default
   // is platform-specific which isn't necessary for Mesop e2e tests