-
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
⚠️ Fixed accessing an obsolete isCollapsed
value on pointerup
handling ⚠️
#163
base: main
Are you sure you want to change the base?
⚠️ Fixed accessing an obsolete isCollapsed
value on pointerup
handling ⚠️
#163
Conversation
const stopPollingInMs = 50; | ||
setTimeout(() => stopPolling = true, stopPollingInMs); | ||
|
||
return poll(() => isCollapsed = sel?.isCollapsed, pollingDelayMs, shouldStopPolling); |
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 first call is immediate, which won't hold off desktop users, for which the isCollapsed: false
is available immediately.
And for the rest of the cases, the polling will stop as soon as the range
finally gets recognized as collapsed
. However, to prevent endless polling and not make the experience sluggish, I limited it with 50ms.
cap
1922b43
to
5d3a8a0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # package-lock.json # packages/text-annotator/package.json
Issue
Source - #136 (comment)
Changes Made
Instead, I introduced "polling" for the range's collapsed state! If the
timeDifference
is smaller than theCLICK_TIMEOUT
and therange
isn't collapsed - we start polling it for up to50ms.
(to be extra cautious). So if the browser changes its state quicker - it won't be held off by a "blind" timeout.