fix(overlays): avoid native dialog esc interference

.changeset/stale-camels-fry.md

@@ -0,0 +1,7 @@
+---
+'@lion/ui': minor
+---
+
+[overlays]: avoid interference of native dialog escape handler and escape handlers defined by OverlayController.
+This is needed until we can configure `closedby="none"` on the native dialog for all browsers: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dialog#closedby.
+(We release this as a minor change, as we stop propagation of HTMLDialogElement 'cancel' and 'close' events, and some consumers might (ab)use them...)

packages/ui/components/overlays/src/OverlayController.js

@@ -528,8 +528,6 @@ export class OverlayController extends EventTarget {
       this.__contentHasBeenInitialized = true;
     }
 
-    this.__wrappingDialogNode?.addEventListener('cancel', this.__cancelHandler);
-
     // Reset all positioning styles (local, c.q. Popper) and classes (global)
     this.contentWrapperNode.removeAttribute('style');
     this.contentWrapperNode.removeAttribute('class');
@@ -618,8 +616,9 @@ export class OverlayController extends EventTarget {
       contentWrapperNodeL2: this.contentWrapperNode,
       contentNodeL3: this.contentNode,
     });
-    // @ts-ignore
+    // @ts-expect-error
     wrappingDialogElement.open = true;
+
     if (this.isTooltip) {
       // needed to prevent tooltip getting focus in Safari and Firefox
       wrappingDialogElement.setAttribute('tabindex', '-1');
@@ -633,6 +632,35 @@ export class OverlayController extends EventTarget {
       // computed height of 0...
       this.contentNode.style.position = 'static';
     }
+
+    // Here we prevent any interference of the native <dialog> element with the keyboard behavior
+    // as defined by the OverlayController. This is needed until we can configure `closedby="none"`
+    // on the native dialog for all browsers: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dialog#closedby
+    const hasClosedBySupport = HTMLDialogElement && 'closedBy' in HTMLDialogElement.prototype;
+    if (hasClosedBySupport) {
+      // @ts-expect-error
+      wrappingDialogElement.closedBy = 'none';
+    } else {
+      wrappingDialogElement.addEventListener(
+        'keydown',
+        (/** @type {* & KeyboardEvent} */ event) => {
+          if (event.key === 'Escape') {
+            event.preventDefault();
+          }
+        },
+      );
+      wrappingDialogElement.addEventListener('keyup', (/** @type {* & KeyboardEvent} */ event) => {
+        if (event.key === 'Escape') {
+          event.preventDefault();
+        }
+      });
+      wrappingDialogElement.addEventListener('cancel', event => {
+        event.stopPropagation();
+      });
+      wrappingDialogElement.addEventListener('close', event => {
+        event.stopPropagation();
+      });
+    }
   }
 
   /**
@@ -1188,6 +1216,7 @@ export class OverlayController extends EventTarget {
     const hasPressedInside =
       event.composedPath().includes(this.contentNode) ||
       deepContains(this.contentNode, /** @type {HTMLElement|ShadowRoot} */ (event.target));
+
     if (hasPressedInside) {
       this.hide();
       // We could do event.stopPropagation() here, but we don't want to hide info for
@@ -1384,8 +1413,6 @@ export class OverlayController extends EventTarget {
   teardown() {
     this.__handleOverlayStyles({ phase: 'teardown' });
     this._handleFeatures({ phase: 'teardown' });
-    this.__wrappingDialogNode?.removeEventListener('cancel', this.__cancelHandler);
-
     if (this.#isRegisteredOnManager()) {
       this.manager.remove(this);
     }

packages/ui/components/overlays/test-suites/OverlayMixin.suite.js

@@ -9,6 +9,7 @@ import {
   html,
 } from '@open-wc/testing';
 import { overlays as overlaysManager, OverlayController } from '@lion/ui/overlays.js';
+import { sendKeys } from '@web/test-runner-commands';
 import { browserDetection } from '@lion/ui/core.js';
 import { cache } from 'lit/directives/cache.js';
 import '@lion/ui/define/lion-dialog.js';
@@ -31,6 +32,28 @@ function resetOverlaysManager() {
   overlaysManager.list.forEach(overlayCtrl => overlaysManager.remove(overlayCtrl));
 }
 
+/**
+ * A small wrapper function that closely mimics an escape press from a user
+ * (prevents common mistakes like no bubbling or keydown)
+ * @param {HTMLElement|Document} element
+ */
+async function mimicEscapePress(element) {
+  // Make sure that the element inside the dialog is focusable (and cleanup after)
+  if (element instanceof HTMLElement) {
+    const { tabIndex: tabIndexBefore } = element;
+    // eslint-disable-next-line no-param-reassign
+    element.tabIndex = -1;
+    element.focus();
+    // eslint-disable-next-line no-param-reassign
+    element.tabIndex = tabIndexBefore; // make sure element is focusable
+  }
+
+  // Send the event
+  await sendKeys({ press: 'Escape' });
+  // Wait for at least a microtask, so that possible property effects are performed
+  await aTimeout(0);
+}
+
 /**
  * @param {{tagString:string, tag: object, suffix?:string}} config
  */
@@ -367,6 +390,26 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) {
       stub.restore();
     });
 
+    it('should not visually hide when hidesOnEsc / hidesOnOutsideEsc is not configured', async () => {
+      const el = /** @type {OverlayEl} */ (
+        await fixture(html`
+        <${tag} .config="${{ hidesOnEsc: false, hidesOnOutsideEsc: false }}" opened>
+          <div slot="content">content of the overlay</div>
+          <button slot="invoker">invoker button</button>
+        </${tag}>
+      `)
+      );
+
+      const dialogEl = /** @type {HTMLDialogElement} */ (el._overlayCtrl.__wrappingDialogNode);
+
+      // @ts-expect-error [allow-protected-in-tests]
+      expect(dialogEl.checkVisibility()).to.be.true;
+      // @ts-expect-error [allow-protected-in-tests]
+      await mimicEscapePress(el._overlayContentNode);
+      // @ts-expect-error [allow-protected-in-tests]
+      expect(dialogEl.checkVisibility()).to.be.true;
+    });
+
     describe('Teardown', () => {
       it('does not teardown when moving dom nodes', async () => {
         const el = /** @type {OverlayEl} */ (

packages/ui/components/overlays/test/OverlayController.test.js

@@ -1,6 +1,7 @@
 /* eslint-disable no-new */
 import { OverlayController, overlays } from '@lion/ui/overlays.js';
 import { mimicClick } from '@lion/ui/overlays-test-helpers.js';
+import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import {
   unsafeStatic,
@@ -28,12 +29,22 @@ const wrappingDialogNodeStyle = 'display: none; z-index: 9999; padding: 0px;';
 /**
  * A small wrapper function that closely mimics an escape press from a user
  * (prevents common mistakes like no bubbling or keydown)
- * @param {Element|Document} element
+ * @param {HTMLElement|Document} element
  */
 async function mimicEscapePress(element) {
-  element.dispatchEvent(
+  // Make sure that the element inside the dialog is focusable (and cleanup after)
-    new KeyboardEvent('keyup', { key: 'Escape', bubbles: true, composed: true }),
+  if (element instanceof HTMLElement) {
-  );
+    const { tabIndex: tabIndexBefore } = element;
+    // eslint-disable-next-line no-param-reassign
+    element.tabIndex = -1;
+    element.focus();
+    // eslint-disable-next-line no-param-reassign
+    element.tabIndex = tabIndexBefore; // make sure element is focusable
+  }
+
+  // Send the event
+  await sendKeys({ press: 'Escape' });
+  // Wait for at least a microtask, so that possible property effects are performed
   await aTimeout(0);
 }
 
@@ -726,6 +737,7 @@ describe('OverlayController', () => {
           )
         );
         const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent);
+
         await mimicEscapePress(childOverlay.contentNode);
 
         expect(parentOverlay.isShown).to.be.true;