Fix: construct annotator when BoxAnnotations loads

src/lib/viewers/BaseViewer.js

@@ -138,8 +138,10 @@ class BaseViewer extends EventEmitter {
         this.mobileZoomChangeHandler = this.mobileZoomChangeHandler.bind(this);
         this.mobileZoomEndHandler = this.mobileZoomEndHandler.bind(this);
         this.handleAnnotatorEvents = this.handleAnnotatorEvents.bind(this);
-        this.annotationsLoadHandler = this.annotationsLoadHandler.bind(this);
+        this.createAnnotator = this.createAnnotator.bind(this);
         this.viewerLoadHandler = this.viewerLoadHandler.bind(this);
+        this.initAnnotations = this.initAnnotations.bind(this);
+        this.loadBoxAnnotations = this.loadBoxAnnotations.bind(this);
     }
 
     /**
@@ -176,11 +178,12 @@ class BaseViewer extends EventEmitter {
             this.containerEl.classList.add(CLASS_BOX_PREVIEW_MOBILE);
         }
 
-        // Attempts to load annotations assets and initializes annotations if
-        // the assets are available, the showAnnotations flag is true, and the
-        // expiring embed is not a shared link
+        // Creates a promise that the annotator will be constructed if annotations are
+        // enabled and the expiring embed is not a shared link
         if (this.areAnnotationsEnabled() && !this.options.sharedLink) {
-            this.loadAnnotator();
+            this.annotatorPromise = new Promise((resolve) => {
+                this.annotatorPromiseResolver = resolve;
+            });
         }
     }
 
@@ -232,6 +235,8 @@ class BaseViewer extends EventEmitter {
         }
 
         this.destroyed = true;
+        this.annotatorPromise = null;
+        this.annotatorPromiseResolver = null;
         this.emit('destroy');
     }
 
@@ -482,8 +487,9 @@ class BaseViewer extends EventEmitter {
             this.scale = event.scale;
         }
 
-        if (this.annotationsLoadPromise) {
-            this.annotationsLoadPromise.then(this.annotationsLoadHandler).catch(() => {});
+        // Ensures that the annotator has been created first
+        if (this.annotatorPromise) {
+            this.annotatorPromise.then(this.initAnnotations);
         }
     }
 
@@ -826,27 +832,34 @@ class BaseViewer extends EventEmitter {
     //--------------------------------------------------------------------------
 
     /**
-     * Loads the appropriate annotator and loads the file's annotations
+     * Loads the BoxAnnotations static assets
      *
      * @protected
-     * @return {void}
+     * @return {Promise} promise that is resolved when the assets are loaded
      */
