fix(core): initialize slotcontroller map after update

.changeset/fresh-things-burn.md

@@ -0,0 +1,6 @@
+---
+"@patternfly/pfe-core": patch
+---
+
+`SlotController`: correctly report slot content after updating
+

core/pfe-core/controllers/slot-controller.ts

@@ -38,16 +38,16 @@ export function isObjectSpread(config: SlotControllerArgs): config is [SlotsConf
   return config.length === 1 && typeof config[0] === 'object' && config[0] !== null;
 }
 
-/**
- * If it's a named slot, return its children,
- * for the default slot, look for direct children not assigned to a slot
- * @param n slot name
- */
-const isSlot =
-  <T extends Element = Element>(n: string | typeof SlotController.default) =>
-    (child: Element): child is T =>
-        n === SlotController.default ? !child.hasAttribute('slot')
-      : child.getAttribute('slot') === n;
+function isContent(node: Node) {
+  switch (node.nodeType) {
+    case Node.TEXT_NODE:
+      return !!node.textContent?.trim();
+    case Node.COMMENT_NODE:
+      return false;
+    default:
+      return true;
+  }
+}
 
 export declare class SlotControllerPublicAPI implements ReactiveController {
   static default: symbol;
@@ -98,25 +98,67 @@ export declare class SlotControllerPublicAPI implements ReactiveController {
   isEmpty(...names: (string | null | undefined)[]): boolean;
 }
 
+class SlotRecord {
+  constructor(
+    public slot: HTMLSlotElement,
+    public name: string | symbol,
+    private host: ReactiveElement,
+  ) {}
+
+  get elements() {
+    return this.slot?.assignedElements?.();
+  }
+
+  get hasContent() {
+    if (this.name === SlotController.default) {
+      return !!this.elements.length
+        || !![...this.host.childNodes]
+            .some(node => {
+              if (node instanceof Element) {
+                return !node.hasAttribute('slot');
+              } else {
+                return isContent(node);
+              }
+            });
+    } else {
+      return !!this.slot.assignedNodes()
+          .some(isContent);
+    }
+  }
+}
+
 export class SlotController implements SlotControllerPublicAPI {
   public static default = Symbol('default slot') satisfies symbol as symbol;
 
   /** @deprecated use `default` */
   public static anonymous: symbol = this.default;
 
-  #nodes = new Map<string | typeof SlotController.default, Slot>();
-
-  #slotMapInitialized = false;
+  #slotRecords = new Map<string | typeof SlotController.default, SlotRecord>();
 
-  #slotNames: (string | null)[] = [];
+  #slotNames: (string | symbol | null)[] = [];
 
   #deprecations: Record<string, string> = {};
 
-  #mo = new MutationObserver(this.#initSlotMap.bind(this));
+  #initSlotMap = async () => {
+    const { host } = this;
+    await host.updateComplete;
+    const slotRecords = this.#slotRecords;
+    // Loop over the properties provided by the schema
+    for (let slotName of this.#slotNames.concat(Object.values(this.#deprecations))) {
+      slotName ||= SlotController.default;
+      const slot = this.#getSlotElement(slotName);
+      if (slot) {
+        slotRecords.set(slotName, new SlotRecord(slot, slotName, host));
+      }
+    }
+    host.requestUpdate();
+  };
+
+  #mo = new MutationObserver(this.#initSlotMap);
 
   constructor(public host: ReactiveElement, ...args: SlotControllerArgs) {
-    this.#initialize(...args);
     host.addController(this);
+    this.#initialize(...args);
     if (!this.#slotNames.length) {
       this.#slotNames = [null];
     }
@@ -133,59 +175,27 @@ export class SlotController implements SlotControllerPublicAPI {
     }
   }
 
+  #getSlotElement(slotId: string | symbol) {
+    const selector =
+      slotId === SlotController.default ? 'slot:not([name])' : `slot[name="${slotId as string}"]`;
+    return this.host.shadowRoot?.querySelector?.<HTMLSlotElement>(selector) ?? null;
+  }
+
   async hostConnected(): Promise<void> {
     this.#mo.observe(this.host, { childList: true });
     // Map the defined slots into an object that is easier to query
-    this.#nodes.clear();
+    this.#slotRecords.clear();
+    await this.host.updateComplete;
     this.#initSlotMap();
     // insurance for framework integrations
     await this.host.updateComplete;
     this.host.requestUpdate();
   }
 
-  hostUpdated(): void {
-    if (!this.#slotMapInitialized) {
-      this.#initSlotMap();
-    }
-  }
-
   hostDisconnected(): void {
     this.#mo.disconnect();
   }
 
