Fix loadingDirectives match elements from nested components

smnandre · smnandre · commit 506c2d1697ad · 2024-01-30T16:02:04.000+01:00
diff --git a/src/LiveComponent/assets/dist/Component/plugins/LoadingPlugin.d.ts b/src/LiveComponent/assets/dist/Component/plugins/LoadingPlugin.d.ts
@@ -8,11 +8,11 @@ interface ElementLoadingDirectives {
 }
 export default class implements PluginInterface {
     attachToComponent(component: Component): void;
-    startLoading(targetElement: HTMLElement | SVGElement, backendRequest: BackendRequest): void;
-    finishLoading(targetElement: HTMLElement | SVGElement): void;
+    startLoading(component: Component, targetElement: HTMLElement | SVGElement, backendRequest: BackendRequest): void;
+    finishLoading(component: Component, targetElement: HTMLElement | SVGElement): void;
     private handleLoadingToggle;
     private handleLoadingDirective;
-    getLoadingDirectives(element: HTMLElement | SVGElement): ElementLoadingDirectives[];
+    getLoadingDirectives(component: Component, element: HTMLElement | SVGElement): ElementLoadingDirectives[];
     private showElement;
     private hideElement;
     private addClass;
diff --git a/src/LiveComponent/assets/dist/live_controller.js b/src/LiveComponent/assets/dist/live_controller.js
@@ -1948,7 +1948,7 @@ class Component {
             }
         }
         this.backendRequest = this.backend.makeRequest(this.valueStore.getOriginalProps(), this.pendingActions, this.valueStore.getDirtyProps(), this.getChildrenFingerprints(), this.valueStore.getUpdatedPropsFromParent(), filesToSend);
-        this.hooks.triggerHook('loading.state:started', this.element, this.backendRequest);
+        this.hooks.triggerHook('loading.state:started', this.element, this.backendRequest, this);
         this.pendingActions = [];
         this.valueStore.flushDirtyPropsToPending();
         this.isRequestPending = false;
@@ -1995,7 +1995,7 @@ class Component {
             }
             return;
         }
