fix: split-button tests & lots of cleanup based on review feedback

I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189

Author: adixon-adobe

packages/picker/src/Picker.ts

@@ -27,7 +27,7 @@ import pickerStyles from './picker.css.js';
 import chevronStyles from '@spectrum-web-components/icon/src/spectrum-icon-chevron.css.js';
 
 import { Focusable } from '@spectrum-web-components/shared/src/focusable.js';
-import reparentChildren from '@spectrum-web-components/shared/src/reparent-children.js';
+import { reparentChildren } from '@spectrum-web-components/shared/src/reparent-children.js';
 import '@spectrum-web-components/icon/sp-icon.js';
 import '@spectrum-web-components/icons-workflow/icons/sp-icon-alert.js';
 import '@spectrum-web-components/menu/sp-menu.js';
@@ -38,6 +38,7 @@ import {
     MenuQueryRoleEventDetail,
 } from '@spectrum-web-components/menu';
 import '@spectrum-web-components/popover/sp-popover.js';
+import { Popover } from '@spectrum-web-components/popover/src/popover.js';
 import {
     Placement,
     openOverlay,
@@ -94,9 +95,10 @@ export class PickerBase extends SizedMixin(Focusable) {
     @property({ type: Boolean, reflect: true })
     public open = false;
 
-    private reparentableChildren?: Element[];
+    public menuItems: MenuItem[] = [];
     private restoreChildren?: Function;
-    public optionsMenu?: Menu;
+
+    public optionsMenu!: Menu;
 
     /**
      * @type {"auto" | "auto-start" | "auto-end" | "top" | "bottom" | "right" | "left" | "top-start" | "top-end" | "bottom-start" | "bottom-end" | "right-start" | "right-end" | "left-start" | "left-end" | "none"}
@@ -118,7 +120,7 @@ export class PickerBase extends SizedMixin(Focusable) {
     private closeOverlay?: () => void;
 
     @query('sp-popover')
-    private popover?: HTMLElement;
+    private popover!: Popover;
 
     protected listRole = 'listbox';
     protected itemRole = 'option';
@@ -144,7 +146,7 @@ export class PickerBase extends SizedMixin(Focusable) {
     }
 
     public get focusElement(): HTMLElement {
-        if (this.open && this.optionsMenu) {
+        if (this.open) {
             return this.optionsMenu;
         }
         return this.button;
@@ -192,10 +194,6 @@ export class PickerBase extends SizedMixin(Focusable) {
             return;
         }
         event.preventDefault();
-        /* c8 ignore next 3 */
-        if (!this.optionsMenu) {
-            return;
-        }
         this.open = true;
     };
 
@@ -243,18 +241,26 @@ export class PickerBase extends SizedMixin(Focusable) {
 
     private async openMenu(): Promise<void> {
         /* c8 ignore next 9 */
-        if (
-            !this.popover ||
-            !this.optionsMenu ||
-            !this.reparentableChildren ||
-            this.reparentableChildren.length === 0
-        ) {
+        let reparentableChildren: Element[] = [];
+
+        const deprecatedMenu = this.querySelector('sp-menu');
+        if (deprecatedMenu) {
+            reparentableChildren = Array.from(deprecatedMenu.children);
+        } else {
+            reparentableChildren = Array.from(this.children).filter(
+                (element) => {
+                    return !element.hasAttribute('slot');
+                }
+            );
+        }
+
+        if (reparentableChildren.length === 0) {
             this.menuStateResolver();
             return;
         }
 
         this.restoreChildren = reparentChildren(
-            this.reparentableChildren,
+            reparentableChildren,
             this.optionsMenu
         );
 
@@ -331,21 +337,27 @@ export class PickerBase extends SizedMixin(Focusable) {
 
     protected render(): TemplateResult {
         return html`
-            ${this.renderButton}
+            ${this.renderButton} ${this.renderPopover}
+        `;
+    }
+
+    protected get renderPopover(): TemplateResult {
+        return html`
             <sp-popover
                 open
                 id="popover"
                 @click=${this.onClick}
                 @sp-overlay-closed=${this.onOverlayClosed}
             >
-                <sp-menu id="menu" role="listbox"></sp-menu>
+                <sp-menu id="menu" role="${this.listRole}"></sp-menu>
             </sp-popover>
         `;
     }
 
     protected firstUpdated(changedProperties: PropertyValues): void {
         super.firstUpdated(changedProperties);
 
+        // Since the sp-menu gets reparented by the popover, initialize it here
         this.optionsMenu = this.shadowRoot.querySelector('sp-menu') as Menu;
 
         const deprecatedMenu = this.querySelector('sp-menu');
@@ -354,18 +366,14 @@ export class PickerBase extends SizedMixin(Focusable) {
                 `Deprecation Notice: You no longer need to provide an sp-menu child to ${this.tagName.toLowerCase()}. Any styling or attributes on the sp-menu will be ignored.`
             );
         }
-        this.reparentableChildren = Array.from(
-            (deprecatedMenu || this).children
-        );
+        this.menuItems = [
+            ...this.querySelectorAll(`sp-menu-item`),
+        ] as MenuItem[];
     }
 
     protected updated(changedProperties: PropertyValues): void {
         super.updated(changedProperties);
-        if (
-            changedProperties.has('value') &&
-            !changedProperties.has('selectedItem') &&
-            this.reparentableChildren
-        ) {
+        if (changedProperties.has('value') && !changedProperties.has('selectedItem')) {
             this.manageSelection();
         }
         if (changedProperties.has('disabled') && this.disabled) {
@@ -386,20 +394,15 @@ export class PickerBase extends SizedMixin(Focusable) {
         }
     }
 
-    private async manageSelection(): Promise<void> {
+    protected async manageSelection(): Promise<void> {
         /* c8 ignore next 3 */
-        if (!this.reparentableChildren) {
-            return;
-        }
-        if (this.reparentableChildren.length) {
+        if (this.menuItems.length > 0) {
             let selectedItem: MenuItem | undefined;
-            this.reparentableChildren.map((item) => {
-                if (item instanceof MenuItem) {
-                    if (this.value === item.value && !item.disabled) {
-                        selectedItem = item;
-                    } else {
-                        item.selected = false;
-                    }
+            this.menuItems.forEach((item) => {
+                if (this.value === item.value && !item.disabled) {
+                    selectedItem = item;
+                } else {
+                    item.selected = false;
                 }
             });
             if (selectedItem) {
@@ -409,12 +412,12 @@ export class PickerBase extends SizedMixin(Focusable) {
                 this.value = '';
                 this.selectedItem = undefined;
             }
-            if (this.open && this.optionsMenu) {
+            if (this.open) {
                 this.optionsMenu.updateSelectedItemIndex();
             }
             return;
         }
-        if (this.open && this.optionsMenu) {
+        if (this.open) {
             await this.optionsMenu.updateComplete;
             if (this.optionsMenu.menuItems.length) {
                 this.manageSelection();

packages/picker/test/picker.test.ts

@@ -488,11 +488,9 @@ describe('Picker', () => {
         expect(el.open).to.be.true;
         expect(isMenuActiveElement()).to.be.true;
 
-        const menu = document.activeElement;
-        if (menu) {
-            // we know this is defined, but typescript doesn't
-            menu.dispatchEvent(tabEvent);
-        }
+        const menu = document.activeElement as Menu;
+        menu.dispatchEvent(tabEvent);
+
         await elementUpdated(el);
         await waitUntil(() => !el.open);
 

packages/shared/src/reparent-children.ts

@@ -2,11 +2,6 @@ function restoreChildren(
     placeholderItems: Comment[],
     srcElements: Element[]
 ): void {
-    if (srcElements.length != placeholderItems.length) {
-        throw new Error(
-            'Unexpected length mismatch attempting to restore parent elements'
-        );
-    }
     for (let index = 0; index < srcElements.length; ++index) {
         const srcElement = srcElements[index];
         const placeholderItem = placeholderItems[index];
@@ -17,7 +12,10 @@ function restoreChildren(
     }
 }
 
-export default function (srcElements: Element[], newParent: Element): Function {
+export const reparentChildren = (
+    srcElements: Element[],
+    newParent: Element
+): Function => {
     let placeholderItems: Comment[] = [];
 
     for (let index = 0; index < srcElements.length; ++index) {
@@ -36,4 +34,4 @@ export default function (srcElements: Element[], newParent: Element): Function {
     return function () {
         restoreChildren(placeholderItems, srcElements);
     };
-}
+};