bug #1392 [LiveComponent] Fix loadingDirectives match elements from nested components (smnandre)

weaverryan · weaverryan · commit ed578041b5cc · 2024-01-30T10:36:13.000-05:00
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Fix loadingDirectives match elements from nested components | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Issues | Fix #1382 | License | MIT The elements with loading directives were used in all LiveController they were in. This PR fixes that and restrict loadingDirective to the scope of the LiveContoller (and filter those belonging to nested / child components) Commits ------- 7867e44 [LiveComponent] Fix loadingDirectives match elements from nested components
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
@@ -2370,27 +2370,27 @@ class StandardElementDriver {
 class LoadingPlugin {
     attachToComponent(component) {
         component.on('loading.state:started', (element, request) => {
-            this.startLoading(element, request);
+            this.startLoading(component, element, request);
         });
         component.on('loading.state:finished', (element) => {
-            this.finishLoading(element);
+            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']);
             }
@@ -2474,9 +2474,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
`@@ -2370,27 +2370,27 @@ class StandardElementDriver {`
`2370`	`2370`	`class LoadingPlugin {`
`2371`	`2371`	`attachToComponent(component) {`
`2372`	`2372`	`component.on('loading.state:started', (element, request) => {`
`2373`		`- this.startLoading(element, request);`
	`2373`	`+ this.startLoading(component, element, request);`
`2374`	`2374`	`});`
`2375`	`2375`	`component.on('loading.state:finished', (element) => {`
`2376`		`- this.finishLoading(element);`
	`2376`	`+ this.finishLoading(component, element);`
`2377`	`2377`	`});`
`2378`		`- this.finishLoading(component.element);`
	`2378`	`+ this.finishLoading(component, component.element);`
`2379`	`2379`	`}`
`2380`		`- startLoading(targetElement, backendRequest) {`
`2381`		`- this.handleLoadingToggle(true, targetElement, backendRequest);`
	`2380`	`+ startLoading(component, targetElement, backendRequest) {`
	`2381`	`+ this.handleLoadingToggle(component, true, targetElement, backendRequest);`
`2382`	`2382`	`}`
`2383`		`- finishLoading(targetElement) {`
`2384`		`- this.handleLoadingToggle(false, targetElement, null);`
	`2383`	`+ finishLoading(component, targetElement) {`
	`2384`	`+ this.handleLoadingToggle(component, false, targetElement, null);`
`2385`	`2385`	`}`
`2386`		`- handleLoadingToggle(isLoading, targetElement, backendRequest) {`
	`2386`	`+ handleLoadingToggle(component, isLoading, targetElement, backendRequest) {`
`2387`	`2387`	`if (isLoading) {`
`2388`	`2388`	`this.addAttributes(targetElement, ['busy']);`
`2389`	`2389`	`}`
`2390`	`2390`	`else {`
`2391`	`2391`	`this.removeAttributes(targetElement, ['busy']);`
`2392`	`2392`	`}`
`2393`		`- this.getLoadingDirectives(targetElement).forEach(({ element, directives }) => {`
	`2393`	`+ this.getLoadingDirectives(component, targetElement).forEach(({ element, directives }) => {`
`2394`	`2394`	`if (isLoading) {`
`2395`	`2395`	`this.addAttributes(element, ['data-live-is-loading']);`
`2396`	`2396`	`}`
`@@ -2474,9 +2474,10 @@ class LoadingPlugin {`
`2474`	`2474`	`}`
`2475`	`2475`	`loadingDirective();`
`2476`	`2476`	`}`
`2477`		`- getLoadingDirectives(element) {`
	`2477`	`+ getLoadingDirectives(component, element) {`
`2478`	`2478`	`const loadingDirectives = [];`
`2479`		`- let matchingElements = element.querySelectorAll('[data-loading]');`
	`2479`	`+ let matchingElements = [...element.querySelectorAll('[data-loading]')];`
	`2480`	`+ matchingElements = matchingElements.filter((elt) => elementBelongsToThisComponent(elt, component));`
`2480`	`2481`	`if (element.hasAttribute('data-loading')) {`
`2481`	`2482`	`matchingElements = [element, ...matchingElements];`
`2482`	`2483`	`}`