-
Notifications
You must be signed in to change notification settings - Fork 7
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
Revised Selection Behavior #151
Conversation
83b53fa
to
c8000a1
Compare
I'm about to merge this into I so far tested this on FF + Chrome (Mac, iPad, Android) and will do another round of testing on FF/Chrome/Edge on Linux tomorrow, with a hybrid desktop/touch computer. |
Thank you 🙌🏻🙌🏻🙌🏻 |
Update: I added a few more tweaks. Bad news first: I could not find a perfect solution for CTRL/CMD-A, unfortunately. (There's a timeout involved, which might presumably lead to timing issues if large texts are involved. Not sure if CTRL-A on large texts is such a big use case though?) Other than that, however, everything seems to work as it should! I tested with the standard test page from
* The hybrid Ubuntu notebook had some issues when used with touch. But I think none of these are dealbreakers, and tolerable:
|
packages/text-annotator-react/src/TextAnnotatorPopup/isMobile.ts
Outdated
Show resolved
Hide resolved
packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx
Show resolved
Hide resolved
packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx
Show resolved
Hide resolved
Co-authored-by: Oleksandr Danylchenko <68850090+oleksandr-danylchenko@users.noreply.github.com>
Co-authored-by: Oleksandr Danylchenko <68850090+oleksandr-danylchenko@users.noreply.github.com>
packages/text-annotator-react/src/TextAnnotatorPopup/TextAnnotatorPopup.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look great and clear. I tested them locally and they seem to work nicely. Thank you 🔥
The only feasible concern I have is losing the focus-out speedbump for the popup.
} else { | ||
// Proper lifecycle management: clear selection first... | ||
selection.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we may need to restore the selection.clear()
call if there's nothing to update. Because now the previous, "discarded selection" unexpectedly stays up until you create a brand-new one.
unselect.mp4
*in my testing env, the annotations that don't have either a highlight or a note assigned get automatically garbage collected when they get unselected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPD: Submitted a PR - #155
} else if (currentTarget && currentTarget.selector.length > 0) { | ||
selection.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also... I'm not entirely sure why the current selection gets cleared and then immediately reselected back 🤔. I believe that discarding the previous selection in the selectionchanged
handler should be enough, isn't it?
const onPointerDown = (evt: PointerEvent) => { | ||
if (isContextMenuOpen) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, @rsimon 👋🏻
Could you please clarify why the context menu guard was added for the pointer
event handlers? With it in place, it takes 2 taps to dismiss the annotation selection. The first tap is needed to dismiss the context menu and the second one is to dismiss the annotation itself.
That makes the annotation selection look like it's lagging behind 🤔
Record_2024-10-29-17-40-29.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that it was added under the Minor fix
commit fb3d70b. However, it doesn't carry enough context 😅
In this PR
This PR revises the selection behavior fundamentally. Instead of creating an annotation immediately, the SelectionHandler now waits until the (native browser text-)selection is 'complete'. Because there is not actually a 'complete' event, there are different approaches, depending on platform and interaction mode:
pointerup
event.keyup
event of the Shift key. (Note that selection remains editable - unless popup intervenes.)Ctrl
+A
(orCmd
+A
) is handled separately, as a single operation.pointerup
orcontextmenu
event.contextmenu
event.Note: in all cases, the selection remains editable after "completion", either by using Shift+Arrow keys (desktop) or by dragging the handlebars (on mobile/touch).
React Popup
I also modified the React
TextAnnotatorPopup
with the following behaviors:Warning: mobile mode is detected through
navigator.userAgent
. It appears that Firefox currently does not include sufficient information to determine whether it is running on iOS. Therefore, FF/iOS currently behaves like on the desktop, which also means it will lose the selection handlebars as soon as the<TextAnnotatorPopup>
opens.