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

Rewrite selection buffering implementation #2703

Merged
merged 1 commit into from
Nov 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@
"typescript": "^4.0.2",
"vinyl": "^2.2.0",
"watchify": "^3.7.0",
"wrap-text": "^1.0.7",
"zen-observable": "^0.3.0"
"wrap-text": "^1.0.7"
},
"browserslist": "chrome 55, edge 17, firefox 53, safari 10.1",
"browserify": {
Expand Down
20 changes: 10 additions & 10 deletions src/annotator/guest.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
setHighlightsVisible,
} from './highlighter';
import * as rangeUtil from './range-util';
import selections from './selections';
import { SelectionObserver } from './selection-observer';
import { normalizeURI } from './util/url';

/**
Expand Down Expand Up @@ -121,14 +121,13 @@ export default class Guest extends Delegator {
this.selectAnnotations(anns);
},
});
this.selections = selections(document).subscribe({
next: range => {
if (range) {
this._onSelection(range);
} else {
this._onClearSelection();
}
},

this.selectionObserver = new SelectionObserver(range => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nice little detail that the API surface is consistent in the new observer class, but without the extra layer of "subscribing".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (range) {
this._onSelection(range);
} else {
this._onClearSelection();
}
});

this.plugins = {};
Expand Down Expand Up @@ -329,7 +328,8 @@ export default class Guest extends Delegator {

destroy() {
this._removeElementEvents();
this.selections.unsubscribe();

this.selectionObserver.disconnect();
this.adderToolbar.remove();

removeAllHighlights(this.element);
Expand Down
98 changes: 98 additions & 0 deletions src/annotator/selection-observer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* Return the current selection or `null` if there is no selection or it is empty.
*
* @param {Document} document
* @return {Range|null}
*/
function selectedRange(document) {
const selection = document.getSelection();
if (!selection || selection.rangeCount === 0) {
return null;
}
const range = selection.getRangeAt(0);
if (range.collapsed) {
return null;
}
return range;
}

/**
* An observer that watches for and buffers changes to the document's current selection.
*/
export class SelectionObserver {
/**
* Start observing changes to the current selection in the document.
*
* @param {(range: Range|null) => any} callback -
* Callback invoked with the selected region of the document when it has
* changed.
* @param {Document} document_ - Test seam
*/
constructor(callback, document_ = document) {
let isMouseDown = false;

this._pendingCallback = null;

const scheduleCallback = (delay = 10) => {
this._pendingCallback = setTimeout(() => {
callback(selectedRange(document_));
}, delay);
};

/** @param {Event} event */
this._eventHandler = event => {
if (event.type === 'mousedown') {
isMouseDown = true;
}
if (event.type === 'mouseup') {
isMouseDown = false;
}

// If the user makes a selection with the mouse, wait until they release
// it before reporting a selection change.
if (isMouseDown) {
return;
}

this._cancelPendingCallback();

// Schedule a notification after a short delay. The delay serves two
// purposes:
//
// - If this handler was called as a result of a 'mouseup' event then the
// selection will not be updated until the next tick of the event loop.
// In this case we only need a short delay.
//
// - If the user is changing the selection with a non-mouse input (eg.
// keyboard or selection handles on mobile) this buffers updates and
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

scheduleCallback(delay);
};

this._document = document_;
this._events = ['mousedown', 'mouseup', 'selectionchange'];
for (let event of this._events) {
document_.addEventListener(event, this._eventHandler);
}

// Report the initial selection.
scheduleCallback(1);
}

disconnect() {
for (let event of this._events) {
this._document.removeEventListener(event, this._eventHandler);
}
this._cancelPendingCallback();
}

_cancelPendingCallback() {
if (this._pendingCallback) {
clearTimeout(this._pendingCallback);
this._pendingCallback = null;
}
}
}
56 changes: 0 additions & 56 deletions src/annotator/selections.js

This file was deleted.

