recogito / text-annotator-js

A JavaScript library for text annotation.
BSD 3-Clause "New" or "Revised" License
21 stars 7 forks source link

⚠️ Fixed checking `null` selection `anchorNode` for being `not-annotatable` ⚠️ #164

Closed oleksandr-danylchenko closed 2 days ago

oleksandr-danylchenko commented 1 month ago

Issue

On iOS when the user clicks a button it emits the selectionchange event! While listening to the selectionchange - we check whether its achorNode is even annotatable. https://github.com/recogito/text-annotator-js/blob/c5ffd362e977ff788991609cef40b344e34ea7f6/packages/text-annotator/src/SelectionHandler.ts#L77-L86

However, I discovered that sometimes when a button is clicked - it produces a collapsed selection where the anchorNode is null: image Then, null gets passed to the isNotAnnotatable which expectedly fails: image

Changes Made

Added the anchorNode presence check before running the isNotAnnotatable on the selection range.

Also, we cannot simply check whether the range is collapsed, because it would produce false-negative checks for the not-annotatable presence: image

oleksandr-danylchenko commented 1 month ago

And I found another related bug on iOS! When the range doesn't have any anchorNode assigned - such a selection will pass the not annotatable check! Therefore, the selectionchange handler will think that the selection was collapsed and it should delete the newly created annotation! image

Therefore, we should treat ranges with a missing anchorNode as not annotatable by default.

rsimon commented 1 month ago

If I understand correctly, shouldn't we simply pack the if (!sel?.anchorNode) check into the isNotAnnotatable util function?

export const isNotAnnotatable = (node: Node): boolean => {
  const closestNotAnnotatable = node instanceof HTMLElement
    ? node.closest(NOT_ANNOTATABLE_SELECTOR)
    // add node?.parentElement... here?
    : node.parentElement?.closest(NOT_ANNOTATABLE_SELECTOR);
  return Boolean(closestNotAnnotatable);
}
oleksandr-danylchenko commented 1 month ago

I think that we shouldn't because the missing anchorNode doesn't give us any info about where such a range was placed. So, semantically, we're not able to check whether it's annotatable 🤔 Also, when the range is not annotatable, the currentTarget gets undefined: https://github.com/recogito/text-annotator-js/blob/69be7ea1b6a8be3dcd4e02e7803179c6a37a4f6d/packages/text-annotator/src/SelectionHandler.ts#L80-L86 However, it shouldn't happen when an iOS user clicks on some button within the popup. That's why I added an early bail out when the anchorNode is missing.