fix: improve the way the default slots are moved inside the component

.changeset/twenty-bugs-battle.md

@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+[listbox] fix rendering for lazy slottables

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`<span id="lazyRenderedSlotId">(Optional)</span>`
+                : html`${nothing}`,
+              renderAsDirectHostChild: true,
+            }),
+          };
+        }
+
+        _labelTemplate() {
+          return html`
+            <div class="form-field__label">
+              <slot name="label"></slot>
+              <slot name="_lazyRenderedSlot"></slot>
+              <slot></slot>
+            </div>
+          `;
+        }
+
+        constructor() {
+          super();
+          this.renderSlot = false;
+        }
+      }
+      const tagName = defineCE(MyEl);
+      const wrappingTag = unsafeStatic(tagName);
+
+      const el = /** @type {MyEl} */ (
+        await fixture(html`
+          <${wrappingTag} label="my label">
+            <lion-option .choiceValue="${'1'}">${'one'}</lion-option>
+          </${wrappingTag}>
+        `)
+      );
+      await el.registrationComplete;
+
+      el.renderSlot = true;
+      await el.updateComplete;
+      const lazyRenderedSlot = el.querySelector('#lazyRenderedSlotId');
+      expect(lazyRenderedSlot?.parentElement === el).to.be.true;
+    });
   });
 
   describe('Values', () => {

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
+ * <sel-ect>
+ *  <opt-ion></opt-ion>
+ * </sel-ect>
+ * # desired end state
+ * <sel-ect>
+ *  <div role="listbox" slot="lisbox">
+ *    <opt-ion></opt-ion>
+ *  </div>
+ * </sel-ect>
+ *
+ * Note, the function does not move the nodes specified by a subclasser in the `slots` getter
+ * @param {HTMLElement} source host of ShadowRoot with default <slot>
+ * @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 <!--_start_slot_*-->
+   * and ending by a comment node like <!--_end_slot_*-->
+   * 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}

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 = `
+      <!--_start_slot_lit-el-->
+      <!-- {lit-el} -->
+      <!--_end_slot_lit-el-->`;
+
+    /**
+     * Exmple of usage:
+     * get slots() {
+     *   return {
+     *     '': () => ({
+     *       template: html`<div>text<div>`,
+     *       renderAsDirectHostChild: true,
+     *     }),
+     *   }
+     * }
+     */
+    const defaultSlottableProvidedViaSlotsGetter = `
+      <!--_start_slot_-->
+      <div>text</div>
+      <!--_end_slot_-->
+    `;
+
+    /**
+     * Exmple of usage:
+     * get slots() {
+     *   return {
+     *     label: () => ({
+     *       template: html`<div>text<div>`,
+     *       renderAsDirectHostChild: true,
+     *     }),
+     *   }
+     * }
+     */
+    const namedSlottable = `
+      <!--_start_slot_label-->
+      <div slot="label">text</div>
+      <!--_end_slot_label-->
+    `;
+
+    /**
+     * Here we assume .test1, .test2 and .test3 are the ones provided as content projection f.e.:
+     * <my-comp>
+     *  <div class="test1"><div>
+     *  <div class="test2"><div>
+     *  <div class="test3"><div>
+     * </my-comp>
+     *
+     * 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 = `
+      <div class="test1"></div>
+      ${variableNothing}
+      <div class="test2"></div>
+      ${defaultSlottableProvidedViaSlotsGetter}
+      <div class="test3"></div>
+      ${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(

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
- * <sel-ect>
- *  <opt-ion></opt-ion>
- * </sel-ect>
- * # desired end state
- * <sel-ect>
- *  <div role="listbox" slot="lisbox">
- *    <opt-ion></opt-ion>
- *  </div>
- * </sel-ect>
- * @param {HTMLElement} source host of ShadowRoot with default <slot>
- * @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<import('lit').LitElement>} 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);
         });
       }
     }

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;