25 changes: 14 additions & 11 deletions src/annotator/test/guest-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Observable } from '../util/observable';
import Delegator from '../delegator';
import Guest from '../guest';
import { $imports } from '../guest';
Expand Down Expand Up @@ -37,7 +36,7 @@ describe('Guest', () => {
let guestConfig;
let htmlAnchoring;
let rangeUtil;
let selections;
let notifySelectionChanged;

const createGuest = (config = {}) => {
const element = document.createElement('div');
Expand All @@ -62,7 +61,7 @@ describe('Guest', () => {
isSelectionBackwards: sinon.stub(),
selectionFocusRect: sinon.stub(),
};
selections = null;
notifySelectionChanged = null;

sinon.stub(window, 'requestAnimationFrame').yields();

Expand All @@ -74,6 +73,13 @@ describe('Guest', () => {
destroy: sinon.stub(),
};

class FakeSelectionObserver {
constructor(callback) {
notifySelectionChanged = callback;
this.disconnect = sinon.stub();
}
}

CrossFrame = sandbox.stub().returns(fakeCrossFrame);
guestConfig.pluginClasses.CrossFrame = CrossFrame;

Expand All @@ -82,11 +88,8 @@ describe('Guest', () => {
'./anchoring/html': htmlAnchoring,
'./highlighter': highlighter,
'./range-util': rangeUtil,
'./selections': () => {
return new Observable(function (obs) {
selections = obs;
return () => {};
});
'./selection-observer': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicer to have this mocked instead of faked out here...good...

SelectionObserver: FakeSelectionObserver,
},
'./delegator': Delegator,
'scroll-into-view': scrollIntoView,
Expand Down Expand Up @@ -451,12 +454,12 @@ describe('Guest', () => {
width: 5,
height: 5,
});
return selections.next({});
notifySelectionChanged({});
};

const simulateSelectionWithoutText = () => {
rangeUtil.selectionFocusRect.returns(null);
return selections.next({});
notifySelectionChanged({});
};

it('shows the adder if the selection contains text', () => {
Expand Down Expand Up @@ -488,7 +491,7 @@ describe('Guest', () => {

it('hides the adder if the selection is empty', () => {
createGuest();
selections.next(null);
notifySelectionChanged(null);
assert.called(FakeAdder.instance.hide);
});

Expand Down
86 changes: 86 additions & 0 deletions src/annotator/test/selection-observer-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { SelectionObserver } from '../selection-observer';

class FakeDocument extends EventTarget {
constructor() {
super();
this.selection = null;
}

getSelection() {
return this.selection;
}
}

describe('SelectionObserver', () => {
let clock;
let fakeDocument;
let range;
let observer;
let onSelectionChanged;

beforeEach(() => {
clock = sinon.useFakeTimers();
fakeDocument = new FakeDocument();
onSelectionChanged = sinon.stub();

range = { collapsed: false };
fakeDocument.selection = {
rangeCount: 1,
getRangeAt: function (index) {
return index === 0 ? range : null;
},
};

observer = new SelectionObserver(range => {
onSelectionChanged(range);
}, fakeDocument);

// Move the clock forwards past the initial event.
clock.tick(10);
onSelectionChanged.reset();
});

afterEach(() => {
observer.disconnect();
clock.restore();
});

it('invokes callback when mouseup occurs', () => {
fakeDocument.dispatchEvent(new Event('mouseup'));
clock.tick(20);
assert.calledWith(onSelectionChanged, range);
});

it('invokes callback with initial selection', () => {
const onInitialSelection = sinon.stub();
const observer = new SelectionObserver(onInitialSelection, fakeDocument);
clock.tick(10);
assert.called(onInitialSelection);
observer.disconnect();
});

describe('when the selection changes', () => {
it('invokes callback if mouse is not down', () => {
fakeDocument.dispatchEvent(new Event('selectionchange'));
clock.tick(200);
assert.calledWith(onSelectionChanged, range);
});

it('does not invoke callback if mouse is down', () => {
fakeDocument.dispatchEvent(new Event('mousedown'));
fakeDocument.dispatchEvent(new Event('selectionchange'));
clock.tick(200);
assert.notCalled(onSelectionChanged);
});

it('does not invoke callback until there is a pause since the last change', () => {
fakeDocument.dispatchEvent(new Event('selectionchange'));
clock.tick(90);
fakeDocument.dispatchEvent(new Event('selectionchange'));
clock.tick(90);
assert.notCalled(onSelectionChanged);
clock.tick(20);
assert.called(onSelectionChanged);
});
});
});
Loading