From c326981c6ca8d62f15f7ee6258406213224cca78 Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Mon, 11 May 2026 17:59:34 -0700 Subject: [PATCH 1/3] fix(annotations): portal annotation popup so it stays upright on rotate --- src/common/BaseAnnotator.scss | 11 ++++++ src/common/DeselectListener.tsx | 2 +- src/components/Popups/PopupDrawingToolbar.tsx | 6 ---- src/document/DocumentAnnotator.ts | 3 +- src/drawing/DrawingAnnotations.tsx | 34 +++++++++++-------- src/drawing/DrawingManager.tsx | 15 ++++++-- src/image/ImageAnnotator.ts | 3 +- src/popup/PopupLayer.tsx | 4 ++- 8 files changed, 52 insertions(+), 26 deletions(-) diff --git a/src/common/BaseAnnotator.scss b/src/common/BaseAnnotator.scss index 43d86de9f..c6258c09d 100644 --- a/src/common/BaseAnnotator.scss +++ b/src/common/BaseAnnotator.scss @@ -28,3 +28,14 @@ .ba-Layer--popup { z-index: 4; } + +.ba-PopupPortal { + position: relative; + z-index: 10; + pointer-events: none; + + .ba-Popup { + z-index: 150; + pointer-events: auto; + } +} diff --git a/src/common/DeselectListener.tsx b/src/common/DeselectListener.tsx index ff3604d65..a28353372 100644 --- a/src/common/DeselectListener.tsx +++ b/src/common/DeselectListener.tsx @@ -8,7 +8,7 @@ export default function DeselectListener(): null { React.useEffect(() => { const handleMouseDown = (event: MouseEvent): void => { // Popup is portaled outside .ba-Layer; native mousedown still bubbles here. - if ((event.target as Element | null)?.closest?.('.ba-PopupV2')) return; + if ((event.target as Element | null)?.closest?.('.ba-PopupV2, .ba-Popup')) return; dispatch(setActiveAnnotationIdAction(null)); }; diff --git a/src/components/Popups/PopupDrawingToolbar.tsx b/src/components/Popups/PopupDrawingToolbar.tsx index 5e4341659..b79c0ecc6 100644 --- a/src/components/Popups/PopupDrawingToolbar.tsx +++ b/src/components/Popups/PopupDrawingToolbar.tsx @@ -33,12 +33,6 @@ export type Props = { const options: Partial = { modifiers: [ - { - name: 'eventListeners', - options: { - scroll: false, - }, - }, { name: 'offset', options: { diff --git a/src/document/DocumentAnnotator.ts b/src/document/DocumentAnnotator.ts index 5402a2259..18b3016e7 100644 --- a/src/document/DocumentAnnotator.ts +++ b/src/document/DocumentAnnotator.ts @@ -103,7 +103,8 @@ export default class DocumentAnnotator extends BaseAnnotator { // Annotations mode managers.add(new PopupManager({ location: pageNumber, popupPortalEl: this.popupPortalEl, referenceEl: pageReferenceEl, resinTags })); - managers.add(new DrawingManager({ location: pageNumber, referenceEl: pageReferenceEl, resinTags })); + managers.add(new DrawingManager({ location: pageNumber, popupPortalEl: this.popupPortalEl, referenceEl: pageReferenceEl, resinTags })); + const textLayer = pageEl.querySelector('.textLayer') as HTMLElement; diff --git a/src/drawing/DrawingAnnotations.tsx b/src/drawing/DrawingAnnotations.tsx index c5b1f12cc..bb3946e73 100644 --- a/src/drawing/DrawingAnnotations.tsx +++ b/src/drawing/DrawingAnnotations.tsx @@ -1,4 +1,5 @@ import React from 'react'; +import ReactDOM from 'react-dom'; import classNames from 'classnames'; import DecoratedDrawingPath from './DecoratedDrawingPath'; import DrawingList from './DrawingList'; @@ -22,6 +23,7 @@ export type Props = { drawnPathGroups: Array; isCreating: boolean; location: number; + popupPortalEl?: HTMLElement | null; redoDrawingPathGroup: () => void; resetDrawing: () => void; rotation: number; @@ -46,6 +48,7 @@ const DrawingAnnotations = (props: Props): JSX.Element => { drawnPathGroups, isCreating, location, + popupPortalEl, redoDrawingPathGroup, resetDrawing, rotation, @@ -176,20 +179,23 @@ const DrawingAnnotations = (props: Props): JSX.Element => { /> )} - {isCreating && hasPathGroups && drawingSVGGroup && canShowPopupToolbar && ( - - )} + {isCreating && hasPathGroups && drawingSVGGroup && canShowPopupToolbar && (() => { + const toolbar = ( + + ); + return popupPortalEl ? ReactDOM.createPortal(toolbar, popupPortalEl) : toolbar; + })()} ); }; diff --git a/src/drawing/DrawingManager.tsx b/src/drawing/DrawingManager.tsx index 7eceb31fe..2e6e1de31 100644 --- a/src/drawing/DrawingManager.tsx +++ b/src/drawing/DrawingManager.tsx @@ -1,9 +1,20 @@ import * as React from 'react'; import * as ReactDOM from 'react-dom/client'; -import BaseManager, { Props } from '../common/BaseManager'; +import BaseManager, { Options as BaseOptions, Props } from '../common/BaseManager'; import DrawingAnnotationsContainer from './DrawingAnnotationsContainer'; +export type Options = BaseOptions & { + popupPortalEl?: HTMLElement | null; +}; + export default class DrawingListManager extends BaseManager { + popupPortalEl?: HTMLElement | null; + + constructor({ popupPortalEl, ...options }: Options) { + super(options); + this.popupPortalEl = popupPortalEl; + } + decorate(): void { this.reactEl.classList.add('ba-Layer--drawing'); this.reactEl.dataset.testid = 'ba-Layer--drawing'; @@ -14,6 +25,6 @@ export default class DrawingListManager extends BaseManager { this.root = ReactDOM.createRoot(this.reactEl); } - this.root.render(); + this.root.render(); } } diff --git a/src/image/ImageAnnotator.ts b/src/image/ImageAnnotator.ts index 12d785655..424cea53b 100644 --- a/src/image/ImageAnnotator.ts +++ b/src/image/ImageAnnotator.ts @@ -74,7 +74,8 @@ export default class ImageAnnotator extends BaseAnnotator { } this.managers.add(new PopupManager({ popupPortalEl: this.popupPortalEl, referenceEl, resinTags })); - this.managers.add(new DrawingManager({ referenceEl, resinTags })); + this.managers.add(new DrawingManager({ popupPortalEl: this.popupPortalEl, referenceEl, resinTags })); + this.managers.add(new RegionManager({ referenceEl, resinTags })); this.managers.add(new RegionCreationManager({ referenceEl, resinTags })); } diff --git a/src/popup/PopupLayer.tsx b/src/popup/PopupLayer.tsx index 104c29715..30a7383a8 100644 --- a/src/popup/PopupLayer.tsx +++ b/src/popup/PopupLayer.tsx @@ -1,4 +1,5 @@ import * as React from 'react'; +import * as ReactDOM from 'react-dom'; import noop from 'lodash/noop'; import PopupReply from '../components/Popups/PopupReply'; import PopupV2 from '../components/Popups/PopupV2'; @@ -121,7 +122,7 @@ const PopupLayer = (props: Props): JSX.Element | null => { } if (showCreator) { - return ( + const replyPopup = (
{ />
); + return popupPortalEl ? ReactDOM.createPortal(replyPopup, popupPortalEl) : replyPopup; } if (isThreadedAnnotation && activeAnnotationId && activeReference) { From 9b143440dcd78963718e5d83a3ca172d0422af39 Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Tue, 12 May 2026 16:06:59 -0700 Subject: [PATCH 2/3] fix(popup): PopupDrawingToolbar behavior on scroll --- src/components/Popups/PopupDrawingToolbar.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Popups/PopupDrawingToolbar.tsx b/src/components/Popups/PopupDrawingToolbar.tsx index b79c0ecc6..177f94131 100644 --- a/src/components/Popups/PopupDrawingToolbar.tsx +++ b/src/components/Popups/PopupDrawingToolbar.tsx @@ -42,7 +42,6 @@ const options: Partial = { { name: 'preventOverflow', options: { - altAxis: true, padding: 50, }, }, From d0559fb44af47331ec895ef4ee2fc80477983dbe Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Tue, 12 May 2026 16:40:25 -0700 Subject: [PATCH 3/3] fix(popup): add unit tests and return null if no popupPortalEl --- src/document/DocumentAnnotator.ts | 1 - src/drawing/DrawingAnnotations.tsx | 11 ++++--- .../__tests__/DrawingAnnotations-test.tsx | 30 +++++++++++++++++++ src/image/ImageAnnotator.ts | 1 - src/popup/PopupLayer.tsx | 7 +++-- src/popup/__tests__/PopupLayer-test.tsx | 17 +++++++++++ 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/document/DocumentAnnotator.ts b/src/document/DocumentAnnotator.ts index 18b3016e7..cd01b7fa1 100644 --- a/src/document/DocumentAnnotator.ts +++ b/src/document/DocumentAnnotator.ts @@ -105,7 +105,6 @@ export default class DocumentAnnotator extends BaseAnnotator { managers.add(new PopupManager({ location: pageNumber, popupPortalEl: this.popupPortalEl, referenceEl: pageReferenceEl, resinTags })); managers.add(new DrawingManager({ location: pageNumber, popupPortalEl: this.popupPortalEl, referenceEl: pageReferenceEl, resinTags })); - const textLayer = pageEl.querySelector('.textLayer') as HTMLElement; if (textLayer) { diff --git a/src/drawing/DrawingAnnotations.tsx b/src/drawing/DrawingAnnotations.tsx index bb3946e73..c5b988fe8 100644 --- a/src/drawing/DrawingAnnotations.tsx +++ b/src/drawing/DrawingAnnotations.tsx @@ -179,8 +179,8 @@ const DrawingAnnotations = (props: Props): JSX.Element => { /> )} - {isCreating && hasPathGroups && drawingSVGGroup && canShowPopupToolbar && (() => { - const toolbar = ( + {isCreating && hasPathGroups && drawingSVGGroup && canShowPopupToolbar && popupPortalEl && + ReactDOM.createPortal( { onReply={handleReply} onUndo={handleUndo} reference={drawingSVGGroup} - /> - ); - return popupPortalEl ? ReactDOM.createPortal(toolbar, popupPortalEl) : toolbar; - })()} + />, + popupPortalEl, + )} ); }; diff --git a/src/drawing/__tests__/DrawingAnnotations-test.tsx b/src/drawing/__tests__/DrawingAnnotations-test.tsx index 9146f0673..5a71f5349 100644 --- a/src/drawing/__tests__/DrawingAnnotations-test.tsx +++ b/src/drawing/__tests__/DrawingAnnotations-test.tsx @@ -38,6 +38,7 @@ describe('DrawingAnnotations', () => { drawnPathGroups: [], isCreating: false, location: 0, + popupPortalEl: document.createElement('div'), redoDrawingPathGroup: jest.fn(), resetDrawing: jest.fn(), rotation: 0, @@ -319,6 +320,35 @@ describe('DrawingAnnotations', () => { ); }); + describe('popup portal', () => { + test('should render PopupDrawingToolbar into popupPortalEl', () => { + const popupPortalEl = document.createElement('div'); + document.body.appendChild(popupPortalEl); + + getWrapper({ + canShowPopupToolbar: true, + drawnPathGroups: pathGroups, + isCreating: true, + popupPortalEl, + }); + + expect(popupPortalEl.querySelector('.ba-PopupDrawingToolbar')).toBeTruthy(); + + document.body.removeChild(popupPortalEl); + }); + + test('should not render PopupDrawingToolbar when popupPortalEl is missing', () => { + const wrapper = getWrapper({ + canShowPopupToolbar: true, + drawnPathGroups: pathGroups, + isCreating: true, + popupPortalEl: null, + }); + + expect(wrapper.find(PopupDrawingToolbar).exists()).toBe(false); + }); + }); + // Use shared video annotation tests createVideoAnnotationTests({ componentName: 'DrawingAnnotations', diff --git a/src/image/ImageAnnotator.ts b/src/image/ImageAnnotator.ts index 424cea53b..1bd6c2291 100644 --- a/src/image/ImageAnnotator.ts +++ b/src/image/ImageAnnotator.ts @@ -75,7 +75,6 @@ export default class ImageAnnotator extends BaseAnnotator { this.managers.add(new PopupManager({ popupPortalEl: this.popupPortalEl, referenceEl, resinTags })); this.managers.add(new DrawingManager({ popupPortalEl: this.popupPortalEl, referenceEl, resinTags })); - this.managers.add(new RegionManager({ referenceEl, resinTags })); this.managers.add(new RegionCreationManager({ referenceEl, resinTags })); } diff --git a/src/popup/PopupLayer.tsx b/src/popup/PopupLayer.tsx index 30a7383a8..39465997f 100644 --- a/src/popup/PopupLayer.tsx +++ b/src/popup/PopupLayer.tsx @@ -122,7 +122,8 @@ const PopupLayer = (props: Props): JSX.Element | null => { } if (showCreator) { - const replyPopup = ( + if (!popupPortalEl) return null; + return ReactDOM.createPortal(
{ reference={reference} value={message} /> -
+ , + popupPortalEl, ); - return popupPortalEl ? ReactDOM.createPortal(replyPopup, popupPortalEl) : replyPopup; } if (isThreadedAnnotation && activeAnnotationId && activeReference) { diff --git a/src/popup/__tests__/PopupLayer-test.tsx b/src/popup/__tests__/PopupLayer-test.tsx index 10933dfc3..0aa39a7d1 100644 --- a/src/popup/__tests__/PopupLayer-test.tsx +++ b/src/popup/__tests__/PopupLayer-test.tsx @@ -56,6 +56,7 @@ describe('PopupLayer', () => { }); const referenceId = '123'; + const popupPortalEl = document.createElement('div'); const getDefaults = (): Props => ({ activeAnnotationId: null, createDrawing: jest.fn(), @@ -65,6 +66,7 @@ describe('PopupLayer', () => { location: 1, message: '', mode: Mode.HIGHLIGHT, + popupPortalEl, referenceId: '123', resetCreator: jest.fn(), setMessage: jest.fn(), @@ -75,6 +77,7 @@ describe('PopupLayer', () => { beforeEach(() => { document.body.innerHTML = `
`; + document.body.appendChild(popupPortalEl); mockOnCancel = undefined; mockOnChange = undefined; mockOnSubmit = undefined; @@ -151,6 +154,20 @@ describe('PopupLayer', () => { }); }); + describe('popup portal', () => { + test('should render PopupReply into popupPortalEl', () => { + renderLayer(); + + expect(popupPortalEl.querySelector('[data-testid="popup-reply"]')).toBeTruthy(); + }); + + test('should render nothing when popupPortalEl is missing', () => { + renderLayer({ popupPortalEl: null }); + + expect(screen.queryByTestId('popup-reply')).toBeNull(); + }); + }); + describe('event handlers', () => { describe('handleCancel()', () => { test('should reset creator and reset isPromoting', () => {