🐛 Fix plugin modal dragging bugs (#8871)

* 🐛 Fix plugin modal drag and close interactions

Switch plugin modal dragging to pointer-capture semantics from the header so drag state remains stable when crossing iframe boundaries. Prevent drag start from close-button pointerdown and add regression tests for both non-draggable close-button interaction and close-event dispatch.

Signed-off-by: Marek Hrabe <marekhrabe@me.com>

* 📚 Update changelog for plugin modal drag fix

Document plugin modal drag and close-button interaction fixes in the unreleased changelog.

Signed-off-by: Marek Hrabe <marekhrabe@me.com>

* 🐛 Simplify plugin modal drag CSS selection rules

Keep user-select disabled at the modal wrapper level and keep touch-action scoped to the header drag handle to remove redundant declarations while preserving drag behavior.

Signed-off-by: Marek Hrabe <marekhrabe@me.com>

---------

Signed-off-by: Marek Hrabe <marekhrabe@me.com>

CHANGES.md

@@ -38,6 +38,7 @@
 - Token tree should be expanded by default [Taiga #13631](https://tree.taiga.io/project/penpot/issue/13631)
 - Fix opacity incorrectly disabled for visible shapes [Taiga #13906](https://tree.taiga.io/project/penpot/issue/13906)
 - Update onboarding image [Taiga #13864](https://tree.taiga.io/project/penpot/issue/13864)
+- Fix plugin modal drag interactions over iframe and close-button behavior (by @marekhrabe) [Github #8871](https://github.com/penpot/penpot/pull/8871)
 - Fix hot update on color-row on texts [Taiga #13923](https://tree.taiga.io/project/penpot/issue/13923)
 - Fix selected color tokens [Taiga #13930](https://tree.taiga.io/project/penpot/issue/13930)
 

plugins/libs/plugins-runtime/src/lib/drag-handler.spec.ts

@@ -1,11 +1,45 @@
 import { expect, describe, vi } from 'vitest';
 import { dragHandler } from './drag-handler.js';
 
+type PointerLikeEvent = MouseEvent & { pointerId: number };
+
+function createPointerEvent(
+  type: string,
+  init: Partial<PointerEventInit> = {},
+): PointerLikeEvent {
+  const event = new MouseEvent(type, {
+    bubbles: true,
+    cancelable: true,
+    clientX: init.clientX ?? 0,
+    clientY: init.clientY ?? 0,
+    button: init.button ?? 0,
+  }) as PointerLikeEvent;
+
+  Object.defineProperty(event, 'pointerId', {
+    configurable: true,
+    value: init.pointerId ?? 1,
+  });
+
+  return event;
+}
+
 describe('dragHandler', () => {
   let element: HTMLElement;
 
   beforeEach(() => {
     element = document.createElement('div');
+    Object.defineProperty(element, 'setPointerCapture', {
+      configurable: true,
+      value: vi.fn(),
+    });
+    Object.defineProperty(element, 'releasePointerCapture', {
+      configurable: true,
+      value: vi.fn(),
+    });
+    Object.defineProperty(element, 'hasPointerCapture', {
+      configurable: true,
+      value: vi.fn().mockReturnValue(true),
+    });
     document.body.appendChild(element);
   });
 
@@ -14,65 +48,109 @@ describe('dragHandler', () => {
     vi.clearAllMocks();
   });
 
-  it('should attach mousedown event listener to the element', () => {
+  it('should attach pointerdown event listener to the element', () => {
     const addEventListenerMock = vi.spyOn(element, 'addEventListener');
 
     dragHandler(element);
 
     expect(addEventListenerMock).toHaveBeenCalledWith(
-      'mousedown',
+      'pointerdown',
       expect.any(Function),
     );
   });
 
-  it('should update element transform on mousemove', () => {
-    const mouseDownEvent = new MouseEvent('mousedown', {
+  it('should update element transform on pointermove', () => {
+    const pointerDownEvent = createPointerEvent('pointerdown', {
       clientX: 100,
       clientY: 100,
     });
 
     dragHandler(element);
 
-    element.dispatchEvent(mouseDownEvent);
+    element.dispatchEvent(pointerDownEvent);
 
-    const mouseMoveEvent = new MouseEvent('mousemove', {
+    const pointerMoveEvent = createPointerEvent('pointermove', {
       clientX: 150,
       clientY: 150,
     });
-    document.dispatchEvent(mouseMoveEvent);
+    element.dispatchEvent(pointerMoveEvent);
 
     expect(element.style.transform).toBe('translate(50px, 50px)');
 
-    const mouseMoveEvent2 = new MouseEvent('mousemove', {
+    const pointerMoveEvent2 = createPointerEvent('pointermove', {
       clientX: 200,
       clientY: 200,
     });
-    document.dispatchEvent(mouseMoveEvent2);
+    element.dispatchEvent(pointerMoveEvent2);
 
     expect(element.style.transform).toBe('translate(100px, 100px)');
   });
 
-  it('should remove event listeners on mouseup', () => {
-    const removeEventListenerMock = vi.spyOn(document, 'removeEventListener');
-
-    const mouseDownEvent = new MouseEvent('mousedown', {
+  it('should run lifecycle callbacks on drag start/end', () => {
+    const start = vi.fn();
+    const end = vi.fn();
+    const pointerDownEvent = createPointerEvent('pointerdown', {
       clientX: 100,
       clientY: 100,
+      pointerId: 2,
+    });
+    const pointerUpEvent = createPointerEvent('pointerup', {
+      pointerId: 2,
     });
 
-    dragHandler(element);
+    dragHandler(element, element, undefined, { start, end });
+    element.dispatchEvent(pointerDownEvent);
+    element.dispatchEvent(pointerUpEvent);
 
-    element.dispatchEvent(mouseDownEvent);
+    expect(start).toHaveBeenCalledTimes(1);
+    expect(end).toHaveBeenCalledTimes(1);
+    expect(element.releasePointerCapture).toHaveBeenCalledWith(2);
+  });
 
-    const mouseUpEvent = new MouseEvent('mouseup');
-    document.dispatchEvent(mouseUpEvent);
+  it('should ignore pointerdown events from button targets', () => {
+    const start = vi.fn();
+    const button = document.createElement('button');
+    const icon = document.createElement('span');
+    button.appendChild(icon);
+    element.appendChild(button);
+
+    dragHandler(element, element, undefined, { start });
+
+    icon.dispatchEvent(
+      createPointerEvent('pointerdown', {
+        pointerId: 5,
+        button: 0,
+      }),
+    );
+
+    expect(start).not.toHaveBeenCalled();
+    expect(element.setPointerCapture).not.toHaveBeenCalled();
+  });
+
+  it('should remove pointer listeners on teardown', () => {
+    const removeEventListenerMock = vi.spyOn(element, 'removeEventListener');
+
+    const cleanup = dragHandler(element);
+    cleanup();
 
     expect(removeEventListenerMock).toHaveBeenCalledWith(
-      'mousemove',
+      'pointerdown',
       expect.any(Function),
     );
     expect(removeEventListenerMock).toHaveBeenCalledWith(
-      'mouseup',
+      'pointermove',
+      expect.any(Function),
+    );
+    expect(removeEventListenerMock).toHaveBeenCalledWith(
+      'pointerup',
+      expect.any(Function),
+    );
+    expect(removeEventListenerMock).toHaveBeenCalledWith(
+      'pointercancel',
+      expect.any(Function),
+    );
+    expect(removeEventListenerMock).toHaveBeenCalledWith(
+      'lostpointercapture',
       expect.any(Function),
     );
   });

plugins/libs/plugins-runtime/src/lib/drag-handler.ts

@@ -1,14 +1,36 @@
 import { parseTranslate } from './parse-translate';
 
+type DragHandlerLifecycle = {
+  start?: () => void;
+  end?: () => void;
+};
+
 export const dragHandler = (
   el: HTMLElement,
   target: HTMLElement = el,
   move?: () => void,
+  lifecycle?: DragHandlerLifecycle,
 ) => {
   let initialTranslate = { x: 0, y: 0 };
   let initialClientPosition = { x: 0, y: 0 };
+  let pointerId: number | null = null;
+  let dragging = false;
+
+  const endDrag = () => {
+    if (!dragging) {
+      return;
+    }
+
+    dragging = false;
+    pointerId = null;
+    lifecycle?.end?.();
+  };
+
+  const handlePointerMove = (moveEvent: PointerEvent) => {
+    if (!dragging || moveEvent.pointerId !== pointerId) {
+      return;
+    }
 
-  const handleMouseMove = (moveEvent: MouseEvent) => {
     const { clientX: moveX, clientY: moveY } = moveEvent;
     const deltaX = moveX - initialClientPosition.x + initialTranslate.x;
     const deltaY = moveY - initialClientPosition.y + initialTranslate.y;
@@ -18,19 +40,70 @@ export const dragHandler = (
     move?.();
   };
 
-  const handleMouseUp = () => {
-    document.removeEventListener('mousemove', handleMouseMove);
-    document.removeEventListener('mouseup', handleMouseUp);
+  const handlePointerUp = (upEvent: PointerEvent) => {
+    if (upEvent.pointerId !== pointerId) {
+      return;
+    }
+
+    if (el.hasPointerCapture(upEvent.pointerId)) {
+      el.releasePointerCapture(upEvent.pointerId);
+    }
+
+    endDrag();
   };
 
-  const handleMouseDown = (e: MouseEvent) => {
+  const handlePointerCancel = (cancelEvent: PointerEvent) => {
+    if (cancelEvent.pointerId !== pointerId) {
+      return;
+    }
+
+    endDrag();
+  };
+
+  const handleLostPointerCapture = (lostCaptureEvent: PointerEvent) => {
+    if (lostCaptureEvent.pointerId !== pointerId) {
+      return;
+    }
+
+    endDrag();
+  };
+
+  const handlePointerDown = (e: PointerEvent) => {
+    if (e.button !== 0) {
+      return;
+    }
+
+    const fromButton =
+      e.target instanceof Element && !!e.target.closest('button');
+
+    if (fromButton) {
+      return;
+    }
+
+    e.preventDefault();
+
     initialClientPosition = { x: e.clientX, y: e.clientY };
     initialTranslate = parseTranslate(target);
-    document.addEventListener('mousemove', handleMouseMove);
-    document.addEventListener('mouseup', handleMouseUp);
+
+    dragging = true;
+    pointerId = e.pointerId;
+    lifecycle?.start?.();
+
+    el.setPointerCapture(e.pointerId);
   };
 
-  el.addEventListener('mousedown', handleMouseDown);
+  el.addEventListener('pointerdown', handlePointerDown);
+  el.addEventListener('pointermove', handlePointerMove);
+  el.addEventListener('pointerup', handlePointerUp);
+  el.addEventListener('pointercancel', handlePointerCancel);
+  el.addEventListener('lostpointercapture', handleLostPointerCapture);
 
-  return handleMouseUp;
+  return () => {
+    el.removeEventListener('pointerdown', handlePointerDown);
+    el.removeEventListener('pointermove', handlePointerMove);
+    el.removeEventListener('pointerup', handlePointerUp);
+    el.removeEventListener('pointercancel', handlePointerCancel);
+    el.removeEventListener('lostpointercapture', handleLostPointerCapture);
+    endDrag();
+  };
 };

plugins/libs/plugins-runtime/src/lib/modal/plugin-modal.spec.ts

@@ -0,0 +1,125 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import './plugin-modal.js';
+
+type PointerLikeEvent = MouseEvent & { pointerId: number };
+
+function createPointerEvent(
+  type: string,
+  init: Partial<PointerEventInit> = {},
+): PointerLikeEvent {
+  const event = new MouseEvent(type, {
+    bubbles: true,
+    cancelable: true,
+    clientX: init.clientX ?? 0,
+    clientY: init.clientY ?? 0,
+    button: init.button ?? 0,
+  }) as PointerLikeEvent;
+
+  Object.defineProperty(event, 'pointerId', {
+    configurable: true,
+    value: init.pointerId ?? 1,
+  });
+
+  return event;
+}
+
+describe('PluginModalElement', () => {
+  let setPointerCaptureSpy: ReturnType<typeof vi.fn>;
+  let releasePointerCaptureSpy: ReturnType<typeof vi.fn>;
+  let hasPointerCaptureSpy: ReturnType<typeof vi.fn>;
+  let originalSetPointerCapture: typeof HTMLElement.prototype.setPointerCapture;
+  let originalReleasePointerCapture: typeof HTMLElement.prototype.releasePointerCapture;
+  let originalHasPointerCapture: typeof HTMLElement.prototype.hasPointerCapture;
+
+  beforeEach(() => {
+    originalSetPointerCapture = HTMLElement.prototype.setPointerCapture;
+    originalReleasePointerCapture = HTMLElement.prototype.releasePointerCapture;
+    originalHasPointerCapture = HTMLElement.prototype.hasPointerCapture;
+
+    setPointerCaptureSpy = vi.fn();
+    releasePointerCaptureSpy = vi.fn();
+    hasPointerCaptureSpy = vi.fn().mockReturnValue(true);
+
+    Object.defineProperty(HTMLElement.prototype, 'setPointerCapture', {
+      configurable: true,
+      value: setPointerCaptureSpy,
+    });
+    Object.defineProperty(HTMLElement.prototype, 'releasePointerCapture', {
+      configurable: true,
+      value: releasePointerCaptureSpy,
+    });
+    Object.defineProperty(HTMLElement.prototype, 'hasPointerCapture', {
+      configurable: true,
+      value: hasPointerCaptureSpy,
+    });
+  });
+
+  afterEach(() => {
+    document.body.innerHTML = '';
+    Object.defineProperty(HTMLElement.prototype, 'setPointerCapture', {
+      configurable: true,
+      value: originalSetPointerCapture,
+    });
+    Object.defineProperty(HTMLElement.prototype, 'releasePointerCapture', {
+      configurable: true,
+      value: originalReleasePointerCapture,
+    });
+    Object.defineProperty(HTMLElement.prototype, 'hasPointerCapture', {
+      configurable: true,
+      value: originalHasPointerCapture,
+    });
+    vi.restoreAllMocks();
+  });
+
+  it('should not start dragging on close button pointerdown', () => {
+    const modal = document.createElement('plugin-modal');
+    modal.setAttribute('title', 'Test modal');
+    modal.setAttribute('iframe-src', 'about:blank');
+    document.body.appendChild(modal);
+
+    const shadow = modal.shadowRoot;
+    expect(shadow).toBeTruthy();
+
+    const wrapper = shadow?.querySelector<HTMLElement>('.wrapper');
+    const closeButton = shadow?.querySelector<HTMLElement>('button');
+
+    expect(wrapper).toBeTruthy();
+    expect(closeButton).toBeTruthy();
+
+    closeButton?.dispatchEvent(
+      createPointerEvent('pointerdown', {
+        pointerId: 11,
+        button: 0,
+      }),
+    );
+
+    expect(wrapper?.classList.contains('is-dragging')).toBe(false);
+    expect(setPointerCaptureSpy).not.toHaveBeenCalled();
+
+    modal.remove();
+  });
+
+  it('should dispatch close event when close button is clicked', () => {
+    const modal = document.createElement('plugin-modal');
+    modal.setAttribute('title', 'Test modal');
+    modal.setAttribute('iframe-src', 'about:blank');
+
+    const onClose = vi.fn();
+    modal.addEventListener('close', onClose);
+    document.body.appendChild(modal);
+
+    const closeButton = modal.shadowRoot?.querySelector<HTMLElement>('button');
+    expect(closeButton).toBeTruthy();
+
+    closeButton?.dispatchEvent(
+      new MouseEvent('click', {
+        bubbles: true,
+        cancelable: true,
+      }),
+    );
+
+    expect(onClose).toHaveBeenCalledTimes(1);
+
+    modal.remove();
+  });
+});

plugins/libs/plugins-runtime/src/lib/modal/plugin-modal.ts

@@ -67,11 +67,6 @@ export class PluginModalElement extends HTMLElement {
     this.wrapper.style.maxInlineSize = '90vw';
     this.wrapper.style.maxBlockSize = '90vh';
 
-    // move modal to the top
-    this.#dragEvents = dragHandler(this.#inner, this.wrapper, () => {
-      this.calculateZIndex();
-    });
-
     const header = document.createElement('div');
     header.classList.add('header');
 
@@ -124,6 +119,23 @@ export class PluginModalElement extends HTMLElement {
       );
     });
 
+    // move modal to the top
+    this.#dragEvents = dragHandler(
+      header,
+      this.wrapper,
+      () => {
+        this.calculateZIndex();
+      },
+      {
+        start: () => {
+          this.wrapper.classList.add('is-dragging');
+        },
+        end: () => {
+          this.wrapper.classList.remove('is-dragging');
+        },
+      },
+    );
+
     this.addEventListener('message', (e: Event) => {
       if (!iframe.contentWindow) {
         return;

plugins/libs/plugins-runtime/src/lib/modal/plugin.modal.css

@@ -42,6 +42,8 @@
   min-inline-size: 25px;
   min-block-size: 200px;
   resize: both;
+  user-select: none;
+  -webkit-user-select: none;
   &:after {
     content: '';
     cursor: se-resize;
@@ -58,7 +60,6 @@
 
 .inner {
   padding: 10px;
-  cursor: grab;
   box-sizing: border-box;
   display: flex;
   flex-direction: column;
@@ -78,6 +79,12 @@
   justify-content: space-between;
   border-block-end: 2px solid var(--color-background-quaternary);
   padding-block-end: var(--spacing-4);
+  cursor: grab;
+  touch-action: none;
+}
+
+.wrapper.is-dragging .header {
+  cursor: grabbing;
 }
 
 button {
@@ -92,7 +99,6 @@ h1 {
   font-weight: var(--font-weight-bold);
   margin: 0;
   margin-inline-end: var(--spacing-4);
-  user-select: none;
 }
 
 iframe {