-    loadAnnotator() {
-        // Auto-resolves promise if BoxAnnotations is passed in as a Preview option
-        this.annotationsLoadPromise =
-            window.BoxAnnotations && this.options.boxAnnotations instanceof window.BoxAnnotations
-                ? Promise.resolve()
-                : this.loadAssets([ANNOTATIONS_JS], [ANNOTATIONS_CSS], false);
+    loadBoxAnnotations() {
+        if (
+            !this.options.showAnnotations ||
+            (window.BoxAnnotations && this.options.boxAnnotations instanceof window.BoxAnnotations)
+        ) {
+            return Promise.resolve();
+        }
+
+        return this.loadAssets([ANNOTATIONS_JS], [ANNOTATIONS_CSS], false);
     }
 
     /**
-     * Fetches the Box Annotations library. Creates an instance of BoxAnnotations
-     * if one isn't passed in to the preview options
+     * Creates an instance of BoxAnnotations if one isn't passed in to the preview options
+     * and instantiates the appropriate annotator
      *
      * @protected
      * @return {void}
      */
-    annotationsLoadHandler() {
+    createAnnotator() {
+        if (!this.options.showAnnotations) {
+            return;
+        }
+
         // Set viewer-specific annotation options
         const viewerOptions = {};
         viewerOptions[this.options.viewer.NAME] = this.viewerConfig;
@@ -860,8 +873,18 @@ class BaseViewer extends EventEmitter {
         const boxAnnotations = this.options.boxAnnotations || new global.BoxAnnotations(viewerOptions);
         this.annotatorConf = boxAnnotations.determineAnnotator(this.options, this.viewerConfig);
 
-        if (this.annotatorConf) {
-            this.initAnnotations();
+        if (!this.annotatorConf) {
+            return;
+        }
+
+        const annotatorOptions = this.createAnnotatorOptions({
+            annotator: this.annotatorConf,
+            modeButtons: ANNOTATION_BUTTONS
+        });
+        this.annotator = new this.annotatorConf.CONSTRUCTOR(annotatorOptions);
+
+        if (this.annotatorPromiseResolver) {
+            this.annotatorPromiseResolver();
         }
     }
 
@@ -872,12 +895,6 @@ class BaseViewer extends EventEmitter {
      * @return {void}
      */
     initAnnotations() {
-        // Construct and init annotator
-        const annotatorOptions = this.createAnnotatorOptions({
-            annotator: this.annotatorConf,
-            modeButtons: ANNOTATION_BUTTONS
-        });
-        this.annotator = new this.annotatorConf.CONSTRUCTOR(annotatorOptions);
         this.annotator.init(this.scale);
 
         // Once the annotator instance has been created, emit it so that clients can attach their events.

src/lib/viewers/__tests__/BaseViewer-test.js

@@ -59,7 +59,7 @@ describe('lib/viewers/BaseViewer', () => {
             sandbox.stub(base, 'addCommonListeners');
             sandbox.stub(base, 'areAnnotationsEnabled').returns(true);
             sandbox.stub(base, 'finishLoadingSetup');
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             base.options.showAnnotations = true;
 
             base.setup();
@@ -79,12 +79,13 @@ describe('lib/viewers/BaseViewer', () => {
             expect(base.addCommonListeners).to.be.called;
             expect(getIconFromExtensionStub).to.be.called;
             expect(base.loadTimeout).to.be.a('number');
-            expect(base.loadAnnotator).to.be.called;
+            expect(base.annotatorPromise).to.not.be.undefined;
+            expect(base.annotatorPromiseResolver).to.not.be.undefined;
         });
 
         it('should add a mobile class to the container if on mobile', () => {
             base.isMobile = true;
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             sandbox.stub(base, 'finishLoadingSetup');
             sandbox.stub(base, 'areAnnotationsEnabled').returns(true);
 
@@ -97,25 +98,25 @@ describe('lib/viewers/BaseViewer', () => {
         it('should not load annotations assets if global preview showAnnotations option is false', () => {
             sandbox.stub(base, 'addCommonListeners');
             sandbox.stub(base, 'areAnnotationsEnabled').returns(false);
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             sandbox.stub(base, 'finishLoadingSetup');
             base.options.showAnnotations = false;
 
             base.setup();
 
-            expect(base.loadAnnotator).to.not.be.called;
+            expect(base.loadBoxAnnotations).to.not.be.called;
         });
 
         it('should not load annotations assets if expiring embed is a shared link', () => {
             sandbox.stub(base, 'addCommonListeners');
             sandbox.stub(base, 'areAnnotationsEnabled').returns(true);
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             sandbox.stub(base, 'finishLoadingSetup');
             base.options.sharedLink = 'url';
 
             base.setup();
 
-            expect(base.loadAnnotator).to.not.be.called;
+            expect(base.loadBoxAnnotations).to.not.be.called;
         });
     });
 
@@ -453,8 +454,6 @@ describe('lib/viewers/BaseViewer', () => {
 
     describe('viewerLoadHandler()', () => {
         beforeEach(() => {
-            base.annotationsLoadPromise = Promise.resolve();
-            stubs.annotationsLoadHandler = sandbox.stub(base, 'annotationsLoadHandler');
             base.options.representation = {
                 content: {
                     url_template: 'dl.boxcloud.com'
@@ -463,6 +462,7 @@ describe('lib/viewers/BaseViewer', () => {
             stubs.getDownloadNotificationToShow = sandbox
                 .stub(DownloadReachability, 'getDownloadNotificationToShow')
                 .returns(undefined);
+            sandbox.stub(base, 'initAnnotations');
         });
 
         it('should show the notification if downloads are degraded and we have not shown the notification yet', () => {
@@ -485,27 +485,19 @@ describe('lib/viewers/BaseViewer', () => {
             expect(base.scale).to.equal(1.5);
         });
 
-        it('should handle the annotations load promise', () => {
-            base.viewerLoadHandler({ scale: 1.5 });
-            return base.annotationsLoadPromise.then(() => {
-                expect(base.annotationsLoadHandler).to.be.called;
+        it('should show annotations if annotatorPromise exists', (done) => {
+            base.annotatorPromise = new Promise((resolve) => {
+                resolve();
+                done();
             });
+            base.viewerLoadHandler({ scale: 1.5 });
+            expect(base.initAnnotations).to.be.called;
         });
 
-        it('should handle the annotations load promise', () => {
-            stubs.annotationsLoadHandler.callsFake(() => {
-                throw new Error('message');
-            });
-
+        it('should show annotations if annotatorPromise does not exist', () => {
+            base.annotatorPromise = null;
             base.viewerLoadHandler({ scale: 1.5 });
-            return base.annotationsLoadPromise
-                .then(() => {
-                    sinon.assert.failException;
-                })
-                .catch((error) => {
-                    expect(error).to.be.an('error');
-                    expect(error.message).to.equal('message');
-                });
+            expect(base.initAnnotations).to.not.be.called;
         });
     });
 
@@ -580,7 +572,7 @@ describe('lib/viewers/BaseViewer', () => {
 
         it('should cleanup the base viewer', () => {
             sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             sandbox.stub(base, 'finishLoadingSetup');
             base.setup();
 
@@ -667,7 +659,7 @@ describe('lib/viewers/BaseViewer', () => {
             });
             sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
             sandbox.stub(base, 'areAnnotationsEnabled').returns(false);
-            sandbox.stub(base, 'loadAnnotator');
+            sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
             sandbox.stub(base, 'finishLoadingSetup');
             base.setup();
             event = {
@@ -999,7 +991,7 @@ describe('lib/viewers/BaseViewer', () => {
         });
     });
 
-    describe('loadAnnotator()', () => {
+    describe('loadBoxAnnotations()', () => {
         const conf = {
             annotationsEnabled: true,
             types: {
@@ -1009,61 +1001,67 @@ describe('lib/viewers/BaseViewer', () => {
         };
 
         beforeEach(() => {
-            sandbox.stub(base, 'loadAssets');
+            sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
             window.BoxAnnotations = function BoxAnnotations() {
                 this.determineAnnotator = sandbox.stub().returns(conf);
             };
         });
 
-        it('should resolve the promise if a BoxAnnotations instance was passed into Preview', (done) => {
+        it('should resolve the promise if a BoxAnnotations instance was passed into Preview', () => {
             base.options.boxAnnotations = new window.BoxAnnotations({});
 
-            base.loadAnnotator();
+            base.loadBoxAnnotations();
             expect(base.loadAssets).to.not.be.calledWith(['annotations.js']);
-            base.annotationsLoadPromise.then(() => done());
         });
 
-        it('should load the annotations assets', () => {
-            base.loadAnnotator();
+        it('should load the annotations assets if showAnnotations option is true', () => {
+            base.options.showAnnotations = true;
+            base.loadBoxAnnotations();
             expect(base.loadAssets).to.be.calledWith(['annotations.js'], ['annotations.css'], false);
         });
+
+        it('should not load the annotations assets if showAnnotations option is false', () => {
+            base.options.showAnnotations = false;
+            base.loadBoxAnnotations();
+            expect(base.loadAssets).to.not.be.called;
+        });
     });
 
-    describe('annotationsLoadHandler()', () => {
+    describe('createAnnotator()', () => {
+        const annotatorMock = {};
         const conf = {
             annotationsEnabled: true,
             types: {
                 point: true,
                 highlight: false
-            }
+            },
+            CONSTRUCTOR: sandbox.stub().returns(annotatorMock)
         };
 
         beforeEach(() => {
             base.options.viewer = { NAME: 'viewerName' };
+            base.options.location = { locale: 'en-US' };
+            base.options.showAnnotations = true;
             window.BoxAnnotations = function BoxAnnotations() {
                 this.determineAnnotator = sandbox.stub().returns(conf);
             };
 
             sandbox.stub(base, 'initAnnotations');
         });
 
-        it('should determine the annotator', () => {
-            base.annotationsLoadHandler();
+        it('should determine and instantiate the annotator', () => {
+            base.createAnnotator();
             expect(base.annotatorConf).to.equal(conf);
+            expect(base.annotator).to.equal(annotatorMock);
         });
 
         it('should not instantiate an instance of BoxAnnotations if one is already passed in', () => {
             base.options.boxAnnotations = {
-                determineAnnotator: sandbox.stub()
+                determineAnnotator: sandbox.stub().returns(conf)
             };
-            base.annotationsLoadHandler();
+            base.createAnnotator();
             expect(base.options.boxAnnotations.determineAnnotator).to.be.called;
         });
-
-        it('should init annotations if a conf is present', () => {
-            base.annotationsLoadHandler();
-            expect(base.initAnnotations).to.be.called;
-        });
     });
 
     describe('initAnnotations()', () => {

src/lib/viewers/doc/DocBaseViewer.js

@@ -279,6 +279,8 @@ class DocBaseViewer extends BaseViewer {
 
         return Promise.all([this.loadAssets(JS, CSS), this.getRepStatus().getPromise()])
             .then(this.handleAssetAndRepLoad)
+            .then(this.loadBoxAnnotations)
+            .then(this.createAnnotator)
             .catch(this.handleAssetError);
     }
 

src/lib/viewers/doc/__tests__/DocBaseViewer-test.js

@@ -410,12 +410,14 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
             sandbox.stub(docBase, 'handleAssetAndRepLoad');
             sandbox.stub(docBase, 'getRepStatus').returns({ getPromise: () => Promise.resolve() });
             sandbox.stub(docBase, 'loadAssets');
+            sandbox.stub(docBase, 'loadBoxAnnotations');
 
             return docBase.load().then(() => {
                 expect(docBase.loadAssets).to.be.called;
                 expect(docBase.setup).to.be.called;
                 expect(docBase.createContentUrlWithAuthParams).to.be.calledWith('foo');
                 expect(docBase.handleAssetAndRepLoad).to.be.called;
+                expect(docBase.loadBoxAnnotations).to.be.called;
             });
         });
     });

src/lib/viewers/image/ImageViewer.js

@@ -62,6 +62,8 @@ class ImageViewer extends ImageBaseViewer {
                 this.startLoadTimer();
                 this.imageEl.src = downloadUrl;
             })
+            .then(this.loadBoxAnnotations)
+            .then(this.createAnnotator)
             .catch(this.handleAssetError);
     }