Commit b22d2de2 authored by Lyza Danger Gardner's avatar Lyza Danger Gardner Committed by Lyza Gardner

Add `getAnnotatableRange` to IntegrationBase interface and classes

Update annotator `IntegrationBase` interface to change `canAnnotate` to
`getAnnotatableContent`. This gives the integration an opportunity to
adjust the Range as needed (e.g. trim it) as part of validating whether
it contains any annotatable content.

Update `Guest` to check the `getAnnotatableContent` results for its
integration when a user selects content.
parent e33027cb
...@@ -706,7 +706,8 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { ...@@ -706,7 +706,8 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
* Show or hide the adder toolbar when the selection changes. * Show or hide the adder toolbar when the selection changes.
*/ */
_onSelection(range: Range) { _onSelection(range: Range) {
if (!this._integration.canAnnotate(range)) { const annotatableRange = this._integration.getAnnotatableRange(range);
if (!annotatableRange) {
this._onClearSelection(); this._onClearSelection();
return; return;
} }
...@@ -720,7 +721,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { ...@@ -720,7 +721,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
return; return;
} }
this.selectedRanges = [range]; this.selectedRanges = [annotatableRange];
this._hostRPC.call('textSelected'); this._hostRPC.call('textSelected');
this._adder.annotationsForSelection = annotationsForSelection(); this._adder.annotationsForSelection = annotationsForSelection();
......
import { TinyEmitter } from 'tiny-emitter'; import { TinyEmitter } from 'tiny-emitter';
import { anchor, describe } from '../anchoring/html'; import { anchor, describe } from '../anchoring/html';
import { TextRange } from '../anchoring/text-range';
import { HTMLMetadata } from './html-metadata'; import { HTMLMetadata } from './html-metadata';
import { import {
...@@ -105,8 +106,21 @@ export class HTMLIntegration extends TinyEmitter { ...@@ -105,8 +106,21 @@ export class HTMLIntegration extends TinyEmitter {
} }
} }
canAnnotate() { /**
return true; * Return a Range trimmed to remove any leading or trailing whitespace, or
* `null` if no valid trimmed Range can be created from `range`
*
* @param {Range} range
*/
getAnnotatableRange(range) {
try {
return TextRange.trimmedRange(range);
} catch (err) {
if (err instanceof RangeError) {
return null;
}
throw err;
}
} }
canStyleClusteredHighlights() { canStyleClusteredHighlights() {
......
...@@ -2,6 +2,7 @@ import debounce from 'lodash.debounce'; ...@@ -2,6 +2,7 @@ import debounce from 'lodash.debounce';
import { render } from 'preact'; import { render } from 'preact';
import { TinyEmitter } from 'tiny-emitter'; import { TinyEmitter } from 'tiny-emitter';
import { TextRange } from '../anchoring/text-range';
import { ListenerCollection } from '../../shared/listener-collection'; import { ListenerCollection } from '../../shared/listener-collection';
import { import {
RenderingStates, RenderingStates,
...@@ -198,12 +199,24 @@ export class PDFIntegration extends TinyEmitter { ...@@ -198,12 +199,24 @@ export class PDFIntegration extends TinyEmitter {
} }
/** /**
* Return true if the text in a range lies within the text layer of a PDF. * Trim `range` to remove leading or trailing empty content, then check to see
* if that trimmed Range lies within a single PDF page's text layer. If so,
* return the trimmed Range.
* *
* @param {Range} range * @param {Range} range
*/ */
canAnnotate(range) { getAnnotatableRange(range) {
return canDescribe(range); try {
const trimmedRange = TextRange.trimmedRange(range);
if (canDescribe(trimmedRange)) {
return trimmedRange;
}
} catch (err) {
if (!(err instanceof RangeError)) {
throw err;
}
}
return null;
} }
/* istanbul ignore next */ /* istanbul ignore next */
......
...@@ -2,6 +2,12 @@ import { delay } from '../../../test-util/wait'; ...@@ -2,6 +2,12 @@ import { delay } from '../../../test-util/wait';
import { FeatureFlags } from '../../features'; import { FeatureFlags } from '../../features';
import { HTMLIntegration, $imports } from '../html'; import { HTMLIntegration, $imports } from '../html';
class FakeTextRange {
static trimmedRange(range) {
return range;
}
}
describe('HTMLIntegration', () => { describe('HTMLIntegration', () => {
let features; let features;
let fakeHTMLAnchoring; let fakeHTMLAnchoring;
...@@ -39,6 +45,7 @@ describe('HTMLIntegration', () => { ...@@ -39,6 +45,7 @@ describe('HTMLIntegration', () => {
const HTMLMetadata = sinon.stub().returns(fakeHTMLMetadata); const HTMLMetadata = sinon.stub().returns(fakeHTMLMetadata);
$imports.$mock({ $imports.$mock({
'../anchoring/html': fakeHTMLAnchoring, '../anchoring/html': fakeHTMLAnchoring,
'../anchoring/text-range': { TextRange: FakeTextRange },
'../util/navigation-observer': { '../util/navigation-observer': {
NavigationObserver: FakeNavigationObserver, NavigationObserver: FakeNavigationObserver,
}, },
...@@ -67,11 +74,38 @@ describe('HTMLIntegration', () => { ...@@ -67,11 +74,38 @@ describe('HTMLIntegration', () => {
assert.equal(integration.describe, fakeHTMLAnchoring.describe); assert.equal(integration.describe, fakeHTMLAnchoring.describe);
}); });
describe('#canAnnotate', () => { describe('#getAnnotatableRange', () => {
it('is always true', () => { let fakeTrimmedRange;
beforeEach(() => {
fakeTrimmedRange = sinon.stub(FakeTextRange, 'trimmedRange');
});
afterEach(() => {
FakeTextRange.trimmedRange.restore();
});
it('returns a trimmed range if range-trimming is successful', () => {
const integration = createIntegration();
const range = new Range();
fakeTrimmedRange.returns(range);
assert.equal(integration.getAnnotatableRange(range), range);
});
it('returns null if range-trimming encounters a RangeError', () => {
fakeTrimmedRange.throws(
new RangeError('Range contains no non-whitespace text')
);
const integration = createIntegration();
const range = new Range();
assert.isNull(integration.getAnnotatableRange(range));
});
it('throws if range-trimming encounters non-RangeError errors', () => {
fakeTrimmedRange.throws(new Error('non-handled Error'));
const integration = createIntegration(); const integration = createIntegration();
const range = new Range(); const range = new Range();
assert.isTrue(integration.canAnnotate(range)); assert.throws(() => integration.getAnnotatableRange(range));
}); });
}); });
......
...@@ -12,6 +12,12 @@ function awaitEvent(target, eventName) { ...@@ -12,6 +12,12 @@ function awaitEvent(target, eventName) {
}); });
} }
class FakeTextRange {
static trimmedRange(range) {
return range;
}
}
describe('annotator/integrations/pdf', () => { describe('annotator/integrations/pdf', () => {
describe('isPDF', () => { describe('isPDF', () => {
beforeEach(() => { beforeEach(() => {
...@@ -93,6 +99,7 @@ describe('annotator/integrations/pdf', () => { ...@@ -93,6 +99,7 @@ describe('annotator/integrations/pdf', () => {
PDFMetadata: sinon.stub().returns(fakePDFMetadata), PDFMetadata: sinon.stub().returns(fakePDFMetadata),
}, },
'../anchoring/pdf': fakePDFAnchoring, '../anchoring/pdf': fakePDFAnchoring,
'../anchoring/text-range': { TextRange: FakeTextRange },
'../util/scroll': fakeScrollUtils, '../util/scroll': fakeScrollUtils,
// Disable debouncing of updates. // Disable debouncing of updates.
...@@ -178,12 +185,37 @@ describe('annotator/integrations/pdf', () => { ...@@ -178,12 +185,37 @@ describe('annotator/integrations/pdf', () => {
}); });
}); });
describe('#canAnnotate', () => { describe('#getAnnotatableRange', () => {
it('checks if range is in text layer of PDF', () => { let fakeTrimmedRange;
beforeEach(() => {
fakeTrimmedRange = sinon.stub(FakeTextRange, 'trimmedRange');
});
afterEach(() => {
FakeTextRange.trimmedRange.restore();
});
it('verifies that range is in text layer of PDF', () => {
const range = new Range(); const range = new Range();
assert.equal(pdfIntegration.canAnnotate(range), true); fakeTrimmedRange.returns(range);
assert.equal(pdfIntegration.getAnnotatableRange(range), range);
assert.calledWith(fakePDFAnchoring.canDescribe, range); assert.calledWith(fakePDFAnchoring.canDescribe, range);
}); });
it('returns null if range-trimming encounters a RangeError', () => {
fakeTrimmedRange.throws(
new RangeError('Range contains no non-whitespace text')
);
const range = new Range();
assert.isNull(pdfIntegration.getAnnotatableRange(range));
});
it('throws if range-trimming encounters non-RangeError errors', () => {
fakeTrimmedRange.throws(new Error('non-handled Error'));
const range = new Range();
assert.throws(() => pdfIntegration.getAnnotatableRange(range));
});
}); });
describe('#describe', () => { describe('#describe', () => {
......
...@@ -80,6 +80,7 @@ describe('annotator/integrations/vitalsource', () => { ...@@ -80,6 +80,7 @@ describe('annotator/integrations/vitalsource', () => {
]), ]),
destroy: sinon.stub(), destroy: sinon.stub(),
fitSideBySide: sinon.stub().returns(false), fitSideBySide: sinon.stub().returns(false),
getAnnotatableRange: sinon.stub().returnsArg(0),
scrollToAnchor: sinon.stub(), scrollToAnchor: sinon.stub(),
sideBySideEnabled: false, sideBySideEnabled: false,
}; };
...@@ -296,9 +297,12 @@ describe('annotator/integrations/vitalsource', () => { ...@@ -296,9 +297,12 @@ describe('annotator/integrations/vitalsource', () => {
integrations.forEach(int => int.destroy()); integrations.forEach(int => int.destroy());
}); });
it('allows annotation', () => { it('delegates to HTML integration to check if selected content is annotatable', () => {
const integration = createIntegration(); const integration = createIntegration();
assert.isTrue(integration.canAnnotate()); const range = new Range();
assert.equal(integration.getAnnotatableRange(range), range);
assert.calledWith(fakeHTMLIntegration.getAnnotatableRange, range);
}); });
it('asks guest to wait for feature flags before sending document info', () => { it('asks guest to wait for feature flags before sending document info', () => {
......
...@@ -304,8 +304,8 @@ export class VitalSourceContentIntegration ...@@ -304,8 +304,8 @@ export class VitalSourceContentIntegration
} }
} }
canAnnotate() { getAnnotatableRange(range: Range) {
return true; return this._htmlIntegration.getAnnotatableRange(range);
} }
destroy() { destroy() {
......
...@@ -139,7 +139,7 @@ describe('Guest', () => { ...@@ -139,7 +139,7 @@ describe('Guest', () => {
fakeIntegration = Object.assign(new TinyEmitter(), { fakeIntegration = Object.assign(new TinyEmitter(), {
anchor: sinon.stub(), anchor: sinon.stub(),
canAnnotate: sinon.stub().returns(true), getAnnotatableRange: sinon.stub().returnsArg(0),
canStyleClusteredHighlights: sinon.stub().returns(false), canStyleClusteredHighlights: sinon.stub().returns(false),
contentContainer: sinon.stub().returns({}), contentContainer: sinon.stub().returns({}),
describe: sinon.stub(), describe: sinon.stub(),
...@@ -726,8 +726,7 @@ describe('Guest', () => { ...@@ -726,8 +726,7 @@ describe('Guest', () => {
it('hides the adder if the integration indicates that the selection cannot be annotated', () => { it('hides the adder if the integration indicates that the selection cannot be annotated', () => {
// Simulate integration indicating text is not part of annotatable content // Simulate integration indicating text is not part of annotatable content
// (eg. text that is part of the PDF.js UI) // (eg. text that is part of the PDF.js UI)
fakeIntegration.canAnnotate.returns(false); fakeIntegration.getAnnotatableRange = sinon.stub().returns(null);
createGuest(); createGuest();
simulateSelectionWithText(); simulateSelectionWithText();
......
...@@ -156,11 +156,6 @@ export type SidebarLayout = { ...@@ -156,11 +156,6 @@ export type SidebarLayout = {
* of supporting a specific document type (web page, PDF, ebook, etc.). * of supporting a specific document type (web page, PDF, ebook, etc.).
*/ */
export type IntegrationBase = { export type IntegrationBase = {
/**
* Return whether the specified DOM range is part of the annotatable content
* of the current document.
*/
canAnnotate(range: Range): boolean;
/** /**
* Return whether this integration supports styling multiple clusters of highlights * Return whether this integration supports styling multiple clusters of highlights
*/ */
...@@ -185,6 +180,14 @@ export type IntegrationBase = { ...@@ -185,6 +180,14 @@ export type IntegrationBase = {
*/ */
fitSideBySide(layout: SidebarLayout): boolean; fitSideBySide(layout: SidebarLayout): boolean;
/**
* Return a DOM Range representing the extent of annotatable content within
* `range`, or `null` if `range` does not contain any annotatable content.
* For example, `range` might be trimmed of leading or trailing whitespace.
* `range` may be returned unmodified if already valid.
*/
getAnnotatableRange(range: Range): Range | null;
/** Return the metadata of the currently loaded document, such as title, PDF fingerprint, etc. */ /** Return the metadata of the currently loaded document, such as title, PDF fingerprint, etc. */
getMetadata(): Promise<DocumentMetadata>; getMetadata(): Promise<DocumentMetadata>;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment