diff --git a/packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx b/packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx index 3a3272e0..a7a56373 100644 --- a/packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx +++ b/packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx @@ -1,6 +1,6 @@ import { PointerEvent, ReactNode, useCallback, useEffect, useMemo, useState } from 'react'; import { useAnnotator, useSelection } from '@annotorious/react'; -import type { TextAnnotation, TextAnnotator } from '@recogito/text-annotator'; +import { isRevived, TextAnnotation, TextAnnotator } from '@recogito/text-annotator'; import { isMobile } from './isMobile'; import { autoUpdate, @@ -71,15 +71,10 @@ export const TextAnnotatorPopup = (props: TextAnnotationPopupProps) => { const { getFloatingProps } = useInteractions([dismiss, role]); useEffect(() => { - setOpen( - // Selected annotation exists and has a selector? - annotation?.target.selector && - // Selector not empty? (Annotations from plugins, general defensive programming) - annotation.target.selector.length > 0 && - // Range not collapsed? (E.g. lazy loading PDFs. Note that this will have to - // change if we switch from ranges to pre-computed bounds!) - !annotation.target.selector[0].range.collapsed - ); + const annotationSelector = annotation?.target.selector; + if (!annotationSelector) return; + + setOpen(isRevived(annotationSelector)); }, [annotation]); useEffect(() => { @@ -154,4 +149,4 @@ export const TextAnnotatorPopup = (props: TextAnnotationPopupProps) => { ) : null; -} \ No newline at end of file +} diff --git a/packages/text-annotator/src/SelectionHandler.ts b/packages/text-annotator/src/SelectionHandler.ts index 801e0f1c..d198d4bb 100644 --- a/packages/text-annotator/src/SelectionHandler.ts +++ b/packages/text-annotator/src/SelectionHandler.ts @@ -13,7 +13,7 @@ import { isMac, isWhitespaceOrEmpty, trimRangeToContainer, - NOT_ANNOTATABLE_SELECTOR + isNotAnnotatable } from './utils'; const CLICK_TIMEOUT = 300; @@ -64,13 +64,14 @@ export const SelectionHandler = ( * be annotatable (like a component popup). * Note that Chrome/iOS will sometimes return the root doc as target! */ - const annotatable = !(evt.target as Node).parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); - currentTarget = annotatable ? { - annotation: uuidv4(), - selector: [], - creator: currentUser, - created: new Date() - } : undefined; + currentTarget = isNotAnnotatable(evt.target as Node) + ? undefined + : { + annotation: uuidv4(), + selector: [], + creator: currentUser, + created: new Date() + }; }; const onSelectionChange = debounce((evt: Event) => { @@ -79,8 +80,7 @@ export const SelectionHandler = ( // This is to handle cases where the selection is "hijacked" by another element // in a not-annotatable area. A rare case in theory. But rich text editors // will like Quill do it... - const annotatable = !sel.anchorNode?.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); - if (!annotatable) { + if (isNotAnnotatable(sel.anchorNode)) { currentTarget = undefined; return; } @@ -149,6 +149,9 @@ export const SelectionHandler = ( */ if (store.getAnnotation(currentTarget.annotation)) { store.updateTarget(currentTarget, Origin.LOCAL); + } else { + // Proper lifecycle management: clear the previous selection first... + selection.clear(); } }); @@ -160,8 +163,7 @@ export const SelectionHandler = ( const onPointerDown = (evt: PointerEvent) => { if (isContextMenuOpen) return; - const annotatable = !(evt.target as Node).parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); - if (!annotatable) return; + if (isNotAnnotatable(evt.target as Node)) return; /** * Cloning the event to prevent it from accidentally being `undefined` @@ -188,8 +190,7 @@ export const SelectionHandler = ( const onPointerUp = (evt: PointerEvent) => { if (isContextMenuOpen) return; - const annotatable = !(evt.target as Node).parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); - if (!annotatable || !isLeftClick) return; + if (isNotAnnotatable(evt.target as Node) || !isLeftClick) return; // Logic for selecting an existing annotation const clickSelect = () => { @@ -206,7 +207,7 @@ export const SelectionHandler = ( if (selected.length !== 1 || selected[0].id !== hovered.id) { selection.userSelect(hovered.id, evt); } - } else if (!selection.isEmpty()) { + } else { selection.clear(); } }; diff --git a/packages/text-annotator/src/utils/cancelSingleClickEvents.ts b/packages/text-annotator/src/utils/cancelSingleClickEvents.ts index 7d671988..acc4943d 100644 --- a/packages/text-annotator/src/utils/cancelSingleClickEvents.ts +++ b/packages/text-annotator/src/utils/cancelSingleClickEvents.ts @@ -1,4 +1,4 @@ -import { NOT_ANNOTATABLE_SELECTOR } from './splitAnnotatableRanges'; +import { NOT_ANNOTATABLE_SELECTOR } from './isNotAnnotatable'; /** * Calls .preventDefault() on click events in annotable areas, in order diff --git a/packages/text-annotator/src/utils/getAnnotatableFragment.ts b/packages/text-annotator/src/utils/getAnnotatableFragment.ts deleted file mode 100644 index 98b87c11..00000000 --- a/packages/text-annotator/src/utils/getAnnotatableFragment.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { NOT_ANNOTATABLE_SELECTOR } from './splitAnnotatableRanges'; - -/** - * Returns a DocumentFragment consisting of ONLY the annotatable content - * in the given DOM Range. - */ -export const getAnnotatableFragment = (range: Range): DocumentFragment => { - const fragment = range.cloneContents(); - fragment.querySelectorAll(NOT_ANNOTATABLE_SELECTOR).forEach(el => el.remove()); - return fragment; -} diff --git a/packages/text-annotator/src/utils/index.ts b/packages/text-annotator/src/utils/index.ts index fe84a63b..2105761f 100644 --- a/packages/text-annotator/src/utils/index.ts +++ b/packages/text-annotator/src/utils/index.ts @@ -1,11 +1,12 @@ export * from './cancelSingleClickEvents'; +export * from './cloneEvents'; export * from './device'; export * from './programmaticallyFocusable'; export * from './debounce'; -export * from './getAnnotatableFragment'; export * from './getQuoteContext'; -export * from './isWhitespaceOrEmpty'; +export * from './isNotAnnotatable'; export * from './isRevived'; +export * from './isWhitespaceOrEmpty'; export * from './mergeClientRects'; export * from './rangeToSelector'; export * from './reviveAnnotation'; @@ -13,5 +14,5 @@ export * from './reviveSelector'; export * from './reviveTarget'; export * from './splitAnnotatableRanges'; export * from './trimRangeToContainer'; -export * from './cloneEvents'; + diff --git a/packages/text-annotator/src/utils/isNotAnnotatable.ts b/packages/text-annotator/src/utils/isNotAnnotatable.ts new file mode 100644 index 00000000..ece9d7bd --- /dev/null +++ b/packages/text-annotator/src/utils/isNotAnnotatable.ts @@ -0,0 +1,15 @@ +export const NOT_ANNOTATABLE_CLASS = 'not-annotatable'; + +export const NOT_ANNOTATABLE_SELECTOR = `.${NOT_ANNOTATABLE_CLASS}`; + +export const isNotAnnotatable = (node: Node): boolean => { + const closestNotAnnotatable = node instanceof HTMLElement + ? node.closest(NOT_ANNOTATABLE_SELECTOR) + : node.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); + return Boolean(closestNotAnnotatable); +} + +export const isRangeAnnotatable = (range: Range): boolean => { + const ancestor = range.commonAncestorContainer; + return !isNotAnnotatable(ancestor); +} \ No newline at end of file diff --git a/packages/text-annotator/src/utils/reviveSelector.ts b/packages/text-annotator/src/utils/reviveSelector.ts index b3990053..c0ee7756 100644 --- a/packages/text-annotator/src/utils/reviveSelector.ts +++ b/packages/text-annotator/src/utils/reviveSelector.ts @@ -1,5 +1,5 @@ import type { TextSelector } from '../model'; -import { NOT_ANNOTATABLE_SELECTOR } from './splitAnnotatableRanges'; +import { NOT_ANNOTATABLE_SELECTOR } from './isNotAnnotatable'; /** * Creates a new selector object with the revived DOM range from the given text annotation position diff --git a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts index d91f45da..4269b9e0 100644 --- a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts +++ b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts @@ -1,13 +1,4 @@ -export const NOT_ANNOTATABLE_CLASS = 'not-annotatable'; - -export const NOT_ANNOTATABLE_SELECTOR = `.${NOT_ANNOTATABLE_CLASS}`; - -const isRangeAnnotatable = (range: Range): boolean => { - const ancestor = range.commonAncestorContainer; - return ancestor instanceof HTMLElement - ? !ancestor.closest(NOT_ANNOTATABLE_SELECTOR) - : !ancestor.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); -} +import { isRangeAnnotatable, NOT_ANNOTATABLE_CLASS, NOT_ANNOTATABLE_SELECTOR } from './isNotAnnotatable'; const iterateNotAnnotatableElements = function*(range: Range): Generator { const notAnnotatableIterator = document.createNodeIterator( diff --git a/packages/text-annotator/test/model/w3c/fixtures.ts b/packages/text-annotator/test/model/w3c/fixtures.ts index 7ce75045..142fb168 100644 --- a/packages/text-annotator/test/model/w3c/fixtures.ts +++ b/packages/text-annotator/test/model/w3c/fixtures.ts @@ -28,8 +28,8 @@ export const textAnnotation: W3CTextAnnotation = { }, { type: 'TextPositionSelector', - start: 986, - end: 998 + start: 958, + end: 970 }, ] }