-        this.hooks.triggerHook('loading.state:finished', this.element);
+        this.hooks.triggerHook('loading.state:finished', this.element, this);
         const modifiedModelValues = {};
         Object.keys(this.valueStore.getDirtyProps()).forEach((modelName) => {
             modifiedModelValues[modelName] = this.valueStore.get(modelName);
@@ -2296,28 +2296,28 @@ class StandardElementDriver {
 
 class LoadingPlugin {
     attachToComponent(component) {
-        component.on('loading.state:started', (element, request) => {
-            this.startLoading(element, request);
+        component.on('loading.state:started', (element, request, component) => {
+            this.startLoading(component, element, request);
         });
-        component.on('loading.state:finished', (element) => {
-            this.finishLoading(element);
+        component.on('loading.state:finished', (element, component) => {
+            this.finishLoading(component, element);
         });
-        this.finishLoading(component.element);
+        this.finishLoading(component, component.element);
     }
-    startLoading(targetElement, backendRequest) {
-        this.handleLoadingToggle(true, targetElement, backendRequest);
+    startLoading(component, targetElement, backendRequest) {
+        this.handleLoadingToggle(component, true, targetElement, backendRequest);
     }
-    finishLoading(targetElement) {
-        this.handleLoadingToggle(false, targetElement, null);
+    finishLoading(component, targetElement) {
+        this.handleLoadingToggle(component, false, targetElement, null);
     }
-    handleLoadingToggle(isLoading, targetElement, backendRequest) {
+    handleLoadingToggle(component, isLoading, targetElement, backendRequest) {
         if (isLoading) {
             this.addAttributes(targetElement, ['busy']);
         }
         else {
             this.removeAttributes(targetElement, ['busy']);
         }
-        this.getLoadingDirectives(targetElement).forEach(({ element, directives }) => {
+        this.getLoadingDirectives(component, targetElement).forEach(({ element, directives }) => {
             if (isLoading) {
                 this.addAttributes(element, ['data-live-is-loading']);
             }
@@ -2401,9 +2401,10 @@ class LoadingPlugin {
         }
         loadingDirective();
     }
-    getLoadingDirectives(element) {
+    getLoadingDirectives(component, element) {
         const loadingDirectives = [];
-        let matchingElements = element.querySelectorAll('[data-loading]');
+        let matchingElements = [...element.querySelectorAll('[data-loading]')];
+        matchingElements = matchingElements.filter((elt) => elementBelongsToThisComponent(elt, component));
         if (element.hasAttribute('data-loading')) {
             matchingElements = [element, ...matchingElements];
         }
diff --git a/src/LiveComponent/assets/src/Component/plugins/LoadingPlugin.ts b/src/LiveComponent/assets/src/Component/plugins/LoadingPlugin.ts
@@ -3,6 +3,7 @@ import {
     DirectiveModifier,
     parseDirectives
 } from '../../Directive/directives_parser';
+import { elementBelongsToThisComponent } from '../../dom_utils';
 import { combineSpacedArray } from '../../string_utils';
 import BackendRequest from '../../Backend/BackendRequest';
 import Component from '../../Component';
@@ -16,32 +17,32 @@ interface ElementLoadingDirectives {
 export default class implements PluginInterface {
     attachToComponent(component: Component): void {
         component.on('loading.state:started', (element: HTMLElement, request: BackendRequest) => {
-            this.startLoading(element, request);
+            this.startLoading(component, element, request);
         });
         component.on('loading.state:finished', (element: HTMLElement) => {
-            this.finishLoading(element);
+            this.finishLoading(component, element);
         });
         // hide "loading" elements to begin with
         // This is done with CSS, but only for the most basic cases
-        this.finishLoading(component.element);
+        this.finishLoading(component, component.element);
     }
 
-    startLoading(targetElement: HTMLElement|SVGElement, backendRequest: BackendRequest): void {
-        this.handleLoadingToggle(true, targetElement, backendRequest);
+    startLoading(component: Component, targetElement: HTMLElement|SVGElement, backendRequest: BackendRequest): void {
+        this.handleLoadingToggle(component, true, targetElement, backendRequest);
     }
 
-    finishLoading(targetElement: HTMLElement|SVGElement): void {
-        this.handleLoadingToggle(false, targetElement, null);
+    finishLoading(component: Component, targetElement: HTMLElement|SVGElement): void {
+        this.handleLoadingToggle(component,false, targetElement, null);
     }
 
-    private handleLoadingToggle(isLoading: boolean, targetElement: HTMLElement|SVGElement, backendRequest: BackendRequest|null) {
+    private handleLoadingToggle(component: Component, isLoading: boolean, targetElement: HTMLElement|SVGElement, backendRequest: BackendRequest|null) {
         if (isLoading) {
             this.addAttributes(targetElement, ['busy']);
         } else {
             this.removeAttributes(targetElement, ['busy']);
         }
 
-        this.getLoadingDirectives(targetElement).forEach(({ element, directives }) => {
+        this.getLoadingDirectives(component, targetElement).forEach(({ element, directives }) => {
             // so we can track, at any point, if an element is in a "loading" state
             if (isLoading) {
                 this.addAttributes(element, ['data-live-is-loading']);
@@ -148,9 +149,12 @@ export default class implements PluginInterface {
         loadingDirective();
     }
 
-    getLoadingDirectives(element: HTMLElement|SVGElement) {
+    getLoadingDirectives(component: Component, element: HTMLElement|SVGElement) {
         const loadingDirectives: ElementLoadingDirectives[] = [];
-        let matchingElements = element.querySelectorAll('[data-loading]');
+        let matchingElements = [...element.querySelectorAll('[data-loading]')];
+
+        // ignore elements which are inside a nested "live" component
+        matchingElements = matchingElements.filter((elt) => elementBelongsToThisComponent(elt, component));
 
         // querySelectorAll doesn't include the element itself
         if (element.hasAttribute('data-loading')) {
diff --git a/src/LiveComponent/assets/test/controller/loading.test.ts b/src/LiveComponent/assets/test/controller/loading.test.ts
@@ -223,4 +223,51 @@ describe('LiveController data-loading Tests', () => {
         await (new Promise(resolve => setTimeout(resolve, 30)));
         expect(getByTestId(test.element, 'loading-element')).not.toBeVisible();
     });
+
+    it('does not trigger loading inside component children', async () => {
+       const childTemplate = (data: any) => `
+            <div ${initComponent(data, {id: 'child-id'})} data-testid="child">
+                <span data-loading="show" data-testid="child-loading-element-showing">Loading...</span>
+                <span data-loading="hide" data-testid="child-loading-element-hiding">Loading...</span>
+            </div>
+        `;
+
+        const test = await createTest({} , (data: any) => `
+            <div ${initComponent(data, {id: 'parent-id'})} data-testid="parent">
+                <span data-loading="show" data-testid="parent-loading-element-showing">Loading...</span>
+                <span data-loading="hide" data-testid="parent-loading-element-hiding">Loading...</span>
+                ${childTemplate({})}
+                <button data-action="live#$render">Render</button>
+            </div>
+        `);
+
+        test.expectsAjaxCall()
+            // delay so we can check loading
+            .delayResponse(20);
+
+        // All showing elements should be hidden / hiding elements should be visible
+        await waitFor(() => expect(getByTestId(test.element, 'parent-loading-element-showing')).not.toBeVisible());
+        await waitFor(() => expect(getByTestId(test.element, 'parent-loading-element-hiding')).toBeVisible());
+        await waitFor(() => expect(getByTestId(test.element, 'child-loading-element-showing')).not.toBeVisible());
+        await waitFor(() => expect(getByTestId(test.element, 'child-loading-element-hiding')).toBeVisible());
+
+        getByText(test.element, 'Render').click();
+
+        // Parent: showing elements should be visible / hiding elements should be hidden
+        expect(getByTestId(test.element, 'parent-loading-element-showing')).toBeVisible();
+        expect(getByTestId(test.element, 'parent-loading-element-hiding')).not.toBeVisible();
+        // Child: no change
+        expect(getByTestId(test.element, 'child-loading-element-showing')).not.toBeVisible();
+        expect(getByTestId(test.element, 'child-loading-element-hiding')).toBeVisible();
+
+        // wait for loading to finish
+        await (new Promise(resolve => setTimeout(resolve, 30)));
+
+        // Parent: back to original state
+        expect(getByTestId(test.element, 'parent-loading-element-showing')).not.toBeVisible();
+        expect(getByTestId(test.element, 'parent-loading-element-hiding')).toBeVisible();
+        // Child: no change
+        expect(getByTestId(test.element, 'child-loading-element-showing')).not.toBeVisible();
+        expect(getByTestId(test.element, 'child-loading-element-hiding')).toBeVisible();
+    });
 });
diff --git a/yarn.lock b/yarn.lock

Original file line number	Diff line number	Diff line change
`@@ -1948,7 +1948,7 @@ class Component {`
`1948`	`1948`	`}`
`1949`	`1949`	`}`
`1950`	`1950`	`this.backendRequest = this.backend.makeRequest(this.valueStore.getOriginalProps(), this.pendingActions, this.valueStore.getDirtyProps(), this.getChildrenFingerprints(), this.valueStore.getUpdatedPropsFromParent(), filesToSend);`
`1951`		`- this.hooks.triggerHook('loading.state:started', this.element, this.backendRequest);`
	`1951`	`+ this.hooks.triggerHook('loading.state:started', this.element, this.backendRequest, this);`
`1952`	`1952`	`this.pendingActions = [];`
`1953`	`1953`	`this.valueStore.flushDirtyPropsToPending();`
`1954`	`1954`	`this.isRequestPending = false;`
`@@ -1995,7 +1995,7 @@ class Component {`
`1995`	`1995`	`}`
`1996`	`1996`	`return;`
`1997`	`1997`	`}`
`1998`		`- this.hooks.triggerHook('loading.state:finished', this.element);`
	`1998`	`+ this.hooks.triggerHook('loading.state:finished', this.element, this);`
`1999`	`1999`	`const modifiedModelValues = {};`
`2000`	`2000`	`Object.keys(this.valueStore.getDirtyProps()).forEach((modelName) => {`
`2001`	`2001`	`modifiedModelValues[modelName] = this.valueStore.get(modelName);`
`@@ -2296,28 +2296,28 @@ class StandardElementDriver {`
`2296`	`2296`
`2297`	`2297`	`class LoadingPlugin {`
`2298`	`2298`	`attachToComponent(component) {`
`2299`		`- component.on('loading.state:started', (element, request) => {`
`2300`		`- this.startLoading(element, request);`
	`2299`	`+ component.on('loading.state:started', (element, request, component) => {`
	`2300`	`+ this.startLoading(component, element, request);`
`2301`	`2301`	`});`
`2302`		`- component.on('loading.state:finished', (element) => {`
`2303`		`- this.finishLoading(element);`
	`2302`	`+ component.on('loading.state:finished', (element, component) => {`
	`2303`	`+ this.finishLoading(component, element);`
`2304`	`2304`	`});`
`2305`		`- this.finishLoading(component.element);`
	`2305`	`+ this.finishLoading(component, component.element);`
`2306`	`2306`	`}`
`2307`		`- startLoading(targetElement, backendRequest) {`
`2308`		`- this.handleLoadingToggle(true, targetElement, backendRequest);`
	`2307`	`+ startLoading(component, targetElement, backendRequest) {`
	`2308`	`+ this.handleLoadingToggle(component, true, targetElement, backendRequest);`
`2309`	`2309`	`}`
`2310`		`- finishLoading(targetElement) {`
`2311`		`- this.handleLoadingToggle(false, targetElement, null);`
	`2310`	`+ finishLoading(component, targetElement) {`
	`2311`	`+ this.handleLoadingToggle(component, false, targetElement, null);`
`2312`	`2312`	`}`
`2313`		`- handleLoadingToggle(isLoading, targetElement, backendRequest) {`
	`2313`	`+ handleLoadingToggle(component, isLoading, targetElement, backendRequest) {`
`2314`	`2314`	`if (isLoading) {`
`2315`	`2315`	`this.addAttributes(targetElement, ['busy']);`
`2316`	`2316`	`}`
`2317`	`2317`	`else {`
`2318`	`2318`	`this.removeAttributes(targetElement, ['busy']);`
`2319`	`2319`	`}`
`2320`		`- this.getLoadingDirectives(targetElement).forEach(({ element, directives }) => {`
	`2320`	`+ this.getLoadingDirectives(component, targetElement).forEach(({ element, directives }) => {`
`2321`	`2321`	`if (isLoading) {`
`2322`	`2322`	`this.addAttributes(element, ['data-live-is-loading']);`
`2323`	`2323`	`}`
`@@ -2401,9 +2401,10 @@ class LoadingPlugin {`
`2401`	`2401`	`}`
`2402`	`2402`	`loadingDirective();`
`2403`	`2403`	`}`
`2404`		`- getLoadingDirectives(element) {`
	`2404`	`+ getLoadingDirectives(component, element) {`
`2405`	`2405`	`const loadingDirectives = [];`
`2406`		`- let matchingElements = element.querySelectorAll('[data-loading]');`
	`2406`	`+ let matchingElements = [...element.querySelectorAll('[data-loading]')];`
	`2407`	`+ matchingElements = matchingElements.filter((elt) => elementBelongsToThisComponent(elt, component));`
`2407`	`2408`	`if (element.hasAttribute('data-loading')) {`
`2408`	`2409`	`matchingElements = [element, ...matchingElements];`
`2409`	`2410`	`}`