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

Annotation isn't dismissed clicking an empty space, covered by the native selection range ⚠️ #136

Open
oleksandr-danylchenko opened this issue Aug 14, 2024 · 3 comments

Comments

@oleksandr-danylchenko
Copy link
Contributor

Background

While handling the pointerup event, checking the document.getSelection().isCollapsed property can behave in 2 ways:

  1. You made a selection and clicked on it. Then, the isCollapsed is false even though the native highlight disappears! ⚠️
  2. You made a selection and clicked somewhere else. Then, the isCollapsed is expectedly true, matching the native highlight disappearance.
Screen.Recording.2024-08-14.at.16.16.35.mov

Issue

In the SelectionHander the annotation selection gets dismissed only when the pointerdown + pointerup combination is recognized as the "click" and there's nothing "hovered" (lines 152-155 & 144-146):

const timeDifference = evt.timeStamp - lastPointerDown.timeStamp;
// Just a click, not a selection
if (document.getSelection().isCollapsed && timeDifference < 300) {
currentTarget = undefined;
clickSelect();
} else if (currentTarget) {
selection.userSelect(currentTarget.annotation, evt);
}

// Logic for selecting an existing annotation by clicking it
const clickSelect = () => {
const { x, y } = container.getBoundingClientRect();
const hovered = store.getAt(evt.clientX - x, evt.clientY - y, currentFilter);
if (hovered) {
const { selected } = selection;
if (selected.length !== 1 || selected[0].id !== hovered.id)
selection.userSelect(hovered.id, evt);
} else if (!selection.isEmpty()) {
selection.clear();
}
}

However, as the document.getSelection().isCollapsed is false when you click over the native selection range, the annotation selection sometimes doesn't get dismissed!
Instead, the annotation gets selected another time, because the currentTarget is still present!

Alternatively, we expect that the annotation selection will behave like the native selection highlight in all cases. When you click on an empty space - it should be dismissed.

Issue Examples

Screen.Recording.2024-08-14.at.16.37.28.mov
annotation_dismissed.mp4

Suggested Changes

The root of the issue is the document.getSelection().isCollapsed that is "lagging behind" on the pointerup event handling.

After I added a 0ms. timeout for the reading, I started getting more adequate value consistently. It's always true either when you click on the selected range or some other content.

Screen.Recording.2024-08-14.at.16.47.39.mov

In that way, the "click" will always get processed consistently and the annotation selection will get dismissed when you didn't click on the highlight explicitly.

oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 14, 2024
…nge-click' into staging

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 16, 2024
…nge-click' into keyboard-event-selection

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 30, 2024
…n-native-range-click

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Aug 30, 2024
…nge-click' into keyboard-event-selection

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 2, 2024
…n-native-range-click

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 2, 2024
…nge-click' into keyboard-event-selection

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Sep 2, 2024
…nge-click' into staging

# Conflicts:
#	packages/text-annotator/src/SelectionHandler.ts
rsimon added a commit that referenced this issue Sep 2, 2024
…sssed-annotation-on-native-range-click

#136 Fixed missing annotation dismissal clicking over native selection range
@rsimon
Copy link
Member

rsimon commented Sep 30, 2024

Just adding that this should now also work in the revised-selection-behavior branch.

@oleksandr-danylchenko
Copy link
Contributor Author

oleksandr-danylchenko commented Oct 15, 2024

Hello @rsimon 👋🏻
Unfortunately, I noticed that the "lagging behind isCollapsed property" is still a thing for touch devices! When I tap somewhere around the selection - the isCollapsed is still true when the onPointerUp gets handled, even within the setTimeout.
image

That creates a weird effect that makes a user tap twice to dismiss the selection. Similar to the one we had in the demos above ;(

@oleksandr-danylchenko
Copy link
Contributor Author

Incredibly... increasing the setTimeout's timeout from the default 0 to 1 helps! It starts reporting the isCollapsed properly again!

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

2 participants