From 80b0a7afef3d7029132376223122f3973467b2ae Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Thu, 10 Apr 2025 11:27:17 +0300 Subject: [PATCH 1/3] fix(core): initialize slotcontroller map after update --- .changeset/fresh-things-burn.md | 6 +++ core/pfe-core/controllers/slot-controller.ts | 49 +++++++++++++------- 2 files changed, 38 insertions(+), 17 deletions(-) create mode 100644 .changeset/fresh-things-burn.md diff --git a/.changeset/fresh-things-burn.md b/.changeset/fresh-things-burn.md new file mode 100644 index 0000000000..76130f10fa --- /dev/null +++ b/.changeset/fresh-things-burn.md @@ -0,0 +1,6 @@ +--- +"@patternfly/pfe-core": patch +--- + +`SlotController`: correctly report slot content after updating + \ No newline at end of file diff --git a/core/pfe-core/controllers/slot-controller.ts b/core/pfe-core/controllers/slot-controller.ts index 3a25bcf2ba..8b1bd6b956 100644 --- a/core/pfe-core/controllers/slot-controller.ts +++ b/core/pfe-core/controllers/slot-controller.ts @@ -38,6 +38,17 @@ export function isObjectSpread(config: SlotControllerArgs): config is [SlotsConf return config.length === 1 && typeof config[0] === 'object' && config[0] !== null; } +function isContent(node: Node) { + switch (node.nodeType) { + case Node.TEXT_NODE: + return !!node.textContent?.trim(); + case Node.COMMENT_NODE: + return false; + default: + return true; + } +} + /** * If it's a named slot, return its children, * for the default slot, look for direct children not assigned to a slot @@ -112,7 +123,27 @@ export class SlotController implements SlotControllerPublicAPI { #deprecations: Record = {}; - #mo = new MutationObserver(this.#initSlotMap.bind(this)); + #initSlotMap = async () => { + const { host } = this; + await host.updateComplete; + const nodes = this.#nodes; + // 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 = + slotId === SlotController.default ? !![...host.childNodes].some(isContent) + : !!slot?.assignedNodes?.().some(isContent); + nodes.set(slotId, { elements, name, hasContent, slot }); + } + host.requestUpdate(); + this.#slotMapInitialized = true; + }; + + #mo = new MutationObserver(this.#initSlotMap); constructor(public host: ReactiveElement, ...args: SlotControllerArgs) { this.#initialize(...args); @@ -153,22 +184,6 @@ export class SlotController implements SlotControllerPublicAPI { 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}"]`; From 966a03b3301545cee97fca7f3b17b3125eb719ce Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Thu, 10 Apr 2025 15:11:16 +0300 Subject: [PATCH 2/3] test(core): slot controller tests wip --- .../controllers/test/slot-controller.spec.ts | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 core/pfe-core/controllers/test/slot-controller.spec.ts diff --git a/core/pfe-core/controllers/test/slot-controller.spec.ts b/core/pfe-core/controllers/test/slot-controller.spec.ts new file mode 100644 index 0000000000..0f040ad4ac --- /dev/null +++ b/core/pfe-core/controllers/test/slot-controller.spec.ts @@ -0,0 +1,120 @@ +import { expect, fixture, nextFrame } from '@open-wc/testing'; + +import { customElement } from 'lit/decorators/custom-element.js'; +import { + ReactiveElement, + html, + render, + type PropertyValues, + type TemplateResult, +} from 'lit'; + +import { SlotController } from '../slot-controller.js'; + +describe('SlotController', function() { + describe('with named and anonymous slots', function() { + @customElement('test-slot-controller-with-named-and-anonymous') + class TestSlotControllerWithNamedAndAnonymous extends ReactiveElement { + declare static template: TemplateResult; + + controller = new SlotController(this, 'a', null); + + render(): TemplateResult { + return html` + + + + `; + } + } + describe('with no content', function() { + let element: TestSlotControllerWithNamedAndAnonymous; + beforeEach(async function() { + element = await fixture(html` + + `); + }); + 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; + }); + }); + + describe('with element content in default slot', function() { + let element: TestSlotControllerWithNamedAndAnonymous; + beforeEach(async function() { + element = await fixture(html` + +

element

+ + `); + }); + 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; + }); + }); + + describe('with element content in named slot', function() { + let element: TestSlotControllerWithNamedAndAnonymous; + beforeEach(async function() { + element = await fixture(html` + +

element

+ + `); + }); + 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; + }); + }); + + describe('with text content in default slot', function() { + let element: TestSlotControllerWithNamedAndAnonymous; + beforeEach(async function() { + element = await fixture(html` + + text + + `); + }); + it('reports empty named slots', function() { + expect(element.controller.hasSlotted('a')).to.be.false; + expect(element.controller.isEmpty('a')).to.be.true; + }); + it('reports nonjgempty 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; + }); + }); + }); +}); From b64a333f6470fd8dd712016ff63e26b9003fd9f3 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Thu, 10 Apr 2025 17:22:58 +0300 Subject: [PATCH 3/3] fix(core): slot controller --- core/pfe-core/controllers/slot-controller.ts | 123 +++++++++--------- .../controllers/test/slot-controller.spec.ts | 72 ++++++---- 2 files changed, 113 insertions(+), 82 deletions(-) diff --git a/core/pfe-core/controllers/slot-controller.ts b/core/pfe-core/controllers/slot-controller.ts index 8b1bd6b956..85cb4d962e 100644 --- a/core/pfe-core/controllers/slot-controller.ts +++ b/core/pfe-core/controllers/slot-controller.ts @@ -49,17 +49,6 @@ function isContent(node: Node) { } } -/** - * 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 = - (n: string | typeof SlotController.default) => - (child: Element): child is T => - n === SlotController.default ? !child.hasAttribute('slot') - : child.getAttribute('slot') === n; - export declare class SlotControllerPublicAPI implements ReactiveController { static default: symbol; @@ -109,45 +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(); + #slotRecords = new Map(); - #slotMapInitialized = false; - - #slotNames: (string | null)[] = []; + #slotNames: (string | symbol | null)[] = []; #deprecations: Record = {}; #initSlotMap = async () => { const { host } = this; await host.updateComplete; - const nodes = this.#nodes; + const slotRecords = this.#slotRecords; // 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 = - slotId === SlotController.default ? !![...host.childNodes].some(isContent) - : !!slot?.assignedNodes?.().some(isContent); - nodes.set(slotId, { elements, name, hasContent, slot }); + 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(); - this.#slotMapInitialized = true; }; #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]; } @@ -164,43 +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?.(selector) ?? null; + } + async hostConnected(): Promise { 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(); } - #getSlotElement(slotId: string | symbol) { - const selector = - slotId === SlotController.default ? 'slot:not([name])' : `slot[name="${slotId as string}"]`; - return this.host.shadowRoot?.querySelector?.(selector) ?? null; - } - - #getChildrenForSlot( - 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). @@ -218,12 +213,12 @@ export class SlotController implements SlotControllerPublicAPI { * this.getSlotted(); * ``` */ - getSlotted(...slotNames: string[]): T[] { - if (!slotNames.length) { - return (this.#nodes.get(SlotController.default)?.elements ?? []) as T[]; + public getSlotted(...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[]; } } @@ -232,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; + } + }); } /** @@ -247,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); } } diff --git a/core/pfe-core/controllers/test/slot-controller.spec.ts b/core/pfe-core/controllers/test/slot-controller.spec.ts index 0f040ad4ac..ccd6a96550 100644 --- a/core/pfe-core/controllers/test/slot-controller.spec.ts +++ b/core/pfe-core/controllers/test/slot-controller.spec.ts @@ -1,32 +1,24 @@ import { expect, fixture, nextFrame } from '@open-wc/testing'; import { customElement } from 'lit/decorators/custom-element.js'; -import { - ReactiveElement, - html, - render, - type PropertyValues, - type TemplateResult, -} from 'lit'; +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` + + + + `; + } +} + describe('SlotController', function() { describe('with named and anonymous slots', function() { - @customElement('test-slot-controller-with-named-and-anonymous') - class TestSlotControllerWithNamedAndAnonymous extends ReactiveElement { - declare static template: TemplateResult; - - controller = new SlotController(this, 'a', null); - - render(): TemplateResult { - return html` - - - - `; - } - } describe('with no content', function() { let element: TestSlotControllerWithNamedAndAnonymous; beforeEach(async function() { @@ -46,6 +38,15 @@ describe('SlotController', 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() { @@ -69,6 +70,15 @@ describe('SlotController', 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() { @@ -92,6 +102,15 @@ describe('SlotController', 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() { @@ -107,7 +126,7 @@ describe('SlotController', function() { expect(element.controller.hasSlotted('a')).to.be.false; expect(element.controller.isEmpty('a')).to.be.true; }); - it('reports nonjgempty default slot', function() { + it('reports non-empty default slot', function() { expect(element.controller.hasSlotted(null)).to.be.true; expect(element.controller.isEmpty(null)).to.be.false; }); @@ -115,6 +134,15 @@ describe('SlotController', 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; + }); }); }); });