recogito / text-annotator-js

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

Activating annotation popup on demand #129

Open AnthonyKamers opened 3 months ago

AnthonyKamers commented 3 months ago

Some applications must show the annotation popup on demand, i.e., when a set of behaviors is triggered. The SelectionHandler already knows whether an annotation was clicked, so it just needs to export this possibility for TextAnnotator.

From this point on, we can provide a robust set of options for the TextAnnotatorPopup, which can be opened/closed on demand.

rsimon commented 3 months ago

You can already use the useSelection hook (in the React version) or the setSelected API method (in the vanilla JS version) for this. Or am I misunderstanding?

AnthonyKamers commented 3 months ago

I don't understand how I can do this using the structure provided. TextAnnotatorPopup always open when an annotation is selected/clicked. I want to open the popup via a prop or some other hook. The useSelection, as far as I understood, is used to provide the annotation selected at the moment, am I right?

rsimon commented 3 months ago

Sorry, yes that's right. You'd need to go through the vanilla JS API, either the anno.setSelected method, or the anno.state.selection object.

The Popup, by design, is supposed to show up for the selected annotation (programmatically or not). If your use case is different, you'd need to roll your own Popup component for now.

AnthonyKamers commented 3 months ago

Oh sure! I thought using the TextAnnotatorPopup as an official wrapper would make things easier. But I can make my own TextAnnotatorPopup with that functionality.

My original idea was to provide a way to communicate between TextAnnotatorPopup and TestPopup. I have made a sample for that, is it possible for you to take a look at it? My code is at https://github.com/AnthonyKamers/text-annotator-js/tree/129-popup-on-demand.

If you think it would be better for the dev using the lib to make their own Popup with that behavior, that's fine. It's my first time using a open-source project like that, so if you have any thoughts on how to interact to it properly, please let me know. Thank's!

rsimon commented 2 months ago

I couldn't find TestPopup (but I'm semi-offline right now - looked only briefly from my phone). But I'd like to understand your use case better. Is there a reason you specifically don't want to use the selection to determine where a popup should be open?

rsimon commented 1 month ago

I'm just revisiting this & realize there's (probably) a different issue involved - one which I'm also running into right now. The popup only opens if there is an actual user action involved along with the selection. (Specifically, there needs to be a pointerup event.)

This means programmatic selection does currently not open the popup. I'm going to redesign this, but I'm not quite sure how yet. (CCing @oleksandr-danylchenko.)

One part of the solution is probably that we should (as discussed previously) create annotations only after the selection is complete, not as soon as the selection starts. Why does this relate to this issue? Annotations are selected as soon as they are created. The fact the popup waits for the pointerup event is that it doesn't interfere with the text selection process. (If the any element in the popup takes focus, the selection will be stopped.)

Therefore, if we change the behavior, so that the annotations get created (and selected!) only when the user is done with the text selection, there might be no more need for the pointerup requirement?

rsimon commented 1 month ago

P.S.: I think mobile interaction would still work just fine.

I wonder if there are anything potential gotchas I'm missing related to keyboard-based selection, @oleksandr-danylchenko?

rsimon commented 1 month ago

In a separate branch, I have now made a (surprisingly) tiny tweak to the selection handler to trigger the createAnnotation event on pointer up. This appears to work well on desktop (Chrome, FF / Mac) and mobile (Chrome, FF / iOS, Android).

I don't have a solution for keyboard selection yet. Because there's no pointerup event, there's no defined moment where we can create the annotation. I wonder if keyboard selection should just listen to keyup on the Shift key?

oleksandr-danylchenko commented 1 month ago

This appears to work well on desktop (Chrome, FF / Mac) and mobile (Chrome, FF / iOS, Android).

That's interesting... Because I faced an issue with Android, as it was never dispatching the pointerup event... Therefore, the "User completes text selection by lifting the finger" scenario couldn't be checked there

rsimon commented 1 month ago

Hm, yes indeed - I did have to "tap out" of the selection to make the appear on Chrome/Android (thus creating another pointerup event.) Interestingly I had done that automatically... Behavior is different on FF/Android, where the pointerup event seems to be fired along with completing the initial selection.

Hm, that does suggest that the original model (create annotation immediately + update) may be more sustainable. Still leads back to the question of how to make the popup work without requiring a user event....

oleksandr-danylchenko commented 1 month ago

Still leads back to the question of how to make the popup work without requiring a user event....

