fix: correct the relationship between overlayWillCloseCallback and ph…

…ased animations

packages/dialog/src/DialogBase.ts

@@ -80,18 +80,14 @@ export class DialogBase extends FocusVisiblePolyfillMixin(SpectrumElement) {
         return dialog || this;
     }
 
-    public override focus(): void {
+    public override async focus(): Promise<void> {
         if (this.shadowRoot) {
             const firstFocusable = firstFocusableIn(this.dialog);
             if (firstFocusable) {
-                if (firstFocusable.updateComplete) {
-                    firstFocusable.updateComplete.then(() =>
-                        firstFocusable.focus()
-                    );
-                    /* c8 ignore next 3 */
-                } else {
-                    firstFocusable.focus();
+                if ((firstFocusable as SpectrumElement).updateComplete) {
+                    await firstFocusable.updateComplete;
                 }
+                firstFocusable.focus();
                 this.removeAttribute('tabindex');
             } else {
                 this.dialog.focus();
@@ -102,8 +98,10 @@ export class DialogBase extends FocusVisiblePolyfillMixin(SpectrumElement) {
         }
     }
 
+    private animating = false;
+
     public overlayWillCloseCallback(): boolean {
-        if (!this.open) return false;
+        if (!this.open) return this.animating;
         this.close();
         return true;
     }
@@ -134,8 +132,8 @@ export class DialogBase extends FocusVisiblePolyfillMixin(SpectrumElement) {
 
     protected handleUnderlayTransitionend(event: TransitionEvent): void {
         if (!this.open && event.propertyName === 'visibility') {
-            this.dispatchClosed();
             this.resolveTransitionPromise();
+            this.dispatchClosed();
         }
     }
 
@@ -150,9 +148,13 @@ export class DialogBase extends FocusVisiblePolyfillMixin(SpectrumElement) {
 
     protected override update(changes: PropertyValues<this>): void {
         if (changes.has('open') && changes.get('open') !== undefined) {
-            this.transitionPromise = new Promise(
-                (res) => (this.resolveTransitionPromise = res)
-            );
+            this.animating = true;
+            this.transitionPromise = new Promise((res) => {
+                this.resolveTransitionPromise = () => {
+                    this.animating = false;
+                    res();
+                };
+            });
         }
         super.update(changes);
     }

packages/dialog/test/dialog-base.test.ts

@@ -29,7 +29,6 @@ import '@spectrum-web-components/overlay/overlay-trigger.js';
 import { alertDestructive } from '../stories/dialog.stories.js';
 import { Button } from '@spectrum-web-components/button/src/Button.js';
 import { DialogBase } from '@spectrum-web-components/dialog';
-import { sendMouse } from '../../../test/plugins/browser.js';
 
 async function styledFixture<T extends Element>(
     story: TemplateResult
@@ -50,18 +49,6 @@ const overlayTrigger = (story: () => TemplateResult): TemplateResult => html`
 `;
 
 describe('dialog base', () => {
-    beforeEach(async () => {
-        // Something about this prevents Chromium from swallowing the CSS transitions
-        // to "open" so that test timing can be properly acquired below.
-        await sendMouse({
-            steps: [
-                {
-                    type: 'move',
-                    position: [0, 0],
-                },
-            ],
-        });
-    });
     it('does not close by default with interacting with buttons', async () => {
         const el = await styledFixture<OverlayTrigger>(
             overlayTrigger(
@@ -115,10 +102,6 @@ describe('dialog base', () => {
         expect(dialog.parentElement?.localName).to.equal('overlay-trigger');
     });
     it('does not close by default with interacting with buttons when recycled', async () => {
-        // There is an `sp-dialog-base` recyling issue in Firefox
-        if (/Firefox/.test(window.navigator.userAgent)) {
-            return;
-        }
         const el = await styledFixture<OverlayTrigger>(
             overlayTrigger(
                 () => html`

packages/dialog/test/dialog-wrapper.test.ts

@@ -105,9 +105,7 @@ describe('Dialog Wrapper', () => {
         expect(el.open).to.be.false;
         expect(closeSpy.callCount).to.equal(1);
     });
-    xit('opens and closes when element is recycled', async () => {
-        // Skip while there is an `sp-dialog-base` recyling issue in Chromium and Firefox,
-        // this test passes outside of the testing context and in Safari...
+    it('opens and closes when element is recycled', async () => {
         const closeSpy = spy();
         const test = await styledFixture<OverlayTrigger>(longContent());
         const el = test.querySelector('sp-dialog-wrapper') as DialogWrapper;

packages/overlay/src/overlay-stack.ts

@@ -48,6 +48,10 @@ interface ManagedOverlayContent {
     updateComplete?: Promise<boolean>;
 }
 
+function nextFrame(): Promise<void> {
+    return new Promise((res) => requestAnimationFrame(() => res()));
+}
+
 export class OverlayStack {
     public overlays: ActiveOverlay[] = [];
 
@@ -314,26 +318,26 @@ export class OverlayStack {
          * has to happen AFTER the current call stack completes in case there
          * is work there in to remove the previous "top" overlay.
          */
-        return new Promise((res) => requestAnimationFrame(res)).then(
-            async () => {
-                this.overlays.push(activeOverlay);
-                await activeOverlay.updateComplete;
-                this.addOverlayEventListeners(activeOverlay);
-                if (typeof contentWithLifecycle.open !== 'undefined') {
-                    contentWithLifecycle.open = true;
-                }
-                let cb: () => Promise<void> | void = () => {
-                    return;
-                };
-                if (contentWithLifecycle.overlayOpenCallback) {
-                    const { trigger } = activeOverlay;
-                    const { overlayOpenCallback } = contentWithLifecycle;
-                    cb = async () => await overlayOpenCallback({ trigger });
-                }
-                await activeOverlay.openCallback(cb);
-                return false;
-            }
-        );
+        await nextFrame();
+        this.overlays.push(activeOverlay);
+        await activeOverlay.updateComplete;
+        this.addOverlayEventListeners(activeOverlay);
+        if (typeof contentWithLifecycle.open !== 'undefined') {
+            await nextFrame();
+            // Without the rAF Firefox gets here to early
+            // and is not able trigger the animation.
+            contentWithLifecycle.open = true;
+        }
+        let cb: () => Promise<void> | void = () => {
+            return;
+        };
+        if (contentWithLifecycle.overlayOpenCallback) {
+            const { trigger } = activeOverlay;
+            const { overlayOpenCallback } = contentWithLifecycle;
+            cb = async () => await overlayOpenCallback({ trigger });
+        }
+        await activeOverlay.openCallback(cb);
+        return false;
     }
 
     public addOverlayEventListeners(activeOverlay: ActiveOverlay): void {
@@ -565,6 +569,7 @@ export class OverlayStack {
             this.manageFocusAfterCloseWhenLastOverlay(overlay);
         }
 
+        await overlay.updateComplete;
         overlay.remove();
         overlay.dispose();
 

packages/overlay/test/overlay-trigger-hover.test.ts

@@ -36,7 +36,6 @@ import '@spectrum-web-components/theme/src/themes.js';
 import { TemplateResult } from '@spectrum-web-components/base';
 import { Theme } from '@spectrum-web-components/theme';
 import { Tooltip } from '@spectrum-web-components/tooltip';
-import { sendMouse } from '../../../test/plugins/browser.js';
 import { ignoreResizeObserverLoopError } from '../../../test/testing-helpers.js';
 
 ignoreResizeObserverLoopError(before, after);
@@ -53,32 +52,6 @@ async function styledFixture<T extends Element>(
 }
 
 describe('Overlay Trigger - Hover', () => {
-    afterEach(async () => {
-        const triggers = [
-            ...document.querySelectorAll('overlay-trigger'),
-        ] as OverlayTrigger[];
-        const promises = triggers.map((trigger) => {
-            if (trigger.open) {
-                const closed = oneEvent(trigger, 'sp-closed');
-                trigger.open = undefined;
-                return closed;
-            }
-            return Promise.resolve();
-        });
-        await Promise.all(promises);
-    });
-    beforeEach(async () => {
-        // Something about this prevents Chromium from swallowing the CSS transitions
-        // to "open" so that test timing can be properly acquired below.
-        await sendMouse({
-            steps: [
-                {
-                    type: 'move',
-                    position: [0, 0],
-                },
-            ],
-        });
-    });
     it('displays `hover` declaratively', async () => {
         const openedSpy = spy();
         const closedSpy = spy();

packages/shared/src/focusable.ts

@@ -16,6 +16,10 @@ import { FocusVisiblePolyfillMixin } from './focus-visible.js';
 
 type DisableableElement = HTMLElement & { disabled?: boolean };
 
+function nextFrame(): Promise<void> {
+    return new Promise((res) => requestAnimationFrame(() => res()));
+}
+
 /**
  * Focusable base class handles tabindex setting into shadowed elements automatically.
  *
@@ -259,12 +263,31 @@ export class Focusable extends FocusVisiblePolyfillMixin(SpectrumElement) {
         }
     }
 
+    protected override async getUpdateComplete(): Promise<boolean> {
+        const complete = (await super.getUpdateComplete()) as boolean;
+        if (this._recentlyConnected) {
+            this._recentlyConnected = false;
+            // If at connect time the [autofocus] content is placed within
+            // content that needs to be "hidden" by default, it would need to wait
+            // two rAFs for animations to be triggered on that content in
+            // order for the [autofocus] to become "visisble" and have its
+            // focus() capabilities enabled.
+            //
+            // Await this with `getUpdateComplete` so that the element cannot
+            // become "ready" until `manageFocus` has occured.
+            await nextFrame();
+            await nextFrame();
+        }
+        return complete;
+    }
+
+    private _recentlyConnected = false;
+
     public override connectedCallback(): void {
         super.connectedCallback();
+        this._recentlyConnected = true;
         this.updateComplete.then(() => {
-            requestAnimationFrame(() => {
-                this.manageAutoFocus();
-            });
+            this.manageAutoFocus();
         });
     }
 }

packages/tray/src/Tray.ts

@@ -66,8 +66,10 @@ export class Tray extends SpectrumElement {
         }
     }
 
+    private animating = false;
+
     public overlayWillCloseCallback(): boolean {
-        if (!this.open) return false;
+        if (!this.open) return this.animating;
         this.close();
         return true;
     }
@@ -89,8 +91,8 @@ export class Tray extends SpectrumElement {
 
     protected handleUnderlayTransitionend(): void {
         if (!this.open) {
-            this.dispatchClosed();
             this.resolveTransitionPromise();
+            this.dispatchClosed();
         }
     }
 
@@ -106,9 +108,13 @@ export class Tray extends SpectrumElement {
             changes.get('open') !== undefined &&
             this.prefersMotion.matches
         ) {
-            this.transitionPromise = new Promise(
-                (res) => (this.resolveTransitionPromise = res)
-            );
+            this.animating = true;
+            this.transitionPromise = new Promise((res) => {
+                this.resolveTransitionPromise = () => {
+                    this.animating = false;
+                    res();
+                };
+            });
         }
         super.update(changes);
     }

test/visual/test.ts

@@ -23,26 +23,9 @@ import { StoryDecorator } from '@spectrum-web-components/story-decorator/src/Sto
 import { html, TemplateResult } from '@spectrum-web-components/base';
 import { render } from 'lit';
 import { emulateMedia, sendKeys } from '@web/test-runner-commands';
-import { sendMouse } from '../plugins/browser.js';
+import { ignoreResizeObserverLoopError } from '../testing-helpers.js';
 
-let globalErrorHandler: undefined | OnErrorEventHandler = undefined;
-before(function () {
-    // Save Mocha's handler.
-    (
-        Mocha as unknown as { process: { removeListener(name: string): void } }
-    ).process.removeListener('uncaughtException');
-    globalErrorHandler = window.onerror;
-    addEventListener('error', (error) => {
-        if (error.message?.match?.(/ResizeObserver loop limit exceeded/)) {
-            return;
-        } else {
-            globalErrorHandler?.(error);
-        }
-    });
-});
-after(function () {
-    window.onerror = globalErrorHandler as OnErrorEventHandler;
-});
+ignoreResizeObserverLoopError(before, after);
 
 const wrap = () => html`
     <sp-story-decorator
@@ -208,17 +191,6 @@ export const regressVisuals = async (name: string, stories: TestsType) => {
                 });
             }
         });
-        beforeEach(async () => {
-            // Something about this prevents Chromium from swallowing the CSS transitions in specific contexts.
-            await sendMouse({
-                steps: [
-                    {
-                        type: 'move',
-                        position: [0, 0],
-                    },
-                ],
-            });
-        });
         afterEach(() => {
             const overlays = [
                 ...(document.querySelectorAll('active-overlay') || []),