-  #initSlotMap() {
-    // Loop over the properties provided by the schema
-    for (const slotName of this.#slotNames
-        .concat(Object.values(this.#deprecations))) {
-      const slotId = slotName || SlotController.default;
-      const name = slotName ?? '';
-      const elements = this.#getChildrenForSlot(slotId);
-      const slot = this.#getSlotElement(slotId);
-      const hasContent =
-        !!elements.length || !!slot?.assignedNodes?.()?.filter(x => x.textContent?.trim()).length;
-      this.#nodes.set(slotId, { elements, name, hasContent, slot });
-    }
-    this.host.requestUpdate();
-    this.#slotMapInitialized = true;
-  }
-
-  #getSlotElement(slotId: string | symbol) {
-    const selector =
-      slotId === SlotController.default ? 'slot:not([name])' : `slot[name="${slotId as string}"]`;
-    return this.host.shadowRoot?.querySelector?.<HTMLSlotElement>(selector) ?? null;
-  }
-
-  #getChildrenForSlot<T extends Element = Element>(
-    name: string | typeof SlotController.default,
-  ): T[] {
-    if (this.#nodes.has(name)) {
-      return (this.#nodes.get(name)!.slot?.assignedElements?.() ?? []) as T[];
-    } else {
-      const children = Array.from(this.host.children) as T[];
-      return children.filter(isSlot(name));
-    }
-  }
-
   /**
    * Given a slot name or slot names, returns elements assigned to the requested slots as an array.
    * If no value is provided, it returns all children not assigned to a slot (without a slot attribute).
@@ -203,12 +213,12 @@ export class SlotController implements SlotControllerPublicAPI {
    *          this.getSlotted();
    *          ```
    */
-  getSlotted<T extends Element = Element>(...slotNames: string[]): T[] {
-    if (!slotNames.length) {
-      return (this.#nodes.get(SlotController.default)?.elements ?? []) as T[];
+  public getSlotted<T extends Element = Element>(...slotNames: string[] | [null]): T[] {
+    if (!slotNames.length || slotNames.length === 1 && slotNames.at(0) === null) {
+      return (this.#slotRecords.get(SlotController.default)?.elements ?? []) as T[];
     } else {
       return slotNames.flatMap(slotName =>
-        this.#nodes.get(slotName)?.elements ?? []) as T[];
+        this.#slotRecords.get(slotName ?? SlotController.default)?.elements ?? []) as T[];
     }
   }
 
@@ -217,12 +227,20 @@ export class SlotController implements SlotControllerPublicAPI {
    * @param names The slot names to check.
    * @example this.hasSlotted('header');
    */
-  hasSlotted(...names: (string | null | undefined)[]): boolean {
-    const slotNames = Array.from(names, x => x == null ? SlotController.default : x);
+  public hasSlotted(...names: (string | null | undefined)[]): boolean {
+    const slotNames = Array.from(names, x =>
+      x == null ? SlotController.default : x);
     if (!slotNames.length) {
       slotNames.push(SlotController.default);
     }
-    return slotNames.some(x => this.#nodes.get(x)?.hasContent ?? false);
+    return slotNames.some(slotName => {
+      const slot = this.#slotRecords.get(slotName);
+      if (!slot) {
+        return false;
+      } else {
+        return slot.hasContent;
+      }
+    });
   }
 
   /**
@@ -232,7 +250,7 @@ export class SlotController implements SlotControllerPublicAPI {
    * @example this.isEmpty();
    * @returns
    */
-  isEmpty(...names: (string | null | undefined)[]): boolean {
+  public isEmpty(...names: (string | null | undefined)[]): boolean {
     return !this.hasSlotted(...names);
   }
 }

core/pfe-core/controllers/test/slot-controller.spec.ts

@@ -0,0 +1,148 @@
+import { expect, fixture, nextFrame } from '@open-wc/testing';
+
+import { customElement } from 'lit/decorators/custom-element.js';
+import { LitElement, html, type TemplateResult } from 'lit';
+
+import { SlotController } from '../slot-controller.js';
+
+@customElement('test-slot-controller-with-named-and-anonymous')
+class TestSlotControllerWithNamedAndAnonymous extends LitElement {
+  controller = new SlotController(this, 'a', null);
+  render(): TemplateResult {
+    return html`
+      <slot name="a"></slot>
+      <slot name="b"></slot>
+      <slot></slot>
+    `;
+  }
+}
+
+describe('SlotController', function() {
+  describe('with named and anonymous slots', function() {
+    describe('with no content', function() {
+      let element: TestSlotControllerWithNamedAndAnonymous;
+      beforeEach(async function() {
+        element = await fixture(html`
+          <test-slot-controller-with-named-and-anonymous></test-slot-controller-with-named-and-anonymous>
+        `);
+      });
+      it('reports empty named slots', function() {
+        expect(element.controller.hasSlotted('a')).to.be.false;
+        expect(element.controller.isEmpty('a')).to.be.true;
+      });
+      it('reports empty default slot', function() {
+        expect(element.controller.hasSlotted(null)).to.be.false;
+        expect(element.controller.isEmpty(null)).to.be.true;
+      });
+      it('reports empty default slot with no arguments', function() {
+        expect(element.controller.hasSlotted()).to.be.false;
+        expect(element.controller.isEmpty()).to.be.true;
+      });
+      it('returns empty list for getSlotted("a")', function() {
+        expect(element.controller.getSlotted('a')).to.be.empty;
+      });
+      it('returns empty list for getSlotted(null)', function() {
+        expect(element.controller.getSlotted(null)).to.be.empty;
+      });
+      it('returns empty list for getSlotted()', function() {
+        expect(element.controller.getSlotted()).to.be.empty;
+      });
+    });
+
+    describe('with element content in default slot', function() {
+      let element: TestSlotControllerWithNamedAndAnonymous;
+      beforeEach(async function() {
+        element = await fixture(html`
+          <test-slot-controller-with-named-and-anonymous>
+            <p>element</p>
+          </test-slot-controller-with-named-and-anonymous>
+        `);
+      });
+      it('reports empty named slots', function() {
+        expect(element.controller.hasSlotted('a')).to.be.false;
+        expect(element.controller.isEmpty('a')).to.be.true;
+      });
+      it('reports non-empty default slot', function() {
+        expect(element.controller.hasSlotted(null)).to.be.true;
+        expect(element.controller.isEmpty(null)).to.be.false;
+      });
+      it('reports non-empty default slot with no arguments', function() {
+        expect(element.controller.hasSlotted()).to.be.true;
+        expect(element.controller.isEmpty()).to.be.false;
+      });
+      it('returns empty list for getSlotted("a")', function() {
+        expect(element.controller.getSlotted('a')).to.be.empty;
+      });
+      it('returns lengthy list for getSlotted(null)', function() {
+        expect(element.controller.getSlotted(null)).to.not.be.empty;
+      });
+      it('returns lengthy list for getSlotted()', function() {
+        expect(element.controller.getSlotted()).to.not.be.empty;
+      });
+    });
+
+    describe('with element content in named slot', function() {
+      let element: TestSlotControllerWithNamedAndAnonymous;
+      beforeEach(async function() {
+        element = await fixture(html`
+          <test-slot-controller-with-named-and-anonymous>
+            <p slot="a">element</p>
+          </test-slot-controller-with-named-and-anonymous>
+        `);
+      });
+      it('reports non-empty named slots', function() {
+        expect(element.controller.hasSlotted('a')).to.be.true;
+        expect(element.controller.isEmpty('a')).to.be.false;
+      });
+      it('reports empty default slot', function() {
+        expect(element.controller.hasSlotted(null)).to.be.false;
+        expect(element.controller.isEmpty(null)).to.be.true;
+      });
+      it('reports empty default slot with no arguments', function() {
+        expect(element.controller.hasSlotted()).to.be.false;
+        expect(element.controller.isEmpty()).to.be.true;
+      });
+      it('returns lengthy list for getSlotted("a")', function() {
+        expect(element.controller.getSlotted('a')).to.not.be.empty;
+      });
+      it('returns empty list for getSlotted(null)', function() {
+        expect(element.controller.getSlotted(null)).to.be.empty;
+      });
+      it('returns empty list for getSlotted()', function() {
+        expect(element.controller.getSlotted()).to.be.empty;
+      });
+    });
+
+    describe('with text content in default slot', function() {
+      let element: TestSlotControllerWithNamedAndAnonymous;
+      beforeEach(async function() {
+        element = await fixture(html`
+          <test-slot-controller-with-named-and-anonymous>
+            text
+          </test-slot-controller-with-named-and-anonymous>
+        `);
+      });
+      it('reports empty named slots', function() {
+        expect(element.controller.hasSlotted('a')).to.be.false;
+        expect(element.controller.isEmpty('a')).to.be.true;
+      });
+      it('reports non-empty default slot', function() {
+        expect(element.controller.hasSlotted(null)).to.be.true;
+        expect(element.controller.isEmpty(null)).to.be.false;
+      });
+      it('reports non-empty default slot with no arguments', function() {
+        expect(element.controller.hasSlotted()).to.be.true;
+        expect(element.controller.isEmpty()).to.be.false;
+      });
+      it('returns empty list for getSlotted("a")', function() {
+        expect(element.controller.getSlotted('a')).to.be.empty;
+      });
+      it('returns lengthy list for getSlotted(null)', function() {
+        expect(element.controller.getSlotted(null)).to.be.empty;
+      });
+      it('returns lengthy list for getSlotted()', function() {
+        expect(element.controller.getSlotted()).to.be.empty;
+      });
+    });
+  });
+});