Skip to content

Commit

Permalink
Keyboard selection
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsimon
rsimon committed Sep 30, 2024
1 parent b96f04a commit c14ed99
Showing 1 changed file with 19 additions and 0 deletions.
19 changes: 19 additions & 0 deletions packages/text-annotator/src/SelectionHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,23 @@ export const SelectionHandler = (
});
}

const onKeyup = (evt: KeyboardEvent) => {

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024 •
edited
Loading

Contributor

Warning from stepping on the same rake as me ⚠
In the #118 I intentionally didn't go with creation annotation upon lifting the key. It makes it complicated and error-prone to decide:

  • whether annotation should be created or updated
  • when to open the popup
  • how to handle the Ctrl + A lifting event
    Creating the annotation immediately and then gradually updating it on the selectionchange turned out to be more convenient to maintain.

This comment has been minimized.

Sign in to view

Copy link
@rsimon

rsimon Sep 30, 2024

Author Member

I was just thinking about Ctrl+A lifting event. But was wondering if that should just be another keyup case I should handle explicitly. I definitely don't want to tie the popup to a pointer- or key event anymore, since that makes programmatic opening of the popup impossible (and it's a frequent use case.)

Alternatively, we could also stick to the "create annotation immediately" pattern for keyboard selection only. Although that would possibly be just as complicated?

This comment has been minimized.

Sign in to view

Copy link
@rsimon

rsimon Sep 30, 2024

Author Member

FWIW: how about checking if the annotation exists already in the store on Shift keyup and then updating the target if so? Seems to work in principle?

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024 •
edited
Loading

Contributor

But was wondering if that should just be another keyup case I should handle explicitly

It can be... I even had a commit where I added "selection" completion listening on the keyup events - source. However, it's pretty bulky. As browsers cannot capture system-related hotkeys only on keydown.

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024 •
edited
Loading

Contributor

how about checking if the annotation exists already in the store on Shift keyup and then updating the target if so? Seems to work in principle?

I believe that we should do nothing upon releasing the Shift the 2nd+ time. Because the annotation should have been already updated in the selectionchange handler

This comment has been minimized.

Sign in to view

Copy link
@rsimon

rsimon Sep 30, 2024

Author Member

The problem is: I no longer want to create the annotation immediately, after the first character is selected. It causes a whole bunch of problems with the popup. Therefore, I no longer want to update the store in the selectionchange handler.

I really don't see a good way to stick to the current selection model (=create annotation immediately and then update on every selectionchange) without making it impossible (or at least very difficult) to open the popup programmatically.

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024 •
edited
Loading

Contributor

Yeah, I got you now... That's really tough. Because if we would like to create/update the annotation only on the "selection pauses" - then we'll need to hide the popup each time the user continues the selection. Because its position won't correspond to the selection range in real-time anymore. I believe that was the main reason why I moved from processing the keyup event during the keyboard selection.

This comment has been minimized.

Sign in to view

Copy link
@rsimon

rsimon Sep 30, 2024

Author Member

I wouldn't worry about hiding the popup. Once it's open, it can stay so. (It could just move along as the user changes the selection.) What I'm struggling with is that our popups take focus. Thus, if we show them right after the annotation gets created, the instantly stop the selection.

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024

Contributor

Maybe we should have a separate flag for the cases when the selection pauses. That would help to decide when to show the popup w/o leading to mispositioning. Because just the selection doesn't seem to be enough. As we don't want to discard it when user continues the selection

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024

Contributor

It could just move along as the user changes the selection.

Hmm... But to do that we should automatically update it in the selectionchanged handler. Will we activate the change handling upon the initial annotation creation?

This comment has been minimized.

Sign in to view

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Sep 30, 2024

Contributor

What I'm struggling with is that our popups take focus. Thus, if we show them right after the annotation gets created, the instantly stop the selection.

Aha, then I agree that the creation of the annotation should take place only on the "selection pause" 👌

Context: I didn't suffer with that as popups in my app never receive the focus automatically, so we were good even with auto-creation.
if (evt.key === 'Shift' && currentTarget) {
// Proper lifecycle management: clear selection first...
selection.clear();

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

// ...then make the new annotation the current selection
selection.userSelect(currentTarget.annotation, cloneKeyboardEvent(evt));
}
}

hotkeys(SELECTION_KEYS.join(','), { element: container, keydown: true, keyup: false }, (evt) => {
if (!evt.repeat)
lastDownEvent = cloneKeyboardEvent(evt);
Expand Down Expand Up @@ -267,6 +284,7 @@ export const SelectionHandler = (
document.addEventListener('pointerup', onPointerUp);

if (annotatingEnabled) {
container.addEventListener('keyup', onKeyup);
container.addEventListener('selectstart', onSelectStart);
document.addEventListener('selectionchange', onSelectionChange);
}
Expand All @@ -275,6 +293,7 @@ export const SelectionHandler = (
container.removeEventListener('pointerdown', onPointerDown);
document.removeEventListener('pointerup', onPointerUp);

container.removeEventListener('keyup', onKeyup);
container.removeEventListener('selectstart', onSelectStart);
document.removeEventListener('selectionchange', onSelectionChange);

Expand Down

0 comments on commit c14ed99

Please sign in to comment.