chore: code review updates

packages/overlay/imperative-api.md

@@ -26,7 +26,7 @@ Overlay.open(
 );
 ```
 
-`Overlay.open()` is an asynchronous method that returns an `<sp-overlay>` element that wraps the `HTMLElement` provided as the `overlayElement`. While this process will set the `<sp-overlay>` element to `open`, consumers of this API will need to chose where to append this element to the DOM in order for the content to be made available in the browser.
+`Overlay.open()` is an asynchronous method that returns an `<sp-overlay>` element that wraps the `HTMLElement` provided as the `overlayElement`. While this process will set the `<sp-overlay>` element to `open`, consumers of this API will need to choose where to append this element to the DOM in order for the content to be made available in the browser.
 
 ```js
 (async () => {
@@ -42,7 +42,7 @@ Overlay.open(
 })();
 ```
 
-Keep in mind that a changing DOM tree can update the relationship between existing content and the CSS that you have applied to it when appending the `<sp-overlay>` element to your existing DOM.
+Keep in mind that a changing DOM tree is likely to update the relationship between existing content. Without proper care this can negatively effect the CSS that you have applied to existing content. DOM events and DOM selection methods can also perform differently than expected as the tree shape changes.
 
 ## OverlayOptions
 
@@ -62,7 +62,7 @@ type OverlayOptions = {
 
 ### delayed
 
-An Overlay that is `delayed` will wait until a warm-up period has completed before opening. Once the warmup period has completed, all subsequent Overlays will open immediately. When no Overlays are opened, a cooldown period will begin. Once the cooldown has completed, the next Overlay to be opened will be subject to the warm-up period if provided that option.
+An Overlay that is `delayed` will wait until a warm-up period of 1000ms has completed before opening. Once the warmup period has completed, all subsequent Overlays will open immediately. When no Overlays are opened, a cooldown period of 1000ms will begin. Once the cooldown has completed, the next Overlay to be opened will be subject to the warm-up period if provided that option.
 
 ### notImmediatelyCloseable
 
@@ -90,6 +90,6 @@ The `type` of an Overlay outlines a number of things about the interaction model
 
 -   `'modal'` Overlays are opened with the `showModal()` method on a `<dialog>` element, which causes the Overlay to accept focus and trap the tab stop within the content of said Overlay.
 -   `'page'` Overlays will act in a similar manner to a `'modal'` Overlay, however they will not be allowed to close via the "light dismiss" algorithm (e.g. the Escape key).
--   `'hint'` Overlays are much like tooltips so they are not just ephemeral, but they are delivered primarily as a visual helper and exist outside of the tab order. In this way, be sure _not_ be palce interactive content within this type of Overlay.
+-   `'hint'` Overlays are much like tooltips so they are not just ephemeral, but they are delivered primarily as a visual helper and exist outside of the tab order. In this way, be sure _not_ to place interactive content within this type of Overlay.
 -   `'auto'` Overlays provide a place for content that is ephemeral _and_ interactive. These Overlays can accept focus but will close when losing that focus, or when interacting with other parts of the page.
 -   `'manual'` Overlays act much like `"auto"` Overlays, but do not close when losing focus or interacting with other parts of the page. When a `"manual"` Overlay is at the top of the "overlay stack", it will still respond to the Escape key and close.

packages/overlay/src/Overlay.ts

@@ -60,7 +60,7 @@ export class Overlay extends OverlayFeatures implements OverlayBase {
         target: HTMLElement,
         interaction: TriggerInteractionsV1,
         content: HTMLElement,
-        options: OverlayOptionsV1
+        optionsV1: OverlayOptionsV1
     ): Promise<() => void>;
     public static async open(
         content: HTMLElement,
@@ -73,11 +73,12 @@ export class Overlay extends OverlayFeatures implements OverlayBase {
             | OverlayOptions
             | undefined,
         content?: HTMLElement,
-        options?: OverlayOptionsV1
+        optionsV1?: OverlayOptionsV1
     ): Promise<Overlay | (() => void)> {
         const v2 = arguments.length === 2;
         const overlayContent = content || targetOrContent;
         const overlay = new Overlay();
+        let restored = false;
         overlay.dispose = () => {
             overlay.addEventListener('sp-closed', () => {
                 if (!restored) {
@@ -91,36 +92,37 @@ export class Overlay extends OverlayFeatures implements OverlayBase {
             overlay.open = false;
             overlay.dispose = noop;
         };
-        let restored = false;
+        /**
+         * Since content must exist in an <sp-overlay>, we need a way to get it there.
+         * The best & most-direct way is to declaratively use an <sp-overlay> element,
+         * but for imperative users, we'll reparent content into an overlay that we've created for them.
+         **/
         const restoreContent = reparentChildren([overlayContent], overlay, {
             position: 'beforeend',
             prepareCallback: (el) => {
+                // Ensure that content to be overlaid is no longer targetted to a specific `slot`.
+                // This allow for it to be visible in the overlaid context.
                 const slot = el.slot;
                 el.removeAttribute('slot');
                 return () => {
                     el.slot = slot;
                 };
             },
         });
-        if (v2) {
-            const options = interactionOrOptions as OverlayOptions;
-            overlay.append(overlayContent);
-            overlay.receivesFocus = options.receivesFocus ?? 'auto';
-            overlay.triggerElement = options.trigger || null;
-            overlay.type = options.type || 'modal';
-            overlay.offset = options.offset || 6;
-            overlay.placement = options.placement;
-            overlay.willPreventClose = !!options.notImmediatelyClosable;
-            requestAnimationFrame(() => {
-                requestAnimationFrame(() => {
-                    // Do we want to "open" this path, or leave that to the consumer?
-                    overlay.open = true;
-                });
-            });
-            return overlay;
-        } else if (overlayContent && options) {
+
+        const v1 = !v2 && overlayContent && optionsV1;
+        if (v1) {
+            if (window.__swc.DEBUG) {
+                window.__swc.warn(
+                    overlay,
+                    `You are interacting with an ${overlay.localName} element via a deprecated imperative API. This API will be removed in a future version of the SWC library. Consider leveraging an ${overlay.localName} directly.`,
+                    'https://opensource.adobe.com/spectrum-web-components/components/overlay/',
+                    { level: 'deprecation' }
+                );
+            }
             const target = targetOrContent;
             const interaction = interactionOrOptions;
+            const options = optionsV1;
             overlay.receivesFocus = options.receivesFocus ?? 'auto';
             overlay.triggerElement = options.virtualTrigger || target;
             overlay.type =
@@ -132,25 +134,28 @@ export class Overlay extends OverlayFeatures implements OverlayBase {
             overlay.offset = options.offset ?? 6;
             overlay.placement = options.placement;
             overlay.willPreventClose = !!options.notImmediatelyClosable;
-            // This is super dirty...find a better way.
-            // Maybe imperative open should go _at the end_ of everything?
-            // Having an option is likely useful.
-            // Make imperative overlays less useful?
-            // Delete the imperative approach to an overlay?
-            // Possibly the giving all of the responsiblities to the user is the best path.
-            const parent = target.getRootNode() as Document;
-            if (parent === document) {
-                target.insertAdjacentElement('afterend', overlay);
-            } else {
-                parent.append(overlay);
-            }
+            target.insertAdjacentElement('afterend', overlay);
             await new Promise<void>((res) =>
                 requestAnimationFrame(() => requestAnimationFrame(() => res()))
             );
             overlay.open = true;
             return overlay.dispose;
         }
-        /* c8 ignore next 1 */
+
+        const options = interactionOrOptions as OverlayOptions;
+        overlay.append(overlayContent);
+        overlay.receivesFocus = options.receivesFocus ?? 'auto';
+        overlay.triggerElement = options.trigger || null;
+        overlay.type = options.type || 'modal';
+        overlay.offset = options.offset ?? 6;
+        overlay.placement = options.placement;
+        overlay.willPreventClose = !!options.notImmediatelyClosable;
+        requestAnimationFrame(() => {
+            requestAnimationFrame(() => {
+                // Do we want to "open" this path, or leave that to the consumer?
+                overlay.open = true;
+            });
+        });
         return overlay;
     }
 }

packages/overlay/src/OverlayBase.ts

@@ -53,7 +53,7 @@ export type OpenableElement = HTMLElement & {
 
 const LONGPRESS_DURATION = 300;
 
-export type LongpressEvent = {
+type LongpressEvent = {
     source: 'pointer' | 'keyboard';
 };
 

packages/overlay/src/OverlayTrigger.ts

@@ -22,10 +22,6 @@ import {
     query,
     state,
 } from '@spectrum-web-components/base/src/decorators.js';
-import {
-    isAndroid,
-    isIOS,
-} from '@spectrum-web-components/shared/src/platform.js';
 
 import { OverlayTriggerInteractions } from './overlay-types';
 import overlayTriggerStyles from './overlay-trigger.css.js';
@@ -35,12 +31,6 @@ import { BeforetoggleOpenEvent, OverlayBase } from './OverlayBase.js';
 
 export type OverlayContentTypes = 'click' | 'hover' | 'longpress';
 
-export const LONGPRESS_INSTRUCTIONS = {
-    touch: 'Double tap and long press for additional options',
-    keyboard: 'Press Space or Alt+Down Arrow for additional options',
-    mouse: 'Click and hold for additional options',
-};
-
 /**
  * @element overlay-trigger
  *
@@ -76,8 +66,6 @@ export class OverlayTrigger extends SpectrumElement {
     @property({ type: Boolean, reflect: true })
     public disabled = false;
 
-    private longpressDescriptor?: HTMLElement;
-
     @state()
     private clickContent: HTMLElement[] = [];
 
@@ -254,47 +242,6 @@ export class OverlayTrigger extends SpectrumElement {
             this.open = undefined;
             return;
         }
-        if (
-            changes.has('longpressContent') &&
-            (this.longpressContent.length ||
-                typeof changes.get('longpressContent') !== 'undefined')
-        ) {
-            this.manageLongpressDescriptor();
-        }
-    }
-
-    protected manageLongpressDescriptor(): void {
-        const trigger = this.querySelector(
-            '[slot="trigger"]'
-        ) as SpectrumElement;
-        const ariaDescribedby = trigger.getAttribute('aria-describedby');
-        let descriptors = ariaDescribedby ? ariaDescribedby.split(/\s+/) : [];
-
-        if (this.longpressContent.length) {
-            if (!this.longpressDescriptor) {
-                this.longpressDescriptor = document.createElement(
-                    'div'
-                ) as HTMLElement;
-
-                this.longpressDescriptor.id = this._longpressId;
-                this.longpressDescriptor.slot = this._longpressId;
-            }
-            const messageType = isIOS() || isAndroid() ? 'touch' : 'keyboard';
-            this.longpressDescriptor.textContent =
-                LONGPRESS_INSTRUCTIONS[messageType];
-            this.appendChild(this.longpressDescriptor);
-            descriptors.push(this._longpressId);
-        } else {
-            if (this.longpressDescriptor) this.longpressDescriptor.remove();
-            descriptors = descriptors.filter(
-                (descriptor) => descriptor !== this._longpressId
-            );
-        }
-        if (descriptors.length) {
-            trigger.setAttribute('aria-describedby', descriptors.join(' '));
-        } else {
-            trigger.removeAttribute('aria-describedby');
-        }
     }
 
     protected override async getUpdateComplete(): Promise<boolean> {

packages/overlay/src/index.ts

@@ -10,6 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 export * from './Overlay.js';
+export * from './OverlayBase.js';
 export * from './OverlayTrigger.js';
 export * from './overlay-types.js';
 export * from './VirtualTrigger.js';

packages/overlay/test/overlay-v1.test.ts

@@ -38,7 +38,6 @@ import {
 } from '../stories/overlay.stories';
 import { PopoverContent } from '../stories/overlay-story-components.js';
 import { sendMouse } from '../../../test/plugins/browser.js';
-import { spy } from 'sinon';
 import '@spectrum-web-components/theme/sp-theme.js';
 import '@spectrum-web-components/theme/src/themes.js';
 import { Theme } from '@spectrum-web-components/theme';
@@ -139,16 +138,12 @@ describe('Overlays, v1', () => {
     ].map((direction) => {
         const placement = direction as Placement;
         it(`opens a popover - ${placement}`, async () => {
-            const clickSpy = spy();
             const button = testDiv.querySelector(
                 '#first-button'
             ) as HTMLElement;
             const outerPopover = testDiv.querySelector(
                 '#outer-popover'
             ) as Popover;
-            outerPopover.addEventListener('click', () => {
-                clickSpy();
-            });
 
             expect(await isInteractive(outerPopover)).to.be.false;
             expect(button).to.exist;

projects/documentation/content/migrations/2023-6-11.md → projects/documentation/content/migrations/2023-8-1.md

@@ -1,11 +1,11 @@
 ---
 layout: guide.njk
 title: 'Migration Guide: Spectrum Web Components'
-displayName: 2023/6/11
+displayName: 2023/9/1
 slug: migration-guide
 ---
 
-## Migration: 2023/6/11
+## Migration: 2023/8/1
 
 As of the 0.33.0 release of the Spectrum Web Components library, we will be leveraging a new version of our Overlay API. We've done our best to ensure a smooth transition from one version of the API to the next, including adding extended support for the argument signature from the previous version. In this way, consumption of elements from the library (e.g. `<overlay-trigger>`, `<sp-picker>`, `<sp-tooltip>`, et al.) or the imperative Overlay API (e.g. `Overlay.open()`) should continue to afford as close to the same functionality as the provided you in the past. Under the covers, many important changes have been made and there are several things you can do in preparation for those changes in your application's lifecycle.
 
@@ -90,6 +90,12 @@ function handleSpOpened(event: OverlayOpenCloseDetail) {
     import '@spectrum-web-components/tooltip/sp-tooltip.js';
 </script>
 
+### Consider leveraging the API declaratively
+
+The new Overlay API will continue to surface the `Overlay.open()` method to more readily support migration from previous verions. However, imperatively interacting with the API in the way will continue to require that the content you wish to overlay be reparented, which can have unexpected side effects. If you begin leveraging the V2 signature of the API, you content will be reparented into an `<sp-overlay>` element, and, rather than immediately placing the element into the page, it will be returned so that you can decide where you would like to append this content to your application. While continuing to leverage the V1 signature, not only will you content be reparented into an `<sp-overlay>` element, but that element will be immediately inserted after the `trigger` element that you have provided in support of applying your overlaid content to the tab order of your content in a predictable manner.
+
+Either of these processes can have negative effects on the application of CSS, the way that events will propagate through your application, and the values returned from DOM selections APIs (like `querySelector(...)`). Leverage an `<sp-overlay>` directly in your application for full control over where the overlaid content will live in the DOM. Choosing a static location in your DOM for the `<sp-overlay>` element controlling your overlay will not only does this normalize interactions with CSS, events, or DOM selection APIs, but also empower you to make more sustainable decisions as to tab order by which keyboard and screen reader users will interact with your content.
+
 ## Explanation
 
 The new version of the Overlay API no longer relies on portalling (moving content to the end of the `<body>` element) to defeat CSS clipping and stacking. While this approach was good at overcoming these realities, the reparenting of the overlaid content required to apply this technique had high-performance costs for constantly reorienting elements to a new parent and broke encapsulation, making it difficult to take full control of the delivery of that content as far as styling. In exchange, the new API will leverage `<dialog>` elements and the `showModal()` method for modal overlays and the `popover` attribute along with the `showPopover()` method they add to elements to which they are applied. Both of these APIs lift content onto the [top layer](https://developer.mozilla.org/en-US/docs/Glossary/Top_layer) of the browser which provides a full guarantee against CSS clipping and stacking interrupting the content addressed to this layer. Be aware that content on this layer is managed as a strict stack, so the last added will always be "on top" regardless of any additional CSS you may apply. [A discussion](https://github.com/adobe/spectrum-web-components/discussions/2764#discussioncomment-5327797) around additional features that could address this reality is ongoing, please jump in with your thoughts if you have them.