recogito / text-annotator-js

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

Annotation isn't dismissed clicking outside of the annotator `container` ⚠️ #133

Open oleksandr-danylchenko opened 3 months ago

oleksandr-danylchenko commented 3 months ago

Issue

When you have a selection within the annotatable container and click outside - the selected annotation won't be dismissed. Instead, the userSelect method is called on it:

https://github.com/user-attachments/assets/8d9bc14c-f388-486d-92bc-a22e53f6eb5e

That's because the pointerDown method is initialized only on the container, but the pointerUp is initialized on the whole document. Therefore, the currentTarget is still present when the latter is executed, and the evt.timeStamp - lastPointerDown.timeStamp is always too large: https://github.com/recogito/text-annotator-js/blob/e6d3f33339ab1f4cd2ccb6b77624c20f12841ee4/packages/text-annotator/src/SelectionHandler.ts#L151-L157

rsimon commented 1 month ago

I've also been back and forth with this. I think we have different UI requirements here, and I can see valid use cases for both. (Which may mean we might need to make this a config option.)

In our case we have a whole bunch of UI components around the annotatable area. Things like setting annotation filters, changing color coding etc. Most of the time, users would expect that nothing should change about the selection when interacting with those parts of the UI. I.e. in this case, clicks outside the annotatable area should simply be ignored completely.

I guess we could use a CSS selector like not-annotatable to mark such areas. (Or maybe not-annotatable is actually enough?) But I wonder if this wouldn't lead to some kind of error prone patchwork where you start sprinkling (and forgetting to sprinkle) these CSS classes all over the place?

oleksandr-danylchenko commented 1 month ago

Or maybe not-annotatable is actually enough?)

Applying the not-annotatable class should be enough. It can be used just for the parent container of the controls, so we'll exclude all of them at once.

But I wonder if this wouldn't lead to some error-prone patchwork where you start sprinkling (and forgetting to sprinkle) these CSS classes all over the place?

I also thought about this, but I concluded that we'll always have a fairly limited set of known controls that we can always cover with the not-annotatable containers. Therefore, it seemed like a good enough compromise to me. Also, I don't think that users should expect to click beyond those controls and expect that the selection still will be retained 🤔.

oleksandr-danylchenko commented 1 month ago

The only issue that differs between your use-case and ours - the positioning of the controls. We have them all located within the annotator container in the form of the popup and side notes. That way - marking controls with not-annotatable makes semantical sense, because we exclude those elements from the annotatable element. But marking an element as not-annotatable outside of the container... 🤔. It doesn't seem to be immediately clear that the clicks will be ignored on such an element... It begs for some other form of inclusion of such elements in the annotator workflow and then ignoring clicks on them.