Maybe it's worth providing an additional Depression argument to the TextAnnotatorPopup that will decide whether the annotation should get opened. It may look smth like this:

type TextPopupOpenExpression = boolean | ((selection: ReturnType<typeof useSelection>) => boolean);

interface TextAnnotationPopupProps {
  ...
  open?: TextPopupOpenExpression;
}
const selectedKey = selected.map(a => a.annotation.id).join('-');
useEffect(() => {
  const { open } = props;

  if (open !== undefined) {
    setOpen(typeof open === 'function' ? open(selection) : open);
  } else {
    if (selected.length > 0 && event) {
      setOpen(event.type === 'pointerup' || event.type === 'keydown');
    }
  }
}, [selectedKey, event]);
rsimon commented 1 month ago

I'm moving our conversation here, rather than burying it in the commit comment.

So here's something I just learned: on Chrome Android, the thing that happens when the browser considers the text selection "complete" is that the context menu bubble pops up; and we're all supposed to listen to the contextmenu event rather than pointerup or touchend.

About the general behavior: the models we've been discussing are:

  1. Create annotation immediately, update on selectionchange (= the current model)
  2. Wait until the selection is 'complete' (however we determine "complete")

The first one has some advantages, like moving the popup along with the annotation when the user changes selection (for your case @oleksandr-danylchenko), or broadcasting state change updates in our own multi-user system. (This way, users can see others make selections in realtime. Eye candy, really. But a nice touch.)

On the other hand, there are also disadvantages. In particular frequent executions of mergeClientRects which is a potential performance bottleneck.

That being said: which model we implement in the end may actually be less relevant. After all, the real question is: at what point should the popup appear? Right now the popup follows the createAnnotation event. But could have its own logic, as you say. However, TBH, I'm still slightly leaning to keep the logic together it in the SelectionHandler for now, to avoid too much duplication when it comes to keeping track of the various (device-dependent) events.

rsimon commented 1 month ago

CTRL+A really is the only oddball here... it does trigger selection change, but is otherwise not easily traceable, as you said.

rsimon commented 1 month ago

Given that CTRL+A is the "edge case" here, maybe it's simply worth treating it separately? Instead of tracing a sequence of events (start, select, end), it's really a one-shot operation which we could handle as such.

Seems that this could be sufficient?

const onSelectAll = (evt: KeyboardEvent) => {
    onSelectStart(evt);

    // Proper lifecycle management: clear selection first...
    selection.clear();

    // ...then add annotation to store
    store.addAnnotation({
        id: currentTarget.annotation,
        bodies: [],
        target: currentTarget
    });
}

hotkeys('⌘+a', { element: container, keydown: true, keyup: false}, evt => {
   onSelectAll(evt);
})
oleksandr-danylchenko commented 1 month ago

That's an interesting approach, I would give it a try! As it'll help capture the changing selection that follows upon the Ctrl + A keydown event!

oleksandr-danylchenko commented 1 month ago

And one more small thing I realized now - we should conditionally listen to the⌘ + A on Mac/iOS and the Ctrl + A for the rest of the devices. Otherwise, users would be to use both of them on Mac, which is unexpected

rsimon commented 1 month ago

And one more small thing I realized now - we should conditionally listen to the⌘ + A on Mac/iOS and the Ctrl + A for the rest of the devices. Otherwise, users would be to use both of them on Mac, which is unexpected

Yes, I was thinking the same. In my branch, I'm now listening to both. But we should branch based on platform, as you say like described here.

rsimon commented 1 month ago

BTW: here's a small progress report: the new branch seems to be working well. As written previously: I'm now waiting for the selection to be "complete" before creating the annotation (for whatever that means on different platforms), avoiding the need to do more complex state tracing in the popup. The currentTarget remains "open", and selectionchange events will cause the annotation to update.

As will not surprise you: the way different platforms and interaction modes behave is... peculiar.

I tested all of this with Chrome and FF, BTW.

Anyways: it's progress. I'll keep working on it.

oleksandr-danylchenko commented 1 month ago

I also tried simply positioning the popup below the annotation, so that it can co-exist with the system context menu.

We decided to go with this strategy too: image And we just "deal" with a slight inconvenience of the downward selection

rsimon commented 1 month ago

Well, then that's probably the solution. (Also, it's probably good practice to keep default browser behavior untouched as much as possible. Blocking the default context popup is probably an anti-pattern, anyway.)