From 55093ebb8c1020e44680f51ad9385afd8f523afd Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Sun, 11 Sep 2016 12:02:49 +0200 Subject: [PATCH] Simplify the sidebar app side of sidebar <-> host frame messaging Previously the communication between the sidebar and host frame was implemented by a shared AnnotationSync class, with a 'crossframe' service which abstracted the event bus on each side. This design however assumed that both sides wanted to listen to the same messages and react to them in similar ways. This is not the case, especially given the change to use Redux for state management in the sidebar app. This commit replaces the crossframe service and AnnotationSync class with a new 'frameSync' service which implements only the event listeners and sidebar -> page RPC calls that are actually needed. As a result, local annotation tags for annotations loaded via the API can now be assigned by the reducer in the Redux store, making this easier to test and getting us another step closer to making Annotation objects immutable in the sidebar app. --- .eslintrc | 5 +- h/static/scripts/app.js | 12 +- h/static/scripts/cross-frame.coffee | 80 ------- h/static/scripts/directive/help-panel.js | 4 +- h/static/scripts/directive/share-dialog.js | 4 +- .../directive/test/share-dialog-test.js | 10 +- h/static/scripts/frame-sync.js | 207 ++++++++++++++++++ h/static/scripts/polyfills.js | 1 + h/static/scripts/reducers/annotations.js | 41 ++-- h/static/scripts/test/annotation-ui-test.js | 28 ++- h/static/scripts/test/cross-frame-test.coffee | 94 -------- h/static/scripts/test/fake-redux-store.js | 29 +++ h/static/scripts/test/frame-sync-test.js | 189 ++++++++++++++++ .../scripts/test/widget-controller-test.js | 45 ++-- h/static/scripts/widget-controller.js | 14 +- 15 files changed, 525 insertions(+), 238 deletions(-) delete mode 100644 h/static/scripts/cross-frame.coffee create mode 100644 h/static/scripts/frame-sync.js delete mode 100644 h/static/scripts/test/cross-frame-test.coffee create mode 100644 h/static/scripts/test/fake-redux-store.js create mode 100644 h/static/scripts/test/frame-sync-test.js diff --git a/.eslintrc b/.eslintrc index cdea9740389..51a043cddd4 100644 --- a/.eslintrc +++ b/.eslintrc @@ -1,3 +1,6 @@ { - "extends": "hypothesis" + "extends": "hypothesis", + "globals": { + "Set": false + } } diff --git a/h/static/scripts/app.js b/h/static/scripts/app.js index b4b48ac7946..35045bf3415 100644 --- a/h/static/scripts/app.js +++ b/h/static/scripts/app.js @@ -83,8 +83,11 @@ function configureRoutes($routeProvider) { } // @ngInject -function setupCrossFrame(crossframe) { - return crossframe.connect(); +function setupFrameSync(frameSync) { + // Setup the connection to the frame hosting the sidebar app. + // This should only be done if this is the sidebar app, not the stream or + // standalone annotation pages. + return frameSync.connect(); } // @ngInject @@ -173,11 +176,11 @@ module.exports = angular.module('h', [ .service('annotationUI', require('./annotation-ui')) .service('auth', require('./auth').service) .service('bridge', require('./bridge')) - .service('crossframe', require('./cross-frame')) .service('drafts', require('./drafts')) .service('features', require('./features')) .service('flash', require('./flash')) .service('formRespond', require('./form-respond')) + .service('frameSync', require('./frame-sync').default) .service('groups', require('./groups')) .service('host', require('./host')) .service('localStorage', require('./local-storage')) @@ -195,7 +198,6 @@ module.exports = angular.module('h', [ .factory('store', require('./store')) - .value('AnnotationSync', require('./annotation-sync')) .value('AnnotationUISync', require('./annotation-ui-sync')) .value('Discovery', require('./discovery')) .value('ExcerptOverflowMonitor', require('./directive/excerpt-overflow-monitor')) @@ -209,7 +211,7 @@ module.exports = angular.module('h', [ .config(configureLocation) .config(configureRoutes) - .run(setupCrossFrame) + .run(setupFrameSync) .run(setupHttp); processAppOpts(); diff --git a/h/static/scripts/cross-frame.coffee b/h/static/scripts/cross-frame.coffee deleted file mode 100644 index 31739acd174..00000000000 --- a/h/static/scripts/cross-frame.coffee +++ /dev/null @@ -1,80 +0,0 @@ -# Instantiates all objects used for cross frame discovery and communication. -module.exports = class CrossFrame - this.$inject = [ - '$rootScope', '$document', '$window', 'store', 'annotationUI' - 'Discovery', 'bridge', - 'AnnotationSync', 'AnnotationUISync' - ] - constructor: ( - $rootScope, $document, $window, store, annotationUI - Discovery, bridge, - AnnotationSync, AnnotationUISync - ) -> - @frames = [] - - createDiscovery = -> - options = - server: true - new Discovery($window, options) - - createAnnotationSync = -> - whitelist = ['$orphan', '$highlight', 'target', 'document', 'uri'] - options = - formatter: (annotation) -> - formatted = {} - for k, v of annotation when k in whitelist - formatted[k] = v - formatted - parser: (annotation) -> - parsed = {} - for k, v of annotation when k in whitelist - parsed[k] = v - parsed - merge: (local, remote) -> - annotationUI.updateAnchorStatus(local.id, local.$$tag, remote.$orphan) - local - emit: (args...) -> - $rootScope.$apply -> - $rootScope.$broadcast.call($rootScope, args...) - on: (event, handler) -> - $rootScope.$on(event, (event, args...) -> handler.apply(this, args)) - - new AnnotationSync(bridge, options) - - createAnnotationUISync = (annotationSync) -> - new AnnotationUISync($rootScope, $window, annotationUI, bridge) - - addFrame = (channel) => - channel.call 'getDocumentInfo', (err, info) => - if err - channel.destroy() - else - searchUris = [info.uri] - - documentFingerprint = null - if info.metadata and info.metadata.documentFingerprint - documentFingerprint = info.metadata.documentFingerprint - searchUris = info.metadata.link.map((link) -> link.href) - - $rootScope.$apply => - @frames.push({ - channel: channel, - uri: info.uri, - searchUris: searchUris, - documentFingerprint: documentFingerprint - }) - - this.connect = -> - discovery = createDiscovery() - - bridge.onConnect(addFrame) - annotationSync = createAnnotationSync() - annotationUISync = createAnnotationUISync(annotationSync) - - onDiscoveryCallback = (source, origin, token) -> - bridge.createChannel(source, origin, token) - discovery.startDiscovery(onDiscoveryCallback) - - this.call = bridge.call.bind(bridge) - - this.call = -> throw new Error('connect() must be called before call()') diff --git a/h/static/scripts/directive/help-panel.js b/h/static/scripts/directive/help-panel.js index fa65cf24814..f580aa303a2 100644 --- a/h/static/scripts/directive/help-panel.js +++ b/h/static/scripts/directive/help-panel.js @@ -11,7 +11,7 @@ module.exports = function () { bindToController: true, controllerAs: 'vm', // @ngInject - controller: function ($scope, $window, crossframe, serviceUrl) { + controller: function ($scope, $window, frameSync, serviceUrl) { this.userAgent = $window.navigator.userAgent; this.version = '__VERSION__'; // replaced by versionify this.dateTime = new Date(); @@ -19,7 +19,7 @@ module.exports = function () { $scope.$watchCollection( function () { - return crossframe.frames; + return frameSync.frames; }, function (frames) { if (frames.length === 0) { diff --git a/h/static/scripts/directive/share-dialog.js b/h/static/scripts/directive/share-dialog.js index 5d6df02562d..f7db286bf3e 100644 --- a/h/static/scripts/directive/share-dialog.js +++ b/h/static/scripts/directive/share-dialog.js @@ -3,7 +3,7 @@ var VIA_PREFIX = 'https://via.hypothes.is/'; // @ngInject -function ShareDialogController($scope, $element, crossframe) { +function ShareDialogController($scope, $element, frameSync) { var self = this; function updateViaLink(frames) { @@ -24,7 +24,7 @@ function ShareDialogController($scope, $element, crossframe) { viaInput.focus(); viaInput.select(); - $scope.$watchCollection(function () { return crossframe.frames; }, + $scope.$watchCollection(function () { return frameSync.frames; }, updateViaLink); } diff --git a/h/static/scripts/directive/test/share-dialog-test.js b/h/static/scripts/directive/test/share-dialog-test.js index c6590bd880a..0bb262d18bf 100644 --- a/h/static/scripts/directive/test/share-dialog-test.js +++ b/h/static/scripts/directive/test/share-dialog-test.js @@ -5,28 +5,28 @@ var angular = require('angular'); var util = require('./util'); describe('shareDialog', function () { - var fakeCrossFrame; + var fakeFrameSync; beforeEach(function () { - fakeCrossFrame = { frames: [] }; + fakeFrameSync = { frames: [] }; angular.module('h', []) .directive('shareDialog', require('../share-dialog')) - .value('crossframe', fakeCrossFrame) + .value('frameSync', fakeFrameSync) .value('urlEncodeFilter', function (val) { return val; }); angular.mock.module('h'); }); it('generates new via link', function () { var element = util.createDirective(document, 'shareDialog', {}); - fakeCrossFrame.frames.push({ uri: 'http://example.com' }); + fakeFrameSync.frames.push({ uri: 'http://example.com' }); element.scope.$digest(); assert.equal(element.ctrl.viaPageLink, 'https://via.hypothes.is/http://example.com'); }); it('does not generate new via link if already on via', function () { var element = util.createDirective(document, 'shareDialog', {}); - fakeCrossFrame.frames.push({ uri: 'https://via.hypothes.is/http://example.com' }); + fakeFrameSync.frames.push({ uri: 'https://via.hypothes.is/http://example.com' }); element.scope.$digest(); assert.equal(element.ctrl.viaPageLink, 'https://via.hypothes.is/http://example.com'); }); diff --git a/h/static/scripts/frame-sync.js b/h/static/scripts/frame-sync.js new file mode 100644 index 00000000000..b2399a639c3 --- /dev/null +++ b/h/static/scripts/frame-sync.js @@ -0,0 +1,207 @@ +'use strict'; + +var events = require('./events'); +var metadata = require('./annotation-metadata'); + +/** + * @typedef FrameInfo + * @property {string} uri - Current primary URI of the document being displayed + * @property {string[]} searchUris - List of URIs that should be passed to the + * search API when searching for annotations on this document. + * @property {string} documentFingerprint - Fingerprint of the document, used + * for PDFs + */ + + /** + * Return a minimal representation of an annotation that can be sent from the + * sidebar app to a connected frame. + * + * Because this representation will be exposed to untrusted third-party + * JavaScript, it includes only the information needed to uniquely identify it + * within the current session and anchor it in the document. + */ +function formatAnnot(ann) { + return { + tag: ann.$$tag, + msg: { + document: ann.document, + target: ann.target, + uri: ann.uri, + }, + }; +} + +/** + * This service runs in the sidebar and is responsible for keeping the set of + * annotations displayed in connected frames in sync with the set shown in the + * sidebar. + */ +// @ngInject +function FrameSync($rootScope, $window, AnnotationUISync, Discovery, + annotationUI, bridge) { + + // List of frames currently connected to the sidebar + var frames = []; + + // Set of tags of annotations that are currently loaded into the frame + var inFrame = new Set(); + + /** + * Watch for changes to the set of annotations displayed in the sidebar and + * notify connected frames about new/updated/deleted annotations. + */ + function setupSyncToFrame() { + // List of loaded annotations in previous state + var prevAnnotations = []; + + annotationUI.subscribe(function () { + var state = annotationUI.getState(); + if (state.annotations === prevAnnotations) { + return; + } + + var inSidebar = new Set(); + var added = []; + + state.annotations.forEach(function (annot) { + if (metadata.isReply(annot)) { + // The frame does not need to know about replies + return; + } + + inSidebar.add(annot.$$tag); + if (!inFrame.has(annot.$$tag)) { + added.push(annot); + } + }); + var deleted = prevAnnotations.filter(function (annot) { + return !inSidebar.has(annot.$$tag); + }); + prevAnnotations = state.annotations; + + // We currently only handle adding and removing annotations from the frame + // when they are added or removed in the sidebar, but not re-anchoring + // annotations if their selectors are updated. + if (added.length > 0) { + bridge.call('loadAnnotations', added.map(formatAnnot)); + added.forEach(function (annot) { + inFrame.add(annot.$$tag); + }); + } + deleted.forEach(function (annot) { + bridge.call('deleteAnnotation', formatAnnot(annot)); + inFrame.delete(annot.$$tag); + }); + }); + } + + /** + * Listen for messages coming in from connected frames and add new annotations + * to the sidebar. + */ + function setupSyncFromFrame() { + // A new annotation, note or highlight was created in the frame + bridge.on('beforeCreateAnnotation', function (event) { + inFrame.add(event.tag); + var annot = Object.assign({}, event.msg, {$$tag: event.tag}); + $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot); + }); + + // Anchoring an annotation in the frame completed + bridge.on('sync', function (events_) { + events_.forEach(function (event) { + inFrame.add(event.tag); + annotationUI.updateAnchorStatus(null, event.tag, event.msg.$orphan); + $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, [event.tag]); + }); + }); + + // Create an instance of the AnnotationUISync class which listens for + // selection/focus messages from the frame and propagates them to the rest + // of the sidebar app. + // + // FIXME: The frame message listeners from AnnotationUISync should be + // extracted and moved here and then the AnnotationUISync class can be + // removed entirely. + new AnnotationUISync($rootScope, $window, annotationUI, bridge); + } + + /** + * Query the Hypothesis annotation client in a frame for the URL and metadata + * of the document that is currently loaded and add the result to the set of + * connected frames. + */ + function addFrame(channel) { + channel.call('getDocumentInfo', function (err, info) { + var searchUris = []; + + if (err) { + channel.destroy(); + } else { + searchUris = [info.uri]; + } + + var documentFingerprint; + if (info.metadata && info.metadata.documentFingerprint) { + documentFingerprint = info.metadata.documentFingerprint; + searchUris = info.metadata.link.map(function (link) { + return link.href; + }); + } + + // The `frames` list is currently stored by this service but should in + // future be moved to the app state. + $rootScope.$apply(function () { + frames.push({ + uri: info.uri, + searchUris: searchUris, + documentFingerprint: documentFingerprint, + }); + }); + }); + } + + /** + * Find and connect to Hypothesis clients in the current window. + */ + this.connect = function () { + var discovery = new Discovery(window, {server: true}); + discovery.startDiscovery(bridge.createChannel.bind(bridge)); + bridge.onConnect(addFrame); + + setupSyncToFrame(); + setupSyncFromFrame(); + }; + + /** + * Focus annotations with the given tags. + * + * This is used to indicate the highlight in the document that corresponds to + * a given annotation in the sidebar. + * + * @param {string[]} tags + */ + this.focusAnnotations = function (tags) { + bridge.call('focusAnnotations', tags); + }; + + /** + * Scroll the frame to the highlight for an annotation with a given tag. + * + * @param {string} tag + */ + this.scrollToAnnotation = function (tag) { + bridge.call('scrollToAnnotation', tag); + }; + + /** + * List of frames that are connected to the app. + * @type {FrameInfo} + */ + this.frames = frames; +} + +module.exports = { + default: FrameSync, + formatAnnot: formatAnnot, +}; diff --git a/h/static/scripts/polyfills.js b/h/static/scripts/polyfills.js index 04e19370004..3986d879364 100644 --- a/h/static/scripts/polyfills.js +++ b/h/static/scripts/polyfills.js @@ -2,6 +2,7 @@ // ES2015 polyfills require('core-js/es6/promise'); +require('core-js/es6/set'); require('core-js/fn/array/find'); require('core-js/fn/array/find-index'); require('core-js/fn/array/from'); diff --git a/h/static/scripts/reducers/annotations.js b/h/static/scripts/reducers/annotations.js index dc5121dee14..f6159b60708 100644 --- a/h/static/scripts/reducers/annotations.js +++ b/h/static/scripts/reducers/annotations.js @@ -49,30 +49,32 @@ function findByTag(annotations, tag) { /** * Initialize the status flags and properties of a new annotation. */ -function initializeAnnot(annotation) { - if (annotation.id) { - return annotation; - } +function initializeAnnot(annotation, tag) { + var orphan = annotation.$orphan; - // Currently the user ID, permissions and group of new annotations are - // initialized in the component controller because the session - // state and focused group are not stored in the Redux store. Once they are, - // that initialization should be moved here. + if (!annotation.id) { + // Currently the user ID, permissions and group of new annotations are + // initialized in the component controller because the session + // state and focused group are not stored in the Redux store. Once they are, + // that initialization should be moved here. - return Object.assign({}, annotation, { - // Copy $$tag explicitly because it is non-enumerable. - // - // FIXME: change $$tag to $tag and make it enumerable so annotations can be - // handled more simply in the sidebar. - $$tag: annotation.$$tag, // New annotations must be anchored - $orphan: false, + orphan = false; + } + + return Object.assign({}, annotation, { + $$tag: annotation.$$tag || tag, + $orphan: orphan, }); } function init() { return { annotations: [], + + // The local tag to assign to the next annotation that is loaded into the + // app + nextTag: 1, }; } @@ -84,6 +86,7 @@ var update = { var added = []; var unchanged = []; var updated = []; + var nextTag = state.nextTag; action.annotations.forEach(function (annot) { var existing; @@ -105,7 +108,8 @@ var update = { updatedTags[existing.$$tag] = true; } } else { - added.push(initializeAnnot(annot)); + added.push(initializeAnnot(annot, 't' + nextTag)); + ++nextTag; } }); @@ -115,7 +119,10 @@ var update = { } }); - return {annotations: added.concat(updated).concat(unchanged)}; + return { + annotations: added.concat(updated).concat(unchanged), + nextTag: nextTag, + }; }, REMOVE_ANNOTATIONS: function (state, action) { diff --git a/h/static/scripts/test/annotation-ui-test.js b/h/static/scripts/test/annotation-ui-test.js index e7dc6b3bd93..cf78b06e07b 100644 --- a/h/static/scripts/test/annotation-ui-test.js +++ b/h/static/scripts/test/annotation-ui-test.js @@ -67,7 +67,21 @@ describe('annotationUI', function () { it('adds annotations not in the store', function () { var annot = defaultAnnotation(); annotationUI.addAnnotations([annot]); - assert.deepEqual(annotationUI.getState().annotations, [annot]); + assert.match(annotationUI.getState().annotations, + [sinon.match(annot)]); + }); + + it('assigns a local tag to annotations', function () { + var annotA = Object.assign(defaultAnnotation(), {id: 'a1'}); + var annotB = Object.assign(defaultAnnotation(), {id: 'a2'}); + + annotationUI.addAnnotations([annotA, annotB]); + + var tags = annotationUI.getState().annotations.map(function (a) { + return a.$$tag; + }); + + assert.deepEqual(tags, ['t1','t2']); }); it('updates annotations with matching IDs in the store', function () { @@ -209,13 +223,21 @@ describe('annotationUI', function () { it('matches annotations to remove by ID', function () { annotationUI.addAnnotations(fixtures.pair); annotationUI.removeAnnotations([{id: fixtures.pair[0].id}]); - assert.deepEqual(annotationUI.getState().annotations, [fixtures.pair[1]]); + + var ids = annotationUI.getState().annotations.map(function (a) { + return a.id; + }); + assert.deepEqual(ids, [fixtures.pair[1].id]); }); it('matches annotations to remove by tag', function () { annotationUI.addAnnotations(fixtures.pair); annotationUI.removeAnnotations([{$$tag: fixtures.pair[0].$$tag}]); - assert.deepEqual(annotationUI.getState().annotations, [fixtures.pair[1]]); + + var tags = annotationUI.getState().annotations.map(function (a) { + return a.$$tag; + }); + assert.deepEqual(tags, [fixtures.pair[1].$$tag]); }); it('switches back to the Annotations tab when the last orphan is removed', function () { diff --git a/h/static/scripts/test/cross-frame-test.coffee b/h/static/scripts/test/cross-frame-test.coffee deleted file mode 100644 index 9e0cb288bea..00000000000 --- a/h/static/scripts/test/cross-frame-test.coffee +++ /dev/null @@ -1,94 +0,0 @@ -{module, inject} = angular.mock - -describe 'CrossFrame', -> - sandbox = sinon.sandbox.create() - crossframe = null - $rootScope = null - $fakeDocument = null - $fakeWindow = null - fakeStore = null - fakeAnnotationUI = null - fakeDiscovery = null - fakeBridge = null - fakeAnnotationSync = null - fakeAnnotationUISync = null - - before -> - angular.module('h', []) - .service('crossframe', require('../cross-frame')) - - beforeEach module('h') - beforeEach module ($provide) -> - $fakeDocument = {} - $fakeWindow = {} - fakeStore = {} - fakeAnnotationUI = {} - fakeDiscovery = - startDiscovery: sandbox.stub() - fakeBridge = - call: sandbox.stub() - createChannel: sandbox.stub() - onConnect: sandbox.stub() - fakeAnnotationSync = {} - fakeAnnotationUISync = {} - - $provide.value('$document', $fakeDocument) - $provide.value('$window', $fakeWindow) - $provide.value('store', fakeStore) - $provide.value('annotationUI', fakeAnnotationUI) - $provide.value('Discovery', - sandbox.stub().returns(fakeDiscovery)) - $provide.value('bridge', fakeBridge) - $provide.value('AnnotationSync', - sandbox.stub().returns(fakeAnnotationSync)) - $provide.value('AnnotationUISync', - sandbox.stub().returns(fakeAnnotationUISync)) - return # $provide returns a promise. - - beforeEach inject (_$rootScope_, _crossframe_) -> - $rootScope = _$rootScope_ - crossframe = _crossframe_ - - afterEach -> - sandbox.restore() - - describe '.connect()', -> - it 'creates a new channel when the discovery module finds a frame', -> - fakeDiscovery.startDiscovery.yields('source', 'origin', 'token') - crossframe.connect() - assert.calledWith(fakeBridge.createChannel, - 'source', 'origin', 'token') - - it 'queries discovered frames for metadata', -> - uri = 'http://example.com' - channel = {call: sandbox.stub().yields(null, {uri: uri})} - fakeBridge.onConnect.yields(channel) - crossframe.connect() - assert.calledWith(channel.call, 'getDocumentInfo', sinon.match.func) - - it 'updates the frames array', -> - uri = 'http://example.com' - channel = {call: sandbox.stub().yields(null, {uri: uri})} - fakeBridge.onConnect.yields(channel) - crossframe.connect() - assert.deepEqual(crossframe.frames, [ - {channel: channel, uri: uri, searchUris: [uri], documentFingerprint: null} - ]) - - it 'updates the frames array with multiple search uris when the document is a PDF', -> - uri = 'http://example.com/test.pdf' - fingerprint = 'urn:x-pdf:fingerprint' - channel = { - call: sandbox.stub().yields(null, { - uri: uri, - metadata: { - link: [{href: uri}, {href: fingerprint}], - documentFingerprint: fingerprint, - }, - }) - } - fakeBridge.onConnect.yields(channel) - crossframe.connect() - assert.deepEqual(crossframe.frames, [ - {channel: channel, uri: uri, searchUris: [uri, fingerprint], documentFingerprint: fingerprint} - ]) diff --git a/h/static/scripts/test/fake-redux-store.js b/h/static/scripts/test/fake-redux-store.js new file mode 100644 index 00000000000..7a97f839966 --- /dev/null +++ b/h/static/scripts/test/fake-redux-store.js @@ -0,0 +1,29 @@ +'use strict'; + +var redux = require('redux'); + +/** + * Utility function that creates a fake Redux store for use in tests. + * + * Unlike a real store, this has a `setState()` method that can be used to + * set the state directly. + */ +function fakeStore(initialState) { + function update(state, action) { + if (action.state) { + return action.state; + } else { + return state; + } + } + + var store = redux.createStore(update, initialState); + + store.setState = function (state) { + store.dispatch({type: 'SET_STATE', state: state}); + }; + + return store; +} + +module.exports = fakeStore; diff --git a/h/static/scripts/test/frame-sync-test.js b/h/static/scripts/test/frame-sync-test.js new file mode 100644 index 00000000000..574a4cd263a --- /dev/null +++ b/h/static/scripts/test/frame-sync-test.js @@ -0,0 +1,189 @@ +'use strict'; + +var angular = require('angular'); +var EventEmitter = require('tiny-emitter'); + +var annotationFixtures = require('./annotation-fixtures'); +var events = require('../events'); +var FrameSync = require('../frame-sync').default; +var fakeStore = require('./fake-redux-store'); +var formatAnnot = require('../frame-sync').formatAnnot; + +var fixtures = { + ann: Object.assign({$$tag: 't1'}, annotationFixtures.defaultAnnotation()), + + // New annotation received from the frame + newAnnFromFrame: { + tag: 't1', + msg: { + target: [], + }, + }, + + // Response to the `getDocumentInfo` channel message for a frame displaying + // an HTML document + htmlDocumentInfo: { + uri: 'http://example.org', + metadata: { + link: [], + }, + }, + + // Response to the `getDocumentInfo` channel message for a frame displaying + // a PDF + pdfDocumentInfo: { + uri: 'http://example.org/paper.pdf', + metadata: { + documentFingerprint: '1234', + link: [{href: 'http://example.org/paper.pdf'}, {href:'urn:1234'}], + }, + }, +}; + +describe('FrameSync', function () { + var fakeAnnotationUI; + var fakeBridge; + var frameSync; + var $rootScope; + + before(function () { + angular.module('app', []) + .service('frameSync', FrameSync); + }); + + beforeEach(function () { + fakeAnnotationUI = fakeStore({annotations: []}); + fakeAnnotationUI.updateAnchorStatus = sinon.stub(); + + var emitter = new EventEmitter(); + fakeBridge = { + call: sinon.stub(), + createChannel: sinon.stub(), + on: emitter.on.bind(emitter), + onConnect: function (listener) { + emitter.on('connect', listener); + }, + + emit: emitter.emit.bind(emitter), + }; + + function FakeDiscovery() { + this.startDiscovery = sinon.stub(); + } + + angular.mock.module('app', { + AnnotationUISync: sinon.stub(), + Discovery: FakeDiscovery, + annotationUI: fakeAnnotationUI, + bridge: fakeBridge, + }); + + angular.mock.inject(function (_$rootScope_, _frameSync_) { + $rootScope = _$rootScope_; + frameSync = _frameSync_; + }); + }); + + beforeEach(function () { + frameSync.connect(); + }); + + context('when annotations are loaded into the sidebar', function () { + it('sends a "loadAnnotations" message to the frame', function () { + fakeAnnotationUI.setState({annotations: [fixtures.ann]}); + assert.calledWithMatch(fakeBridge.call, 'loadAnnotations', sinon.match([ + formatAnnot(fixtures.ann), + ])); + }); + + it('sends a "loadAnnotations" message only for new annotations', function () { + var ann2 = Object.assign({}, fixtures.ann, {$$tag: 't2', id: 'a2'}); + fakeAnnotationUI.setState({annotations: [fixtures.ann]}); + fakeBridge.call.reset(); + + fakeAnnotationUI.setState({annotations: [fixtures.ann, ann2]}); + + assert.calledWithMatch(fakeBridge.call, 'loadAnnotations', sinon.match([ + formatAnnot(ann2), + ])); + }); + + it('does not send a "loadAnnotations" message for replies', function () { + fakeAnnotationUI.setState({annotations: [annotationFixtures.newReply()]}); + assert.notCalled(fakeBridge.call); + }); + }); + + context('when annotations are removed from the sidebar', function () { + it('sends a "deleteAnnotation" message to the frame', function () { + fakeAnnotationUI.setState({annotations: [fixtures.ann]}); + fakeAnnotationUI.setState({annotations: []}); + assert.calledWithMatch(fakeBridge.call, 'deleteAnnotation', + sinon.match(formatAnnot(fixtures.ann))); + }); + }); + + context('when a new annotation is created in the frame', function () { + it('emits a BEFORE_ANNOTATION_CREATED event', function () { + var onCreated = sinon.stub(); + var ann = {target: []}; + $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, onCreated); + + fakeBridge.emit('beforeCreateAnnotation', {tag: 't1', msg: ann}); + + assert.calledWithMatch(onCreated, sinon.match.any, sinon.match({ + $$tag: 't1', + target: [], + })); + }); + }); + + context('when anchoring completes', function () { + it('updates the anchoring status for the annotation', function () { + fakeBridge.emit('sync', [{tag: 't1', msg: {$orphan: false}}]); + assert.calledWith(fakeAnnotationUI.updateAnchorStatus, null, 't1', false); + }); + + it('emits an ANNOTATIONS_SYNCED event', function () { + var onSync = sinon.stub(); + $rootScope.$on(events.ANNOTATIONS_SYNCED, onSync); + + fakeBridge.emit('sync', [{tag: 't1', msg: {$orphan: false}}]); + + assert.calledWithMatch(onSync, sinon.match.any, sinon.match(['t1'])); + }); + }); + + context('when a new frame connects', function () { + var frameInfo; + var fakeChannel = { + call: function (name, callback) { + callback(null, frameInfo); + }, + }; + + it("adds the page's metadata to the frames list", function () { + frameInfo = fixtures.htmlDocumentInfo; + + fakeBridge.emit('connect', fakeChannel); + + assert.deepEqual(frameSync.frames, [{ + documentFingerprint: undefined, + searchUris: [frameInfo.uri], + uri: frameInfo.uri, + }]); + }); + + it('adds the document fingerprint for PDFs', function () { + frameInfo = fixtures.pdfDocumentInfo; + + fakeBridge.emit('connect', fakeChannel); + + assert.deepEqual(frameSync.frames, [{ + documentFingerprint: frameInfo.metadata.documentFingerprint, + searchUris: [frameInfo.uri, 'urn:1234'], + uri: frameInfo.uri, + }]); + }); + }); +}); diff --git a/h/static/scripts/test/widget-controller-test.js b/h/static/scripts/test/widget-controller-test.js index aeb212c8679..bacd3f74359 100644 --- a/h/static/scripts/test/widget-controller-test.js +++ b/h/static/scripts/test/widget-controller-test.js @@ -41,9 +41,9 @@ describe('WidgetController', function () { var $scope; var annotationUI; var fakeAnnotationMapper; - var fakeCrossFrame; var fakeDrafts; var fakeFeatures; + var fakeFrameSync; var fakeGroups; var fakeRootThread; var fakeSettings; @@ -74,8 +74,9 @@ describe('WidgetController', function () { unloadAnnotations: sandbox.spy(), }; - fakeCrossFrame = { - call: sinon.stub(), + fakeFrameSync = { + focusAnnotations: sinon.stub(), + scrollToAnnotation: sinon.stub(), frames: [], }; @@ -113,9 +114,9 @@ describe('WidgetController', function () { }; $provide.value('annotationMapper', fakeAnnotationMapper); - $provide.value('crossframe', fakeCrossFrame); $provide.value('drafts', fakeDrafts); $provide.value('features', fakeFeatures); + $provide.value('frameSync', fakeFrameSync); $provide.value('rootThread', fakeRootThread); $provide.value('store', fakeStore); $provide.value('streamer', fakeStreamer); @@ -124,6 +125,7 @@ describe('WidgetController', function () { $provide.value('settings', fakeSettings); })); + beforeEach(angular.mock.inject(function ($controller, _annotationUI_, _$rootScope_) { $rootScope = _$rootScope_; $scope = $rootScope.$new(); @@ -142,11 +144,11 @@ describe('WidgetController', function () { // before reloading annotations for each currently-connected client annotationUI.addAnnotations([{id: '123'}]); var uri1 = 'http://example.com/page-a'; - fakeCrossFrame.frames.push({uri: uri1, searchUris: [uri1]}); + fakeFrameSync.frames.push({uri: uri1, searchUris: [uri1]}); $scope.$digest(); fakeAnnotationMapper.unloadAnnotations = sandbox.spy(); var uri2 = 'http://example.com/page-b'; - fakeCrossFrame.frames.push({uri: uri2, searchUris: [uri2]}); + fakeFrameSync.frames.push({uri: uri2, searchUris: [uri2]}); $scope.$digest(); assert.calledWith(fakeAnnotationMapper.unloadAnnotations, annotationUI.getState().annotations); @@ -154,7 +156,7 @@ describe('WidgetController', function () { it('loads all annotations for a frame', function () { var uri = 'http://example.com'; - fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]}); + fakeFrameSync.frames.push({uri: uri, searchUris: [uri]}); $scope.$digest(); var loadSpy = fakeAnnotationMapper.loadAnnotations; assert.calledWith(loadSpy, [sinon.match({id: uri + '123'})]); @@ -164,7 +166,7 @@ describe('WidgetController', function () { it('loads all annotations for a frame with multiple urls', function () { var uri = 'http://example.com/test.pdf'; var fingerprint = 'urn:x-pdf:fingerprint'; - fakeCrossFrame.frames.push({uri: uri, searchUris: [uri, fingerprint]}); + fakeFrameSync.frames.push({uri: uri, searchUris: [uri, fingerprint]}); $scope.$digest(); var loadSpy = fakeAnnotationMapper.loadAnnotations; assert.calledWith(loadSpy, [sinon.match({id: uri + '123'})]); @@ -175,7 +177,7 @@ describe('WidgetController', function () { it('loads all annotations for all frames', function () { var uris = ['http://example.com', 'http://foobar.com']; - fakeCrossFrame.frames = uris.map(function (uri) { + fakeFrameSync.frames = uris.map(function (uri) { return {uri: uri, searchUris: [uri]}; }); $scope.$digest(); @@ -191,7 +193,7 @@ describe('WidgetController', function () { var id = uri + '123'; beforeEach(function () { - fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}]; + fakeFrameSync.frames = [{uri: uri, searchUris: [uri]}]; annotationUI.selectAnnotations([id]); $scope.$digest(); }); @@ -221,7 +223,7 @@ describe('WidgetController', function () { var uri = 'http://example.com'; beforeEach(function () { - fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}]; + fakeFrameSync.frames = [{uri: uri, searchUris: [uri]}]; fakeGroups.focused = function () { return { id: 'a-group' }; }; $scope.$digest(); }); @@ -244,7 +246,7 @@ describe('WidgetController', function () { var id = uri + 'does-not-exist'; beforeEach(function () { - fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}]; + fakeFrameSync.frames = [{uri: uri, searchUris: [uri]}]; annotationUI.selectAnnotations([id]); fakeGroups.focused = function () { return { id: 'private-group' }; }; $scope.$digest(); @@ -262,7 +264,7 @@ describe('WidgetController', function () { it('focuses and scrolls to the annotation if already selected', function () { var uri = 'http://example.com'; annotationUI.selectAnnotations(['123']); - fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]}); + fakeFrameSync.frames.push({uri: uri, searchUris: [uri]}); var annot = { $$tag: 'atag', id: '123', @@ -270,25 +272,24 @@ describe('WidgetController', function () { annotationUI.addAnnotations([annot]); $scope.$digest(); $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, [{tag: 'atag'}]); - assert.calledWith(fakeCrossFrame.call, 'focusAnnotations', ['atag']); - assert.calledWith(fakeCrossFrame.call, 'scrollToAnnotation', 'atag'); + assert.calledWith(fakeFrameSync.focusAnnotations, ['atag']); + assert.calledWith(fakeFrameSync.scrollToAnnotation, 'atag'); }); }); describe('when the focused group changes', function () { it('should load annotations for the new group', function () { var uri = 'http://example.com'; - annotationUI.addAnnotations([{id: '123'}]); annotationUI.addAnnotations = sinon.stub(); - fakeDrafts.unsaved.returns([{id: uri + '123'}, {id: uri + '456'}]); - - fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]}); + fakeFrameSync.frames.push({uri: uri, searchUris: [uri]}); var loadSpy = fakeAnnotationMapper.loadAnnotations; $scope.$broadcast(events.GROUP_FOCUSED); - assert.calledWith(fakeAnnotationMapper.unloadAnnotations, [{id: '123'}]); + + assert.calledWith(fakeAnnotationMapper.unloadAnnotations, + [sinon.match({id: '123'})]); $scope.$digest(); assert.calledWith(loadSpy, [sinon.match({id: uri + '123'})]); assert.calledWith(loadSpy, [sinon.match({id: uri + '456'})]); @@ -299,7 +300,7 @@ describe('WidgetController', function () { beforeEach(function () { // The document has finished loading. - fakeCrossFrame.frames = [ + fakeFrameSync.frames = [ { uri: 'http://www.example.com', searchUris: [], @@ -335,7 +336,7 @@ describe('WidgetController', function () { // There is a selection but the selected annotation isn't available. annotationUI.selectAnnotations(['missing']); // The document hasn't finished loading. - fakeCrossFrame.frames = []; + fakeFrameSync.frames = []; $scope.$digest(); assert.isFalse($scope.selectedAnnotationUnavailable()); diff --git a/h/static/scripts/widget-controller.js b/h/static/scripts/widget-controller.js index 0b1790eb0db..19e2b943e31 100644 --- a/h/static/scripts/widget-controller.js +++ b/h/static/scripts/widget-controller.js @@ -34,8 +34,8 @@ function groupIDFromSelection(selection, results) { // @ngInject module.exports = function WidgetController( - $scope, annotationUI, crossframe, annotationMapper, drafts, - features, groups, rootThread, settings, streamer, streamFilter, store + $scope, annotationUI, annotationMapper, drafts, features, frameSync, groups, + rootThread, settings, streamer, streamFilter, store ) { function thread() { return rootThread.thread(annotationUI.getState()); @@ -66,14 +66,14 @@ module.exports = function WidgetController( if (annotation) { highlights = [annotation.$$tag]; } - crossframe.call('focusAnnotations', highlights); + frameSync.focusAnnotations(highlights); } function scrollToAnnotation(annotation) { if (!annotation) { return; } - crossframe.call('scrollToAnnotation', annotation.$$tag); + frameSync.scrollToAnnotation(annotation.$$tag); } /** Returns the annotation type - note or annotation of the first annotation @@ -160,7 +160,7 @@ module.exports = function WidgetController( } function isLoading() { - if (!crossframe.frames.some(function (frame) { return frame.uri; })) { + if (!frameSync.frames.some(function (frame) { return frame.uri; })) { // The document's URL isn't known so the document must still be loading. return true; } @@ -262,12 +262,12 @@ module.exports = function WidgetController( return; } annotationUI.clearSelectedAnnotations(); - loadAnnotations(crossframe.frames); + loadAnnotations(frameSync.frames); }); // Watch anything that may require us to reload annotations. $scope.$watchCollection(function () { - return crossframe.frames; + return frameSync.frames; }, loadAnnotations); $scope.setCollapsed = function (id, collapsed) {