Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
oleksandr-danylchenko opened this issue Aug 13, 2024 · 3 comments

Comments

@oleksandr-danylchenko
Copy link
Contributor

oleksandr-danylchenko commented Aug 13, 2024

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:

Screen.Recording.2024-08-13.at.16.21.06.mov

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:

// Just a click, not a selection
if (document.getSelection().isCollapsed && timeDifference < 300) {
currentTarget = undefined;
clickSelect();
} else if (currentTarget) {
selection.userSelect(currentTarget.annotation, evt);
}

@oleksandr-danylchenko oleksandr-danylchenko changed the title Selected annotation isn't getting dismissed upon clicking outside of the annotatable container ⚠️ Annotation isn't dismissed clicking outside of the annotator container ⚠️ Aug 13, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 13, 2024
…ick' into staging

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
rsimon added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 30, 2024
rsimon added a commit that referenced this issue Aug 30, 2024
…ssed-annotation-on-outside-click

#133 Fixed missing annotation dismissal clicking outside of the annotator `container`
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 4, 2024
…ick' into staging

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 10, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 10, 2024
…ick' into keyboard-event-selection

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 10, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 26, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 26, 2024
@rsimon
Copy link
Member

rsimon commented Sep 30, 2024

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
Copy link
Contributor Author

oleksandr-danylchenko commented Sep 30, 2024

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
Copy link
Contributor Author

oleksandr-danylchenko commented Sep 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants