From f8dda40696d1b4c72cf539ad26f0187215a6dca7 Mon Sep 17 00:00:00 2001 From: Oleksii Kadurin Date: Tue, 7 Oct 2025 12:35:09 +0200 Subject: [PATCH] fix: improve the way the default slots are moved inside the component --- .changeset/twenty-bugs-battle.md | 5 ++ .../combobox/test/lion-combobox.test.js | 52 +++++++++++- packages/ui/components/core/src/SlotMixin.js | 52 ++++++++++++ .../ui/components/core/test/SlotMixin.test.js | 85 +++++++++++++++++++ .../ui/components/listbox/src/ListboxMixin.js | 38 +-------- .../overlays/test/OverlayController.test.js | 3 + 6 files changed, 198 insertions(+), 37 deletions(-) create mode 100644 .changeset/twenty-bugs-battle.md diff --git a/.changeset/twenty-bugs-battle.md b/.changeset/twenty-bugs-battle.md new file mode 100644 index 000000000..e669e752e --- /dev/null +++ b/.changeset/twenty-bugs-battle.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[listbox] fix rendering for lazy slottables diff --git a/packages/ui/components/combobox/test/lion-combobox.test.js b/packages/ui/components/combobox/test/lion-combobox.test.js index 210c4ff94..4238dae7c 100644 --- a/packages/ui/components/combobox/test/lion-combobox.test.js +++ b/packages/ui/components/combobox/test/lion-combobox.test.js @@ -2,11 +2,11 @@ import { defineCE, expect, fixture, html, unsafeStatic, waitUntil } from '@open- import { Required, Unparseable } from '@lion/ui/form-core.js'; import { sendKeys } from '@web/test-runner-commands'; import { LionCombobox } from '@lion/ui/combobox.js'; -import { browserDetection } from '@lion/ui/core.js'; +import { browserDetection, SlotMixin } from '@lion/ui/core.js'; import '@lion/ui/define/lion-combobox.js'; import '@lion/ui/define/lion-listbox.js'; import '@lion/ui/define/lion-option.js'; -import { LitElement } from 'lit'; +import { LitElement, nothing } from 'lit'; import sinon from 'sinon'; import { getFilteredOptionValues, @@ -413,6 +413,54 @@ describe('lion-combobox', () => { expect(el.validationStates).to.have.property('error'); expect(el.validationStates.error).to.have.property('MatchesOption'); }); + + it('keeps slottable provided in `slots` getter as direct host child', async () => { + class MyEl extends SlotMixin(LionCombobox) { + // @ts-ignore + get slots() { + return { + ...super.slots, + _lazyRenderedSlot: () => ({ + template: this.renderSlot + ? html`(Optional)` + : html`${nothing}`, + renderAsDirectHostChild: true, + }), + }; + } + + _labelTemplate() { + return html` +
+ + + +
+ `; + } + + constructor() { + super(); + this.renderSlot = false; + } + } + const tagName = defineCE(MyEl); + const wrappingTag = unsafeStatic(tagName); + + const el = /** @type {MyEl} */ ( + await fixture(html` + <${wrappingTag} label="my label"> + ${'one'} + + `) + ); + await el.registrationComplete; + + el.renderSlot = true; + await el.updateComplete; + const lazyRenderedSlot = el.querySelector('#lazyRenderedSlotId'); + expect(lazyRenderedSlot?.parentElement === el).to.be.true; + }); }); describe('Values', () => { diff --git a/packages/ui/components/core/src/SlotMixin.js b/packages/ui/components/core/src/SlotMixin.js index d56d936c5..747ef5617 100644 --- a/packages/ui/components/core/src/SlotMixin.js +++ b/packages/ui/components/core/src/SlotMixin.js @@ -12,6 +12,58 @@ import { render } from 'lit'; * @typedef {import('lit').LitElement} LitElement */ +/** + * Sometimes, we want to provide best DX (direct slottables) and be accessible + * at the same time. + * In the first example below, we need to wrap our options in light dom in an element with + * [role=listbox]. We could achieve this via the second example, but it would affect our + * public api negatively. not allowing us to be forward compatible with the AOM spec: + * https://wicg.github.io/aom/explainer.html + * With this method, it's possible to watch elements in the default slot and move them + * to the desired target (the element with [role=listbox]) in light dom. + * + * @example + * # desired api + * + * + * + * # desired end state + * + *
+ * + *
+ * + * + * Note, the function does not move the nodes specified by a subclasser in the `slots` getter + * @param {HTMLElement} source host of ShadowRoot with default + * @param {HTMLElement} target the desired target in light dom + */ +export function moveUserProvidedDefaultSlottablesToTarget(source, target) { + /** + * Nodes injected via `slots` getter are going to be added as host's children + * starting by a comment node like + * and ending by a comment node like + * So we ignore everything that comes between those `start_slot` and `end_slot` comments + */ + let isInsideSlotSection = false; + Array.from(source.childNodes).forEach((/** @type {* & Element} */ c) => { + const isNamedSlottable = c.hasAttribute && c.hasAttribute('slot'); + const isComment = c.nodeType === Node.COMMENT_NODE; + if (isComment && !isInsideSlotSection) { + isInsideSlotSection = c.textContent.includes('_start_slot_'); + } + if (isInsideSlotSection) { + if (c.textContent.includes('_end_slot_')) { + isInsideSlotSection = false; + } + return; + } + if (!isNamedSlottable) { + target.appendChild(c); + } + }); +} + /** * @param {SlotFunctionResult} slotFunctionResult * @returns {'template-result'|'node'|'slot-rerender-object'|null} diff --git a/packages/ui/components/core/test/SlotMixin.test.js b/packages/ui/components/core/test/SlotMixin.test.js index 2f664ae05..b15bfc55e 100644 --- a/packages/ui/components/core/test/SlotMixin.test.js +++ b/packages/ui/components/core/test/SlotMixin.test.js @@ -2,6 +2,7 @@ import { defineCE, expect, fixture, fixtureSync, unsafeStatic, html } from '@ope import { SlotMixin } from '@lion/ui/core.js'; import { LitElement } from 'lit'; import sinon from 'sinon'; +import { moveUserProvidedDefaultSlottablesToTarget } from '../src/SlotMixin.js'; import { ScopedElementsMixin, supportsScopedRegistry } from '../src/ScopedElementsMixin.js'; import { isActiveElement } from '../test-helpers/isActiveElement.js'; @@ -134,6 +135,90 @@ describe('SlotMixin', () => { expect(elNoSlot.didCreateConditionalSlot()).to.be.false; }); + it('should move default slots to target', async () => { + const target = document.createElement('div'); + const source = document.createElement('div'); + /** + * Exmple of usage: + * get slots() { + * return { + * _nothing: () => ({ + * template: html`${nothing}`, + * renderAsDirectHostChild: true, + * }), + * } + * } + */ + const variableNothing = ` + + + `; + + /** + * Exmple of usage: + * get slots() { + * return { + * '': () => ({ + * template: html`
text
`, + * renderAsDirectHostChild: true, + * }), + * } + * } + */ + const defaultSlottableProvidedViaSlotsGetter = ` + +
text
+ + `; + + /** + * Exmple of usage: + * get slots() { + * return { + * label: () => ({ + * template: html`
text
`, + * renderAsDirectHostChild: true, + * }), + * } + * } + */ + const namedSlottable = ` + +
text
+ + `; + + /** + * Here we assume .test1, .test2 and .test3 are the ones provided as content projection f.e.: + * + *
+ *
+ *
+ * + * + * And the rest of content is added via `slots` getter by SlotMixin + * The function should move only content projection and ignore the rest + * */ + + source.innerHTML = ` +
+ ${variableNothing} +
+ ${defaultSlottableProvidedViaSlotsGetter} +
+ ${namedSlottable} + `; + + moveUserProvidedDefaultSlottablesToTarget(source, target); + expect(target.children.length).to.equal(3); + const test1Element = target.querySelector('.test1'); + const test2Element = target.querySelector('.test2'); + const test3Element = target.querySelector('.test3'); + expect(test1Element?.parentElement === target).to.equal(true); + expect(test2Element?.parentElement === target).to.equal(true); + expect(test3Element?.parentElement === target).to.equal(true); + }); + describe('Rerender', () => { it('supports rerender when SlotRerenderObject provided', async () => { const tag = defineCE( diff --git a/packages/ui/components/listbox/src/ListboxMixin.js b/packages/ui/components/listbox/src/ListboxMixin.js index c609c0d48..6b2111435 100644 --- a/packages/ui/components/listbox/src/ListboxMixin.js +++ b/packages/ui/components/listbox/src/ListboxMixin.js @@ -4,6 +4,7 @@ import { dedupeMixin } from '@open-wc/dedupe-mixin'; import { ChoiceGroupMixin, FormControlMixin, FormRegistrarMixin } from '@lion/ui/form-core.js'; import { ScopedElementsMixin } from '../../core/src/ScopedElementsMixin.js'; import { LionOptions } from './LionOptions.js'; +import { moveUserProvidedDefaultSlottablesToTarget } from '../../core/src/SlotMixin.js'; // TODO: extract ListNavigationWithActiveDescendantMixin that can be reused in [role="menu"] // having children with [role="menuitem|menuitemcheckbox|menuitemradio|option"] and @@ -21,39 +22,6 @@ import { LionOptions } from './LionOptions.js'; // TODO: consider adding methods below to @lion/helpers -/** - * Sometimes, we want to provide best DX (direct slottables) and be accessible - * at the same time. - * In the first example below, we need to wrap our options in light dom in an element with - * [role=listbox]. We could achieve this via the second example, but it would affect our - * public api negatively. not allowing us to be forward compatible with the AOM spec: - * https://wicg.github.io/aom/explainer.html - * With this method, it's possible to watch elements in the default slot and move them - * to the desired target (the element with [role=listbox]) in light dom. - * - * @example - * # desired api - * - * - * - * # desired end state - * - *
- * - *
- * - * @param {HTMLElement} source host of ShadowRoot with default - * @param {HTMLElement} target the desired target in light dom - */ -function moveDefaultSlottablesToTarget(source, target) { - Array.from(source.childNodes).forEach((/** @type {* & Element} */ c) => { - const isNamedSlottable = c.hasAttribute && c.hasAttribute('slot'); - if (!isNamedSlottable) { - target.appendChild(c); - } - }); -} - /** * @type {ListboxMixin} * @param {import('@open-wc/dedupe-mixin').Constructor} superclass @@ -899,9 +867,9 @@ const ListboxMixinImplementation = superclass => ); if (slot) { - moveDefaultSlottablesToTarget(this, this._listboxNode); + moveUserProvidedDefaultSlottablesToTarget(this, this._listboxNode); slot.addEventListener('slotchange', () => { - moveDefaultSlottablesToTarget(this, this._listboxNode); + moveUserProvidedDefaultSlottablesToTarget(this, this._listboxNode); }); } } diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js index ff508a498..72b2d2afe 100644 --- a/packages/ui/components/overlays/test/OverlayController.test.js +++ b/packages/ui/components/overlays/test/OverlayController.test.js @@ -799,6 +799,9 @@ describe('OverlayController', () => { const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); await mimicEscapePress(childOverlay.contentNode); + // without this line, the test is unstable on FF sometimes + await aTimeout(0); + expect(parentOverlay.isShown).to.be.false; expect(childOverlay.isShown).to.be.true;