-
Notifications
You must be signed in to change notification settings - Fork 9
Improved Handling of Clicks Outside the Container #208
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
Conversation
if (isNotAnnotatable(container, evt.target as Node)) { | ||
if (options.dismissOnClickOutside) | ||
selection.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether the dismissOnClickOutside
name is strictly applicable here. With that option, any pointerup
event over a non-annotatable element will cause the selection to be dismissed even when it's not "outside" the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth mentioning that the isLeftClick
isn't strictly correct either 🤔
It only captures whether a user's interaction was started with the "LMB", but doesn't check whether it's a "click":
CleanShot.2025-09-04.at.12.11.03.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that depends on your definition of "outside" :-) I'd argue that this init option should simply cover any area that's outside the annotatable regions – including anything outside the container
, but also anything inside the container that's marked as not-annotatable
(which I'd consider "outside the annotatable area").
I wouldn't want to over-complicate at this point, as long as the behavior makes sense. With this PR, you'd either:
- discard the annotation by clicking anywhere, or
- only discard when clicking an annotatable region (incl. when selecting a different annotation, or creating a new one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
including anything outside the container, but also anything inside the container that's marked as not-annotatable (which I'd consider "outside the annotatable area").
That's an interesting point that's surprisingly sensible. As the "inner" not-annotatable
can be treated as "gaps"/"exclusions". I'd like to add it to the JSDoc for the isNotAnnotatable
method if we end up with such a definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rsimon 🙌🏻
I think I grasped your approach with using only the pointerup
with the isNotAnnotatable
check. However, extending the "not-annotatable" category to anything beyond the container
will introduce breaking changes in my case.
In my previous PR #135 I relied on the fact that the lastPointerDown
will be captured even outside the container
. In clickSelect
, the selection will be cleared because the click doesn't land inside the container
- #135 (comment).
text-annotator-js/packages/text-annotator/src/SelectionHandler.ts
Lines 194 to 199 in 66fc406
const hovered = | |
evt.target instanceof Node && | |
container.contains(evt.target) && | |
store.getAt(evt.clientX - x, evt.clientY - y, selectionMode === 'all', currentFilter); | |
if (hovered) { |
text-annotator-js/packages/text-annotator/src/SelectionHandler.ts
Lines 211 to 212 in 66fc406
} else { | |
selection.clear(); |
Although anything that lands on the not-annotatable
element will be ignored:
if (isNotAnnotatable(evt.target as Node) || !isLeftClick) return; |
Such an approach allowed me to create the SideNotes
component beyond the container
. Any clicks within it shouldn't clear the selection. Exactly like in the case of the "toolbar" @rsimon mentioned - #133 (comment).
However, any interactions beyond the container
or the SideNotes
/"toolbar" should cause the selection to clear.
Using just the dismissOnClick
won't be sufficient. That won't allow users to interact with the "toolbar" without unexpectedly dismissing the selection. But not using the dismissOnClick
won't allow them to dismiss the selection when an irrelevant piece of UI is clicked.
I don't understand. Can you summarize?
I guess there are two ways to think about this:
There's definitely pros and cons for both. But I wonder if there's really a clear winner in terms of what's more effort (both inside of Recogito and the host app.) |
Maybe it would be more sensible to pass the |
Yes, allowing a function could be an option! (At the same time, you could pretty much attach that function to your document as a listener, and then clear the selection programmatically accordingly?) |
Yep, all those points are correct ✅ If the |
Yeah, I believe it's a possible option as well. But it makes the consumer come up with custom code wrappers that most likely will conflict with the selection flow/timing. Especially if we decide to modify it in the future. So I'd suggest centralizing the selection dismissal handling on the package's side with the |
Exactly. Therefore, if the application needs mixed behavior (some areas outside should discard and some should not) it will always have to handle one case or the other itself. If you start with the default behavior (no dismissal) and then do a programmatic dismissal on those areas that you want to dismiss, there should be no interference anywhere. I can see, though, how you'd sometime want to mark one or two specific "non-dismiss" areas, rather than the other way round. That might start to get messy with CSS classes though, since we'd now have |
How about the following:
I guess apps that want to use a "positive" way to mark areas could append something like a |
I like that idea! It both provides a sensible set of defaults and allows fine-tuning for the consumers using the function. I'd go with it ✅. |
Ok, I'll put that on the todo list. But I guess this PR is safe to merge, since it doesn't actually change the default behavior, right? (Auto-discarding was and will only be happening when clicking inside the annotatable areas.) |
Well, we can call it "discardOnPointerUp" :-) |
Yeah, I think so ✔️ |
Sorry, I'm only catching up with the conversation. I think it looks good, the default behaviour and the customisable one ! I'm going to test this very soon (next week probably), thanks for the work ! |
This feature seems to be now working fine on my app. |
In this PR
This PR makes changes to the behavior for handling clicks outside the annotatable area (#133).
container
is now automatically considerednot-annotatable
- no need to set the CSS class.dismissOnClick
- setting this arg totrue
will change the behavior and discard selections if the user clicks outside the annotatable areaNote: this PR also includes a small performance improvement:
redraw()
calls from the mutation observer are now debounced. (Mostly because React seems to fire lots of small incremental mutations.)@oleksandr-danylchenko and @SovanramyVar4D: I think this should do the trick and (hopefully...) not introduce any unwanted side effects. I'd appreciate it if you could take a quick look and perhaps test in your environments!