Rewrite selection buffering implementation #2703
Conversation
Rewrite the selection change observer in `src/annotator/selections` to make it easier to understand how this functionality works and change it in future. The major change is to remove the dependency on the `Observable` class provided by `zen-observable`, which adds cognitive overhead here, isn't used elsewhere and is stuck on an old version due to subtle breaking changes in newer releases. Replace it instead with a `SelectionObserver` class which is modeled after DOM APIs like `MutationObserver`. It takes a callback in the constructor that is invoked with the selected `Range` or `null` when the selection changes and provides a `disconnect` method which stops watching for future changes. The implementation only uses DOM APIs.
Codecov Report
@@ Coverage Diff @@
## master #2703 +/- ##
==========================================
- Coverage 97.73% 97.72% -0.01%
==========================================
Files 200 199 -1
Lines 7497 7470 -27
Branches 1647 1654 +7
==========================================
- Hits 7327 7300 -27
Misses 170 170
Continue to review full report at Codecov.
|
| // makes sure that we only report one when the update has stopped | ||
| // changing. In this case we want a longer delay. | ||
|
|
||
| const delay = event.type === 'mouseup' ? 10 : 100; |
There was a problem hiding this comment.
The delay differences here are explained quite well in the preceding comments and make sense. I am wondering, however, how much user-facing difference there would be (I'm wondering if possibly negligible) if we always used the longer delay, for simplicity's sake. Put another way: how much value does shortening the delay to 10ms add versus the complexity introduced to explain and manage the difference?
There was a problem hiding this comment.
Its debatable. 100ms is just long enough to be perceptible, so the effect of always using that delay is that it might feel like the adder was fractionally less responsive in showing up when releasing the mouse.
| } | ||
| }, | ||
|
|
||
| this.selectionObserver = new SelectionObserver(range => { |
There was a problem hiding this comment.
It's a nice little detail that the API surface is consistent in the new observer class, but without the extra layer of "subscribing".
There was a problem hiding this comment.
Yeah, using Observable seemed like a nice idea when the API appeared to be on track to be standardized as part of ES. However progress on that stalled.
| selections = obs; | ||
| return () => {}; | ||
| }); | ||
| './selection-observer': { |
There was a problem hiding this comment.
Nicer to have this mocked instead of faked out here...good...
Rewrite the selection change observer in
src/annotator/selectionstomake it easier to understand how this functionality works and change it
in future (for example, to address issues like #2626 or modify for use with new document types).
The major change is to remove the dependency on the
Observableclassprovided by
zen-observable, which adds cognitive overhead here, isn't usedelsewhere and is stuck on an old version due to subtle breaking changes in newer releases.
Replace it instead with a
SelectionObserverclass which is modeledafter DOM APIs like
MutationObserver. It takes a callback in theconstructor that is invoked with the selected
Rangeornullwhen theselection changes and provides a
disconnectmethod which stopswatching for future changes. The implementation only uses DOM APIs.