Repository: symfony/ux
Pull request #1561 (2024-03-01 01:29:36)

[Live] Improving child render handling: avoid removing element from DOM when possible
| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Issues        | Fixes the timer on https://ux.symfony.com/demos/live-memory/ (which currently resets each time you select the first card)
| License       | MIT

Hi!

## TODO

* [x] Test case for child morphing when position is kept (prove that the element is never removed from the DOM)
* [x] Test case for child morphing when child moves, but another element takes its place (prove that the final HTML is the expected).

## Long explanation 😛 

When handling child rendering - and `data-live-preserve` - the goal is to "preserve" the element on the page instead of morphing it from the matching id in the new HTML. This is especially challenging if the "id" element has changed position, requiring an algorithm where we store the "original" elements, clone them (to avoid morphing from mutating the originals) then swap them back in at the correct position after.

However, instead of doing this "replace and swap" in all situations, we now only do it when the matching elements change position. If they do not (if a `data-live-preserve` element's original element and new element are in the same position - which would happen 99% of the time), then avoid the "replace and swap" and simply tell Idiomorph to avoid morphing the element. This handles and edge case where the original element has a CSS transition. Removing it and re-adding it to the DOM - even for a moment - resets/restarts that CSS transition unnecessarily.

Cheers!


Patch:
diff --git a/src/LiveComponent/assets/dist/live_controller.js b/src/LiveComponent/assets/dist/live_controller.js
index 5fc487f4..f4e0f175 100644
--- a/src/LiveComponent/assets/dist/live_controller.js
+++ b/src/LiveComponent/assets/dist/live_controller.js
@@ -1332,7 +1332,21 @@ const syncAttributes = function (fromEl, toEl) {
     }
 };
 function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, getElementValue, externalMutationTracker) {
-    const preservedOriginalElements = [];
+    const originalElementIdsToSwapAfter = [];
+    const originalElementsToPreserve = new Map();
+    const markElementAsNeedingPostMorphSwap = (id, replaceWithClone) => {
+        const oldElement = originalElementsToPreserve.get(id);
+        if (!(oldElement instanceof HTMLElement)) {
+            throw new Error(`Original element with id ${id} not found`);
+        }
+        originalElementIdsToSwapAfter.push(id);
+        if (!replaceWithClone) {
+            return null;
+        }
+        const clonedOldElement = cloneHTMLElement(oldElement);
+        oldElement.replaceWith(clonedOldElement);
+        return clonedOldElement;
+    };
     rootToElement.querySelectorAll('[data-live-preserve]').forEach((newElement) => {
         const id = newElement.id;
         if (!id) {
@@ -1342,10 +1356,8 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements,
         if (!(oldElement instanceof HTMLElement)) {
             throw new Error(`The element with id "${id}" was not found in the original HTML`);
         }
-        const clonedOldElement = cloneHTMLElement(oldElement);
-        preservedOriginalElements.push(oldElement);
-        oldElement.replaceWith(clonedOldElement);
         newElement.removeAttribute('data-live-preserve');
+        originalElementsToPreserve.set(id, oldElement);
         syncAttributes(newElement, oldElement);
     });
     Idiomorph.morph(rootFromElement, rootToElement, {
@@ -1357,6 +1369,17 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements,
                 if (fromEl === rootFromElement) {
                     return true;
                 }
+                if (fromEl.id && originalElementsToPreserve.has(fromEl.id)) {
+                    if (fromEl.id === toEl.id) {
+                        return false;
+                    }
+                    const clonedFromEl = markElementAsNeedingPostMorphSwap(fromEl.id, true);
+                    if (!clonedFromEl) {
+                        throw new Error('missing clone');
+                    }
+                    Idiomorph.morph(clonedFromEl, toEl);
+                    return false;
+                }
                 if (fromEl instanceof HTMLElement && toEl instanceof HTMLElement) {
                     if (typeof fromEl.__x !== 'undefined') {
                         if (!window.Alpine) {
@@ -1406,6 +1429,10 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements,
                 if (!(node instanceof HTMLElement)) {
                     return true;
                 }
+                if (node.id && originalElementsToPreserve.has(node.id)) {
+                    markElementAsNeedingPostMorphSwap(node.id, false);
+                    return true;
+                }
                 if (externalMutationTracker.wasElementAdded(node)) {
                     return false;
                 }
@@ -1413,12 +1440,13 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements,
             },
         },
     });
-    preservedOriginalElements.forEach((oldElement) => {
-        const newElement = rootFromElement.querySelector(`#${oldElement.id}`);
-        if (!(newElement instanceof HTMLElement)) {
-            throw new Error('Missing preserved element');
+    originalElementIdsToSwapAfter.forEach((id) => {
+        const newElement = rootFromElement.querySelector(`#${id}`);
+        const originalElement = originalElementsToPreserve.get(id);
+        if (!(newElement instanceof HTMLElement) || !(originalElement instanceof HTMLElement)) {
+            throw new Error('Missing elements.');
         }
-        newElement.replaceWith(oldElement);
+        newElement.replaceWith(originalElement);
     });
 }
 
diff --git a/src/LiveComponent/assets/src/morphdom.ts b/src/LiveComponent/assets/src/morphdom.ts
index d56f0f45..12fab827 100644
--- a/src/LiveComponent/assets/src/morphdom.ts
+++ b/src/LiveComponent/assets/src/morphdom.ts
@@ -27,14 +27,48 @@ export function executeMorphdom(
      * To handle them, we:
      *  1) Create an array of the "current" HTMLElements that match each
      *     "data-live-preserve" element.
-     *  2) Replace the "current" elements with clones so that the originals
-     *     aren't modified during the morphing process.
-     *  3) After the morphing is complete, we find the preserved elements and
-     *     replace them with the originals.
+     *  2) If we detect that a "current" preserved element is about to be morphed
+     *     (updated or removed), replace the "current" element with a clone so
+     *     the original isn't modified during the morphing process. Mark that
+     *     this element needs to be replaced after the morphing is complete.
+     *  3) After the morphing is complete, loop over the elements that were
+     *     replaced and swap the original element back into the new position.
+     *
+     * This allows Idiomorph to potentially morph the position of the preserved
+     * elements... but still allowing us to make sure that the element in the
+     * new position is exactly the original HTMLElement.
+     */
+    const originalElementIdsToSwapAfter: Array<string> = [];
+    const originalElementsToPreserve = new Map<string, HTMLElement>();
+
+    /**
+     * Called when a preserved element is about to be morphed.
+     *
+     * Instead of allowing the original to be morphed, a fake clone
+     * is created and morphed instead. The original is then marked
+     * to be replaced after the morph with wherever the final
+     * matching id element ends up.
      */
-    const preservedOriginalElements: HTMLElement[] = [];
+    const markElementAsNeedingPostMorphSwap = (id: string, replaceWithClone: boolean): HTMLElement | null => {
+        const oldElement = originalElementsToPreserve.get(id);
+        if (!(oldElement instanceof HTMLElement)) {
+            throw new Error(`Original element with id ${id} not found`);
+        }
+
+        originalElementIdsToSwapAfter.push(id);
+        if (!replaceWithClone) {
+            return null;
+        }
+
+        const clonedOldElement = cloneHTMLElement(oldElement);
+        oldElement.replaceWith(clonedOldElement);
+
+        return clonedOldElement;
+    };
+
     rootToElement.querySelectorAll('[data-live-preserve]').forEach((newElement) => {
         const id = newElement.id;
+
         if (!id) {
             throw new Error('The data-live-preserve attribute requires an id attribute to be set on the element');
         }
@@ -44,11 +78,8 @@ export function executeMorphdom(
             throw new Error(`The element with id "${id}" was not found in the original HTML`);
         }
 
-        const clonedOldElement = cloneHTMLElement(oldElement);
-        preservedOriginalElements.push(oldElement);
-        oldElement.replaceWith(clonedOldElement);
-
         newElement.removeAttribute('data-live-preserve');
+        originalElementsToPreserve.set(id, oldElement);
         syncAttributes(newElement, oldElement);
     });
 
@@ -64,6 +95,27 @@ export function executeMorphdom(
                     return true;
                 }
 
+                if (fromEl.id && originalElementsToPreserve.has(fromEl.id)) {
+                    if (fromEl.id === toEl.id) {
+                        // the preserved elements match, prevent morph and
+                        // keep the original element
+                        return false;
+                    }
+
+                    // a preserved element is being morphed into something else
+                    // this means that preserved element is being moved
+                    // to avoid the original element being morphed, we swap
+                    // it for a clone, manually morph the clone, and then
+                    // skip trying to morph the original element (we want it untouched)
+                    const clonedFromEl = markElementAsNeedingPostMorphSwap(fromEl.id, true);
+                    if (!clonedFromEl) {
+                        throw new Error('missing clone');
+                    }
+                    Idiomorph.morph(clonedFromEl, toEl);
+
+                    return false;
+                }
+
                 // skip special checking if this is, for example, an SVG
                 if (fromEl instanceof HTMLElement && toEl instanceof HTMLElement) {
                     // We assume fromEl is an Alpine component if it has `__x` property.
@@ -168,6 +220,18 @@ export function executeMorphdom(
                     return true;
                 }
 
+                if (node.id && originalElementsToPreserve.has(node.id)) {
+                    // a preserved element is being removed
+                    // to avoid the original element being destroyed (but still
+                    // allowing this spot on the dom to be removed),
+                    // clone the original element and place it into the
+                    // new position after morphing
+                    markElementAsNeedingPostMorphSwap(node.id, false);
+
+                    // allow this to be morphed to the new element
+                    return true;
+                }
+
                 if (externalMutationTracker.wasElementAdded(node)) {
                     // this element was added by an external mutation, so we don't want to discard it
                     return false;
@@ -178,14 +242,13 @@ export function executeMorphdom(
         },
     });
 
-    preservedOriginalElements.forEach((oldElement) => {
-        const newElement = rootFromElement.querySelector(`#${oldElement.id}`);
-        if (!(newElement instanceof HTMLElement)) {
-            // should not happen, as preservedOriginalElements is built from
-            // the new HTML
-            throw new Error('Missing preserved element');
+    originalElementIdsToSwapAfter.forEach((id: string) => {
+        const newElement = rootFromElement.querySelector(`#${id}`);
+        const originalElement = originalElementsToPreserve.get(id);
+        if (!(newElement instanceof HTMLElement) || !(originalElement instanceof HTMLElement)) {
+            // should not happen
+            throw new Error('Missing elements.');
         }
-
-        newElement.replaceWith(oldElement);
+        newElement.replaceWith(originalElement);
     });
 }


Test patch:
diff --git a/src/LiveComponent/assets/test/controller/child.test.ts b/src/LiveComponent/assets/test/controller/child.test.ts
index aa9888a9..7f7f3cf6 100644
--- a/src/LiveComponent/assets/test/controller/child.test.ts
+++ b/src/LiveComponent/assets/test/controller/child.test.ts
@@ -16,10 +16,12 @@ import {
     shutdownTests,
     getComponent,
     dataToJsonAttribute,
+    getStimulusApplication
 } from '../tools';
 import { getByTestId, waitFor } from '@testing-library/dom';
 import userEvent from '@testing-library/user-event';
 import { findChildren } from '../../src/ComponentRegistry';
+import { Controller } from '@hotwired/stimulus';
 
 describe('Component parent -> child initialization and rendering tests', () => {
     afterEach(() => {
@@ -128,6 +130,74 @@ describe('Component parent -> child initialization and rendering tests', () => {
         expect(test.element).not.toContainHTML('data-live-preserve');
     });
 
+    it('data-live-preserve child in same location is not removed/re-added to the DOM', async () => {
+        const originalChild = `
+            <div ${initComponent({}, {id: 'the-child-id'})}>
+                <div data-controller="track-connect"></div>
+                Original Child Component
+            </div>
+        `;
+        const updatedChild = `
+            <div id="the-child-id" data-live-preserve></div>
+        `;
+
+        const test = await createTest({useOriginalChild: true}, (data: any) => `
+           <div ${initComponent(data)}>
+               ${data.useOriginalChild ? originalChild : updatedChild}
+           </div>
+       `);
+
+        getStimulusApplication().register('track-connect', class extends Controller {
+            disconnect() {
+                this.element.setAttribute('disconnected', '');
+            }
+        });
+
+        test.expectsAjaxCall()
+            .serverWillChangeProps((data: any) => {
+                data.useOriginalChild = false;
+            });
+
+        await test.component.render();
+        // sanity check that the child is there
+        expect(test.element).toHaveTextContent('Original Child Component');
+        // check that the element was never disconnected/removed from the DOM
+        expect(test.element).not.toContainHTML('disconnected');
+    });
+
+    it('data-live-preserve element moved correctly when position changes and old element morphed into different element', async () => {
+        const originalChild = `
+            <div ${initComponent({}, {id: 'the-child-id'})} data-testid="child-component">
+                <div data-controller="track-connect"></div>
+                Original Child Component
+            </div>
+        `;
+        const updatedChild = `
+            <div id="the-child-id" data-live-preserve></div>
+        `;
+
+        // when morphing original -> updated, the outer div (which was the child)
+        // will be morphed into a normal div
+        const test = await createTest({useOriginalChild: true}, (data: any) => `
+           <div ${initComponent(data)}>
+               ${data.useOriginalChild ? originalChild : ''}
+               ${data.useOriginalChild ? '' : `<div class="wrapper">${updatedChild}</div>`}
+           </div>
+       `)
+
+        test.expectsAjaxCall()
+            .serverWillChangeProps((data: any) => {
+                data.useOriginalChild = false;
+            });
+
+        const childElement = getByTestId(test.element, 'child-component');
+        await test.component.render();
+        // sanity check that the child is there
+        expect(test.element).toHaveTextContent('Original Child Component');
+        expect(test.element).toContainHTML('class="wrapper"');
+        expect(childElement.parentElement).toHaveClass('wrapper');
+    });
+
     it('existing child component gets props & triggers re-render', async () => {
         const childTemplate = (data: any) => `
             <div ${initComponent(
@@ -327,10 +397,8 @@ describe('Component parent -> child initialization and rendering tests', () => {
             .willReturn(childTemplate);
 
         // trigger the parent render, which will trigger the children to re-render
-        test.component.render();
+        await test.component.render();
 
-        // wait for parent Ajax call to finish
-        await waitFor(() => expect(test.element).not.toHaveAttribute('busy'));
 
         // wait for child to start and stop loading
         await waitFor(() => expect(getByTestId(test.element, 'child-component-1')).not.toHaveAttribute('busy'));
diff --git a/src/LiveComponent/assets/test/tools.ts b/src/LiveComponent/assets/test/tools.ts
index 4c70596d..e0e17351 100644
--- a/src/LiveComponent/assets/test/tools.ts
+++ b/src/LiveComponent/assets/test/tools.ts
@@ -394,10 +394,14 @@ export async function startStimulus(element: string|HTMLElement) {
 
     return {
         controller,
-        element: controllerElement
+        element: controllerElement,
     }
 }
 
+export const getStimulusApplication = (): Application => {
+    return application;
+}
+
 const getControllerElement = (container: HTMLElement): HTMLElement => {
     if (container.dataset.controller === 'live') {
         return container;