From 23f762a5a473e9c4328a7c373619baef0e698af4 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Thu, 10 Oct 2024 18:24:35 +0300 Subject: [PATCH 1/9] Restored selection cleanup on selection change --- packages/text-annotator/src/SelectionHandler.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/text-annotator/src/SelectionHandler.ts b/packages/text-annotator/src/SelectionHandler.ts index 801e0f1c..3c8fb064 100644 --- a/packages/text-annotator/src/SelectionHandler.ts +++ b/packages/text-annotator/src/SelectionHandler.ts @@ -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(); } }); From d1dddc99b62c6f14e135f86630918771c00fb331 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Thu, 10 Oct 2024 18:25:11 +0300 Subject: [PATCH 2/9] Removed redundant cleaned selection check --- packages/text-annotator/src/SelectionHandler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/text-annotator/src/SelectionHandler.ts b/packages/text-annotator/src/SelectionHandler.ts index 3c8fb064..b61d6dc4 100644 --- a/packages/text-annotator/src/SelectionHandler.ts +++ b/packages/text-annotator/src/SelectionHandler.ts @@ -209,7 +209,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(); } }; From a552780e66ba2f0740321bae6e59f7dcfde0b1bb Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Mon, 14 Oct 2024 18:56:59 +0300 Subject: [PATCH 3/9] Added `isNotAnnotatable` utility --- .../src/utils/splitAnnotatableRanges.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts index d91f45da..e0608a8a 100644 --- a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts +++ b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts @@ -2,11 +2,16 @@ export const NOT_ANNOTATABLE_CLASS = 'not-annotatable'; export const NOT_ANNOTATABLE_SELECTOR = `.${NOT_ANNOTATABLE_CLASS}`; -const isRangeAnnotatable = (range: Range): boolean => { +export const isNotAnnotatable = (node: T): 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 ancestor instanceof HTMLElement - ? !ancestor.closest(NOT_ANNOTATABLE_SELECTOR) - : !ancestor.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); + return !isNotAnnotatable(ancestor); } const iterateNotAnnotatableElements = function*(range: Range): Generator { From a045434b97f2d76e42fedac682d2061feb55b2d2 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Mon, 14 Oct 2024 18:57:41 +0300 Subject: [PATCH 4/9] Added `isNotAnnotatable` usage --- .../text-annotator/src/SelectionHandler.ts | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/text-annotator/src/SelectionHandler.ts b/packages/text-annotator/src/SelectionHandler.ts index b61d6dc4..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; } @@ -163,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` @@ -191,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 = () => { From 68d5e268dd03b0f82f4cc5edc932566317d0e1d6 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Mon, 14 Oct 2024 18:58:13 +0300 Subject: [PATCH 5/9] Removed unused `getAnnotatableFragment` --- .../src/utils/getAnnotatableFragment.ts | 11 ----------- packages/text-annotator/src/utils/index.ts | 1 - 2 files changed, 12 deletions(-) delete mode 100644 packages/text-annotator/src/utils/getAnnotatableFragment.ts 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..c1390635 100644 --- a/packages/text-annotator/src/utils/index.ts +++ b/packages/text-annotator/src/utils/index.ts @@ -2,7 +2,6 @@ export * from './cancelSingleClickEvents'; export * from './device'; export * from './programmaticallyFocusable'; export * from './debounce'; -export * from './getAnnotatableFragment'; export * from './getQuoteContext'; export * from './isWhitespaceOrEmpty'; export * from './isRevived'; From ecd49de53988e1035119a260b7b838cd151cefd0 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Mon, 14 Oct 2024 19:20:06 +0300 Subject: [PATCH 6/9] Simplified the `isNotAnnotatable` arg type --- packages/text-annotator/src/utils/splitAnnotatableRanges.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts index e0608a8a..d66aefd0 100644 --- a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts +++ b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts @@ -2,7 +2,7 @@ export const NOT_ANNOTATABLE_CLASS = 'not-annotatable'; export const NOT_ANNOTATABLE_SELECTOR = `.${NOT_ANNOTATABLE_CLASS}`; -export const isNotAnnotatable = (node: T): boolean => { +export const isNotAnnotatable = (node: Node): boolean => { const closestNotAnnotatable = node instanceof HTMLElement ? node.closest(NOT_ANNOTATABLE_SELECTOR) : node.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR); From ca1b8ec4d8ed82f9832b0ee61fba11b0aa875475 Mon Sep 17 00:00:00 2001 From: Oleksandr Danylchenko Date: Tue, 15 Oct 2024 13:34:33 +0300 Subject: [PATCH 7/9] Simplified popup opening check --- .../TextAnnotatorPopup/TextAnnotatorPopup.tsx | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) 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 +} From 1a3f79375df9099c29e19dae3f4c412ce6b66fd3 Mon Sep 17 00:00:00 2001 From: Rainer Simon Date: Thu, 17 Oct 2024 11:02:26 +0200 Subject: [PATCH 8/9] Refactoring --- .../src/utils/cancelSingleClickEvents.ts | 2 +- packages/text-annotator/src/utils/index.ts | 6 ++++-- .../text-annotator/src/utils/isNotAnnotatable.ts | 15 +++++++++++++++ .../text-annotator/src/utils/reviveSelector.ts | 2 +- .../src/utils/splitAnnotatableRanges.ts | 16 +--------------- 5 files changed, 22 insertions(+), 19 deletions(-) create mode 100644 packages/text-annotator/src/utils/isNotAnnotatable.ts 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/index.ts b/packages/text-annotator/src/utils/index.ts index c1390635..2105761f 100644 --- a/packages/text-annotator/src/utils/index.ts +++ b/packages/text-annotator/src/utils/index.ts @@ -1,10 +1,12 @@ export * from './cancelSingleClickEvents'; +export * from './cloneEvents'; export * from './device'; export * from './programmaticallyFocusable'; export * from './debounce'; export * from './getQuoteContext'; -export * from './isWhitespaceOrEmpty'; +export * from './isNotAnnotatable'; export * from './isRevived'; +export * from './isWhitespaceOrEmpty'; export * from './mergeClientRects'; export * from './rangeToSelector'; export * from './reviveAnnotation'; @@ -12,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 d66aefd0..4269b9e0 100644 --- a/packages/text-annotator/src/utils/splitAnnotatableRanges.ts +++ b/packages/text-annotator/src/utils/splitAnnotatableRanges.ts @@ -1,18 +1,4 @@ -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); -} +import { isRangeAnnotatable, NOT_ANNOTATABLE_CLASS, NOT_ANNOTATABLE_SELECTOR } from './isNotAnnotatable'; const iterateNotAnnotatableElements = function*(range: Range): Generator { const notAnnotatableIterator = document.createNodeIterator( From 69be7ea1b6a8be3dcd4e02e7803179c6a37a4f6d Mon Sep 17 00:00:00 2001 From: Rainer Simon Date: Thu, 17 Oct 2024 11:27:04 +0200 Subject: [PATCH 9/9] Fixed shifted annotation offsets after reformatting --- packages/text-annotator/test/model/w3c/fixtures.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 }, ] }