search

recogito / text-annotator-js

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

Fixed overeager popup closure on touch devices #158

Closed oleksandr-danylchenko closed 1 month ago

oleksandr-danylchenko commented 1 month ago

Issue

When a user scrolls upon the popup opening - the latter gets closed instead of being scrolled along! The issue is the outsidePressEvent that's pointerdown by default. Therefore, it would immediately dismiss the popup when a user touches their screen. Instead, we can use a bit more touch-friendly mousedown: 

useDismiss(context, {
  // Eager on both mouse + touch input.
  outsidePressEvent: 'pointerdown',
  // Eager on mouse input; lazy on touch input.
  outsidePressEvent: 'mousedown',
  // Lazy on both mouse + touch input.
  outsidePressEvent: 'click',
});

So when a user touches a screen and drags the popup won't be dismissed, only when the user touches and releases almost immediately.

oleksandr-danylchenko commented 1 month ago

We shouldn't close the popup on the pointer interactions at all! That should be controlled by the SelectionHandler! Otherwise, we risk introducing a conflicting race condition between the handler's "annotation click" and the previous annotation popup closure. So clicking from annotation to annotation would have a jittering effect

rsimon commented 1 month ago

I was thinking about this, and had my doubts as well. I think the ESC key behavior was useful. But agree it would be cleaner to handle this on the SelectionHandler level.

oleksandr-danylchenko commented 1 month ago

The only thing that can be improved a bit in the SelectionHandler is the support for scrolling on the touch devices. Currently, when the popup is opened and you scroll a bit, that gets considered as a click, and the popup is dismissed. However, the native context menu is still present. So we may wanna narrow the "click" check down a bit to compare the down/up coordinates.

oleksandr-danylchenko commented 3 weeks ago

when the popup is opened and you scroll a bit, that gets considered as a click, and the popup is dismissed. However, the native context menu is still present

The issue is resolved by moving the setOpen under the escape-key & focus-out reasons. Then, the outside-click won't accidentally dismiss the popup, but put the responsibility onto the SelectionHandler "click" processing. https://github.com/recogito/text-annotator-js/blob/69be7ea1b6a8be3dcd4e02e7803179c6a37a4f6d/packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx#L52-L57

So we may wanna narrow the "click" check down a bit to compare the down/up coordinates.

We don't need to do that, because the pointerup is not even dispatched when a user scrolls on the touch device even slightly! Only the pointerdown. Therefore, the "click" won't even be processed! image