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

#133 Fixed missing annotation dismissal clicking outside of the annotator container #135

Conversation

oleksandr-danylchenko
Copy link
Contributor

Issue - #133

Changes Made

To capture the actual lastPointerDown event on the page and make the correct timeDifference calculations, I added pointerdown event to the document scope.

In that way, the "click" will be properly recognized in the pointerup handler and the selection will be dismissed. As there will be no annotation under the pointer.

Comment on lines +139 to +140
evt.target instanceof Node &&
container.contains(evt.target) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the container.contains as a small optimization that will immediately dismiss clicks outside of the container. So we won't need to execute the relatively heavier getAt method

@rsimon
Copy link
Member

rsimon commented Aug 30, 2024

Thanks - another nice & useful one!

@rsimon rsimon merged commit dbb6e3c into recogito:main Aug 30, 2024
@oleksandr-danylchenko oleksandr-danylchenko deleted the #133-fix-not-dismissed-annotation-on-outside-click branch August 30, 2024 09:24
@rsimon
Copy link
Member

rsimon commented Sep 2, 2024

I had to revert this change for now, unfortunately.

We have a 'Delete' button for the current annotation (outside the annotatable area). It is only shown when the annotation is selected. Because the selection got discarded before the click event, clicking the button had no effect. (The button was unmounted before the click event.)

It looks like I can get around this by using the onPointerDown event on the button. But that doesn't seem very clean. I'll think about it a bit on our end. Either way: the problem is that pointer event order seems to be pointerdown, pointerup, click which means clicked elements no longer have access to the selection.

@oleksandr-danylchenko
Copy link
Contributor Author

oleksandr-danylchenko commented Sep 2, 2024

We have a 'Delete' button for the current annotation (outside the annotatable area). It is only shown when the annotation is selected. Because the selection got discarded before the click event, clicking the button had no effect. (The button was unmounted before the click event.)

Right... Until the click happens, the selection gets dismissed...
I believe that it might be mitigated by explicitly checking what element is the target of the pointerdown event and preventing selection dismissal if a specific classname/attribute is present. It can be something like the data-annotation-controls.
From the consumer perspective, it would act like the not-annotatable class, that they should apply manually based on their specific needs. The library will just provide a convenient default behavior that will fit "most" of the cases.

@oleksandr-danylchenko
Copy link
Contributor Author

Unfortunately, moving the pointerdown listener directly to the document has just bitten me too! If I capture it on an input or a button and quickly drag to the textual content - the selection will be dismissed!

Screen.Recording.2024-09-03.at.20.08.28.mov

That happens because starting the selection within the interactive element - traps the selection range within it. Therefore, on the pointerup event the selection will always be collapsed. And as we capture the lastPointerDown even on not-annotatable elements - the timeDifference will be pretty small!

@oleksandr-danylchenko
Copy link
Contributor Author

oleksandr-danylchenko commented Sep 3, 2024

However, I think I have an idea of how to:

  1. resolve Annotation isn't dismissed clicking outside of the annotator container ⚠️ #133, dismiss selection when clicking outside of the container
  2. don't break the "Delete" button functionality you mentioned
  3. resolve the unexpected dismissal when the mouse is being dragged from the interactive element. See - Selection unexpectedly dismisses after dragging mouse from the input/textarea to the browser's toolbar #147 (comment)

It's capturing the lastPointerEvent only on the elements that are not contained within the not-annotatable tree!
That would be helpful for the aforementioned issues because:

  1. When a user clicks somewhere outside the container - such a target usually won't be located within the not-annotatable parent. Therefore, the pointerdown event will be preserved and will create a proper timeDifference in the pointerup handler to dismiss the selection!
  2. If you explicitly apply the not-annotatable to the Delete button - the pointerdown will not get captured on it. So the timeDifference in the pointerup handler would be sufficient to not treat the pointerup as the "click".
  3. Essentially the same as 2., but we already have the not-annotatable on the desired components like the popup or the side notes (in the consuming app)

Does it sound right to you, @rsimon?

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

Successfully merging this pull request may close these issues.

2 participants