Fix: Only execute viewer setup if not already setup (#955)

box · Mar 12, 2019 · 8c2b193 · 8c2b193
1 parent 5616b74
commit 8c2b193
Show file tree

Hide file tree

Showing 37 changed files with 186 additions and 75 deletions.
diff --git a/src/index.html b/src/index.html
@@ -92,7 +92,7 @@
                 }
 
                 /* global Box */
-                var preview = new Box.Preview();
+                preview = new Box.Preview();
 
                 var previewOptions = options || {
                     enableThumbnailsSidebar: true,

diff --git a/src/lib/Preview.js b/src/lib/Preview.js
@@ -1161,6 +1161,9 @@ class Preview extends EventEmitter {
         // Add listeners for viewer events
         this.attachViewerListeners();
 
+        // Setup the viewer DOM
+        this.viewer.setup();
+
         // Load the representation into the viewer
         this.viewer.load();
 
@@ -1472,6 +1475,9 @@ class Preview extends EventEmitter {
         // Instantiate the error viewer
         this.viewer = this.getErrorViewer();
 
+        // Setup error viewer
+        this.viewer.setup();
+
         // Add listeners for viewer events
         this.attachViewerListeners();
 

diff --git a/src/lib/__tests__/Preview-test.js b/src/lib/__tests__/Preview-test.js
@@ -1577,7 +1577,8 @@ describe('lib/Preview', () => {
             stubs.viewer = {
                 load: sandbox.stub(),
                 addListener: sandbox.stub(),
-                getName: sandbox.stub()
+                getName: sandbox.stub(),
+                setup: sandbox.stub()
             };
 
             /* eslint-disable require-jsdoc */
@@ -2163,7 +2164,8 @@ describe('lib/Preview', () => {
     describe('triggerError()', () => {
         const ErrorViewer = {
             load: sandbox.stub(),
-            addListener: sandbox.stub()
+            addListener: sandbox.stub(),
+            setup: sandbox.stub()
         };
 
         beforeEach(() => {

diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js
@@ -18,6 +18,7 @@ import {
     replacePlaceholders
 } from '../util';
 import {
+    ANNOTATOR_EVENT,
     CLASS_BOX_PREVIEW_MOBILE,
     CLASS_HIDDEN,
     FILE_OPTION_START,
@@ -26,10 +27,9 @@ import {
     SELECTOR_BOX_PREVIEW_CONTENT,
     SELECTOR_BOX_PREVIEW_CRAWLER_WRAPPER,
     SELECTOR_BOX_PREVIEW_ICON,
-    STATUS_SUCCESS,
-    STATUS_VIEWABLE,
     SELECTOR_BOX_PREVIEW,
-    ANNOTATOR_EVENT
+    STATUS_SUCCESS,
+    STATUS_VIEWABLE
 } from '../constants';
 import { getIconFromExtension, getIconFromName } from '../icons/icons';
 import { VIEWER_EVENT, ERROR_CODE, LOAD_METRIC, DOWNLOAD_REACHABILITY_METRICS } from '../events';
@@ -114,6 +114,9 @@ class BaseViewer extends EventEmitter {
     /** @property {HTMLElement} - The .bp-content which is the container for the viewer's content */
     containerEl;
 
+    /** @property {boolean} - Stores whether the Viewer has been setup yet. */
+    isSetup = false;
+
     /**
      * [constructor]
      *
@@ -156,6 +159,10 @@ class BaseViewer extends EventEmitter {
      * @return {void}
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         if (this.options.file) {
             const fileExt = this.options.file.extension;
             this.fileLoadingIcon = getIconFromExtension(fileExt);
@@ -193,6 +200,8 @@ class BaseViewer extends EventEmitter {
                 this.annotatorPromiseResolver = resolve;
             });
         }
+
+        this.isSetup = true;
     }
 
     /**

diff --git a/src/lib/viewers/box3d/Box3DViewer.js b/src/lib/viewers/box3d/Box3DViewer.js
@@ -62,6 +62,10 @@ class Box3DViewer extends BaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
 
@@ -179,7 +183,6 @@ class Box3DViewer extends BaseViewer {
      * @return {Promise} to load assets and representation
      */
     load() {
-        this.setup();
         super.load();
         return Promise.all([this.loadAssets(JS), this.getRepStatus().getPromise()])
             .then(this.postLoad)

diff --git a/src/lib/viewers/box3d/__tests__/Box3DViewer-test.js b/src/lib/viewers/box3d/__tests__/Box3DViewer-test.js
@@ -358,7 +358,6 @@ describe('lib/viewers/box3d/Box3DViewer', () => {
         });
 
         it('should call renderer.load()', () => {
-            Object.defineProperty(BaseViewer.prototype, 'setup', { value: sandbox.mock() });
             box3d.containerEl = document.querySelector(SELECTOR_BOX_PREVIEW_CONTENT);
             Object.defineProperty(BaseViewer.prototype, 'load', { value: sandbox.mock() });
             sandbox.stub(box3d, 'loadAssets').returns(Promise.resolve());

diff --git a/src/lib/viewers/box3d/image360/Image360Viewer.js b/src/lib/viewers/box3d/image360/Image360Viewer.js
@@ -11,6 +11,10 @@ class Image360Viewer extends Box3DViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
 

diff --git a/src/lib/viewers/box3d/model3d/Model3DViewer.js b/src/lib/viewers/box3d/model3d/Model3DViewer.js
@@ -59,6 +59,10 @@ class Model3DViewer extends Box3DViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() first to set up common layout
         super.setup();
 

diff --git a/src/lib/viewers/box3d/video360/Video360Viewer.js b/src/lib/viewers/box3d/video360/Video360Viewer.js
@@ -46,6 +46,10 @@ class Video360Viewer extends DashViewer {
 
     /** @inheritdoc */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
 

diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js
@@ -102,6 +102,10 @@ class DocBaseViewer extends BaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
 
@@ -188,6 +192,8 @@ class DocBaseViewer extends BaseViewer {
 
         if (this.thumbnailsSidebar) {
             this.thumbnailsSidebar.destroy();
+            this.thumbnailsSidebarEl.remove();
+            this.thumbnailsSidebarEl = null;
         }
 
         super.destroy();
@@ -305,7 +311,6 @@ class DocBaseViewer extends BaseViewer {
      * @return {Promise} Promise to resolve assets
      */
     load() {
-        this.setup();
         super.load();
         this.showPreload();
 

diff --git a/src/lib/viewers/doc/DocumentViewer.js b/src/lib/viewers/doc/DocumentViewer.js
@@ -12,6 +12,10 @@ class DocumentViewer extends DocBaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
         this.docEl.classList.add('bp-doc-document');

diff --git a/src/lib/viewers/doc/PresentationViewer.js b/src/lib/viewers/doc/PresentationViewer.js
@@ -30,6 +30,10 @@ class PresentationViewer extends DocBaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
         this.docEl.classList.add('bp-doc-presentation');

diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
@@ -506,7 +506,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
 
             return docBase.load().then(() => {
                 expect(docBase.loadAssets).to.be.called;
-                expect(docBase.setup).to.be.called;
+                expect(docBase.setup).not.to.be.called;
                 expect(docBase.createContentUrlWithAuthParams).to.be.calledWith('foo');
                 expect(docBase.handleAssetAndRepLoad).to.be.called;
             });

diff --git a/src/lib/viewers/error/PreviewErrorViewer.js b/src/lib/viewers/error/PreviewErrorViewer.js
@@ -21,6 +21,10 @@ class PreviewErrorViewer extends BaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() first to set up common layout
         super.setup();
 
@@ -65,8 +69,6 @@ class PreviewErrorViewer extends BaseViewer {
      * @return {void}
      */
     load(err) {
-        this.setup();
-
         const error =
             err instanceof PreviewError
                 ? err

diff --git a/src/lib/viewers/error/__tests__/PreviewErrorViewer-test.js b/src/lib/viewers/error/__tests__/PreviewErrorViewer-test.js
@@ -29,6 +29,7 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
         });
         Object.defineProperty(BaseViewer.prototype, 'setup', { value: sandbox.mock() });
         error.containerEl = containerEl;
+        error.setup();
     });
 
     afterEach(() => {
@@ -45,7 +46,6 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
     describe('setup()', () => {
         it('should set appropriate properties', () => {
-            error.setup();
             expect(error.infoEl.classList.contains('bp-error')).to.be.true;
             expect(error.iconEl instanceof HTMLElement).to.be.true;
             expect(error.iconEl.parentNode).to.equal(error.infoEl);
@@ -55,12 +55,7 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
     });
 
     describe('load()', () => {
-        [
-            ['zip', true],
-            ['tgz', true],
-            ['flv', true],
-            ['blah', false]
-        ].forEach((testCase) => {
+        [['zip', true], ['tgz', true], ['flv', true], ['blah', false]].forEach((testCase) => {
             it('should set appropriate icon', () => {
                 const getIconFromExtensionStub = sandbox.stub(icons, 'getIconFromExtension');
                 const getIconFromNameStub = sandbox.stub(icons, 'getIconFromName');
@@ -132,11 +127,9 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
             error.load(err);
 
-            expect(error.emit).to.be.calledWith(
-                VIEWER_EVENT.load, {
-                    error: 'this is bad'
-                }
-            );
+            expect(error.emit).to.be.calledWith(VIEWER_EVENT.load, {
+                error: 'this is bad'
+            });
         });
 
         it('should broadcast the display message if there is no error message', () => {
@@ -145,32 +138,31 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
             error.load(err);
 
-            expect(error.emit).to.be.calledWith(
-                VIEWER_EVENT.load, {
-                    error: 'display message!'
-                }
-            );
+            expect(error.emit).to.be.calledWith(VIEWER_EVENT.load, {
+                error: 'display message!'
+            });
         });
 
         it('should filter out access tokens before broadcasting', () => {
             sandbox.stub(error, 'emit');
-            const err = new PreviewError('some_code', 'display', {},
+            const err = new PreviewError(
+                'some_code',
+                'display',
+                {},
                 'Unexpected server response (0) while retrieving PDF "www.box.com?access_token=blah&test=okay"'
             );
 
             error.load(err);
 
-            expect(error.emit).to.be.calledWith(
-                VIEWER_EVENT.load, {
-                    error: 'Unexpected server response (0) while retrieving PDF "www.box.com?access_token=[FILTERED]&test=okay"'
-                }
-            );
+            expect(error.emit).to.be.calledWith(VIEWER_EVENT.load, {
+                error:
+                    'Unexpected server response (0) while retrieving PDF "www.box.com?access_token=[FILTERED]&test=okay"'
+            });
         });
     });
 
     describe('addLinkButton()', () => {
         it('should add a link button with the appropriate message and URL', () => {
-            error.setup();
             error.addLinkButton('test', 'someUrl');
             const linkBtnEl = error.infoEl.querySelector('a');
 
@@ -183,7 +175,6 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
     describe('addDownloadButton()', () => {
         it('should add a download button and attach a download click handler', () => {
-            error.setup();
             sandbox.stub(error, 'download');
 
             error.addDownloadButton();
@@ -201,7 +192,6 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
     describe('download()', () => {
         it('should emit download', () => {
-            error.setup();
             sandbox.stub(error, 'emit');
             error.download();
             expect(error.emit).to.be.calledWith(VIEWER_EVENT.download);
@@ -210,7 +200,6 @@ describe('lib/viewers/error/PreviewErrorViewer', () => {
 
     describe('destroy()', () => {
         it('should remove download button click handler', () => {
-            error.setup();
             sandbox.stub(error, 'download');
 
             error.addDownloadButton();

diff --git a/src/lib/viewers/iframe/IFrameViewer.js b/src/lib/viewers/iframe/IFrameViewer.js
@@ -6,6 +6,10 @@ class IFrameViewer extends BaseViewer {
      * @inheritdoc
      */
     setup() {
+        if (this.isSetup) {
+            return;
+        }
+
         // Call super() to set up common layout
         super.setup();
 
@@ -23,8 +27,6 @@ class IFrameViewer extends BaseViewer {
      * @return {void}
      */
     load() {
-        this.setup();
-
         let src = '';
         const { file, sharedLink = '', appHost } = this.options;
         const { extension } = file;

diff --git a/src/lib/viewers/iframe/__tests__/IFrameViewer-test.js b/src/lib/viewers/iframe/__tests__/IFrameViewer-test.js
@@ -25,6 +25,7 @@ describe('lib/viewers/iframe/IFrameViewer', () => {
 
         Object.defineProperty(BaseViewer.prototype, 'setup', { value: sandbox.mock() });
         iframe.containerEl = containerEl;
+        iframe.setup();
     });
 
     afterEach(() => {
@@ -41,7 +42,6 @@ describe('lib/viewers/iframe/IFrameViewer', () => {
 
     describe('setup()', () => {
         it('should setup iframe element and load timeout', () => {
-            iframe.setup();
             expect(iframe.iframeEl).to.be.instanceof(HTMLElement);
             expect(iframe.iframeEl).to.have.attribute('width', '100%');
             expect(iframe.iframeEl).to.have.attribute('height', '100%');