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

Activating annotation popup on demand #129

Open
AnthonyKamers opened this issue Aug 7, 2024 · 20 comments
Open

Activating annotation popup on demand #129

AnthonyKamers opened this issue Aug 7, 2024 · 20 comments

Comments

@AnthonyKamers
Copy link

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
Copy link
Member

rsimon commented Aug 7, 2024

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

AnthonyKamers commented Aug 7, 2024

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
Copy link
Member

rsimon commented Aug 7, 2024

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

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
Copy link
Member

rsimon commented Aug 10, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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

  • User starts text selection...
  • User completes text selection by lifting the finger -> annotation gets created & selected -> popup opens
  • On mobile, selection remains editable (regardless of whether the popup has focus)

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

@rsimon
Copy link
Member

rsimon commented Sep 30, 2024

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

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
Copy link
Member

rsimon commented Sep 30, 2024

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

oleksandr-danylchenko commented Sep 30, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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

@rsimon
Copy link
Member

rsimon commented Sep 30, 2024

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

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

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
Copy link
Member

rsimon commented Sep 30, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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.

  • Desktop: seems to work, both with mouse and keyboard selection. (When selecting via keyboard: if you release keys, and then press SHIFT again & continue the selection, the annotation will update, and the popup will move along.)
  • Touch, iOS: works fine. You select until the selection is complete -> popup opens. Selection handlebars remain active, if you drag them, the popup moves along.
  • Touch, Android: the most stubborn platform. I got most things working now (with some USB on-device debugging). The selection is pretty much "complete" as soon as you start selecting, because the contextmenu event fires instantly. But like on iOS, you can drag the handles, and the popup will move along. What I did not solve yet: the default system context menu gets in the way & is positioned in front of the popup. It seems possible to disable this on Chrome. But on Firefox, evt.preventDefault seems to prevent selection altogether. I also tried simply positioning the popup below the annotation, so that it can co-exist with the system context menu. But that gets in the way if the user tries to select text downwards.

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

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

@oleksandr-danylchenko
Copy link
Contributor

oleksandr-danylchenko commented Sep 30, 2024

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
Copy link
Member

rsimon commented Sep 30, 2024

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.)

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

3 participants