Convert Notebook into a react component

Notebook class creates the container element and instantiates the preact
component NotebookModal.

NotebookModal listens to the openNotebook event sent through the
eventBus and hides and shows the modal accordingly.

The creation of the iframe is a costly operation: it involves several
backend calls and the rendering of it in the DOM. Therefore, we delay
the creation of the iframe element until the NotebookModal listens for
the `openNotebook` event.

The useEffect's cleanup of the NotebookModal componente has significant
effects because it resets the overflow CSS property of document.body.

As pointed by @robertknight, `render(null, container)` destroys the
component triggering the useEffect's cleanup function.

src/annotator/components/NotebookModal.js

+import { useEffect, useRef, useState } from 'preact/hooks';
+import classnames from 'classnames';
+
+import { createSidebarConfig } from '../config/sidebar';
+
+// FIXME: use the button from the frontend shared package once this is stable.
+import Button from '../../sidebar/components/Button';
+
+/**
+ * @typedef NotebookIframeProps
+ * @prop {Record<string, any>} config
+ * @prop {string} groupId
+ */
+
+/**
+ * Create the iframe that will load the notebook application.
+ *
+ * @param {NotebookIframeProps} props
+ */
+function NotebookIframe({ config, groupId }) {
+  const notebookConfig = createSidebarConfig(config);
+  // Explicity set the "focused" group
+  notebookConfig.group = groupId;
+  const configParam = encodeURIComponent(JSON.stringify(notebookConfig));
+  const notebookAppSrc = `${config.notebookAppUrl}#config=${configParam}`;
+
+  return (
+    <iframe
+      key={groupId} // force re-rendering on group change
+      title={'Hypothesis annotation notebook'}
+      className="Notebook__iframe"
+      // Enable media in annotations to be shown fullscreen
+      allowFullScreen
+      src={notebookAppSrc}
+    />
+  );
+}
+
+/**
+ * @typedef NotebookModalProps
+ * @prop {import('../util/emitter').EventBus} eventBus
+ * @prop {Record<string, any>} config
+ */
+
+/**
+ * Create a modal component that hosts (1) the notebook iframe and (2) a button to close the modal.
+ *
+ * @param {NotebookModalProps} props
+ */
+export default function NotebookModal({ eventBus, config }) {
+  const [isHidden, setIsHidden] = useState(true);
+  const [groupId, setGroupId] = useState(/** @type {string|null} */ (null));
+  const originalDocumentOverflowStyle = useRef('');
+  const emitter = useRef(
+    /** @type {ReturnType<eventBus['createEmitter']>|null} */ (null)
+  );
+
+  // Stores the original overflow CSS property of document.body and reset it
+  // when the component is destroyed
+  useEffect(() => {
+    originalDocumentOverflowStyle.current = document.body.style.overflow;
+
+    return () => {
+      document.body.style.overflow = originalDocumentOverflowStyle.current;
+    };
+  }, []);
+
+  // The overflow CSS property is set to hidden to prevent scrolling of the host page,
+  // while the notebook modal is open. It is restored when the modal is closed.
+  useEffect(() => {
+    if (isHidden) {
+      document.body.style.overflow = originalDocumentOverflowStyle.current;
+    } else {
+      document.body.style.overflow = 'hidden';
+    }
+  }, [isHidden]);
+
+  useEffect(() => {
+    emitter.current = eventBus.createEmitter();
+    emitter.current.subscribe('openNotebook', (
+      /** @type {string} */ groupId
+    ) => {
+      setIsHidden(false);
+      setGroupId(groupId);
+    });
+
+    return () => {
+      emitter.current.destroy();
+    };
+  }, [eventBus]);
+
+  const onClose = () => {
+    setIsHidden(true);
+    emitter.current.publish('closeNotebook');
+  };
+
+  return (
+    <div className={classnames('Notebook__outer', { 'is-hidden': isHidden })}>
+      <div className="Notebook__inner">
+        <Button
+          icon="cancel"
+          className="Notebook__close-button"
+          buttonText="Close"
+          title="Close the Notebook"
+          onClick={onClose}
+        />
+        {groupId !== null && (
+          <NotebookIframe config={config} groupId={groupId} />
+        )}
+      </div>
+    </div>
+  );
+}

src/annotator/components/test/NotebookModal-test.js

+import { act } from 'preact/test-utils';
+import { mount } from 'enzyme';
+
+import NotebookModal from '../NotebookModal';
+import { EventBus } from '../../util/emitter';
+
+describe('NotebookModal', () => {
+  let components;
+  let eventBus;
+  let emitter;
+
+  const createComponent = config => {
+    const component = mount(
+      <NotebookModal
+        eventBus={eventBus}
+        config={{ notebookAppUrl: '/notebook', ...config }}
+      />
+    );
+    components.push(component);
+    return component;
+  };
+
+  beforeEach(() => {
+    components = [];
+    eventBus = new EventBus();
+    emitter = eventBus.createEmitter();
+  });
+
+  afterEach(() => {
+    components.forEach(component => component.unmount());
+  });
+
+  it('hides modal on first render', () => {
+    const wrapper = createComponent();
+    const outer = wrapper.find('.Notebook__outer');
+
+    assert.isTrue(outer.hasClass('is-hidden'));
+    assert.isFalse(wrapper.find('iframe').exists());
+  });
+
+  it('shows modal on "openNotebook" event', () => {
+    const wrapper = createComponent();
+    let outer = wrapper.find('.Notebook__outer');
+
+    assert.isTrue(outer.hasClass('is-hidden'));
+    assert.isFalse(wrapper.find('iframe').exists());
+
+    emitter.publish('openNotebook', 'myGroup');
+    wrapper.update();
+
+    outer = wrapper.find('.Notebook__outer');
+    assert.isFalse(outer.hasClass('is-hidden'));
+
+    const iframe = wrapper.find('iframe');
+    assert.equal(
+      iframe.prop('src'),
+      `/notebook#config=${encodeURIComponent('{"group":"myGroup"}')}`
+    );
+  });
+
+  it('creates a new iframe element if the group is changed', () => {
+    const wrapper = createComponent();
+
+    emitter.publish('openNotebook', '1');
+    wrapper.update();
+
+    const iframe1 = wrapper.find('iframe');
+    assert.equal(
+      iframe1.prop('src'),
+      `/notebook#config=${encodeURIComponent('{"group":"1"}')}`
+    );
+
+    emitter.publish('openNotebook', '1');
+    wrapper.update();
+
+    const iframe2 = wrapper.find('iframe');
+    assert.equal(
+      iframe2.prop('src'),
+      `/notebook#config=${encodeURIComponent('{"group":"1"}')}`
+    );
+    assert.equal(iframe1.getDOMNode(), iframe2.getDOMNode());
+
+    emitter.publish('openNotebook', '2');
+    wrapper.update();
+
+    const iframe3 = wrapper.find('iframe');
+    assert.equal(
+      iframe3.prop('src'),
+      `/notebook#config=${encodeURIComponent('{"group":"2"}')}`
+    );
+    assert.notEqual(iframe1.getDOMNode(), iframe3.getDOMNode());
+  });
+
+  it('makes the document unscrollable on openNotebook event', () => {
+    createComponent();
+    act(() => {
+      emitter.publish('openNotebook', 'myGroup');
+    });
+    assert.equal(document.body.style.overflow, 'hidden');
+  });
+
+  it('hides modal on closing', () => {
+    const wrapper = createComponent();
+
+    emitter.publish('openNotebook', 'myGroup');
+    wrapper.update();
+
+    let outer = wrapper.find('.Notebook__outer');
+    assert.isFalse(outer.hasClass('is-hidden'));
+
+    act(() => {
+      wrapper.find('Button').prop('onClick')();
+    });
+    wrapper.update();
+
+    outer = wrapper.find('.Notebook__outer');
+
+    assert.isTrue(outer.hasClass('is-hidden'));
+  });
+
+  it('resets document scrollability on closing the modal', () => {
+    const wrapper = createComponent();
+    act(() => {
+      emitter.publish('openNotebook', 'myGroup');
+    });
+    assert.equal(document.body.style.overflow, 'hidden');
+    act(() => {
+      wrapper.find('Button').prop('onClick')();
+    });
+    assert.notEqual(document.body.style.overflow, 'hidden');
+  });
+});

src/annotator/config/sidebar.js

 /**
  * Create the JSON-serializable subset of annotator configuration that should
  * be passed to the sidebar application.
+ *
+ * @param {Record<string, any>} config
  */
 export function createSidebarConfig(config) {
   const sidebarConfig = { ...config };

src/annotator/notebook.js

-import { createSidebarConfig } from './config/sidebar';
 import { createShadowRoot } from './util/shadow-root';
 import { render } from 'preact';
-
-// FIXME: use the button from the frontend shared package once this is stable.
-import Button from '../sidebar/components/Button';
-
-/**
- * Create the iframe that will load the notebook application.
- *
- * @return {HTMLIFrameElement}
- */
-function createNotebookFrame(config, groupId) {
-  const notebookConfig = createSidebarConfig(config);
-  // Explicity set the "focused" group
-  notebookConfig.group = groupId;
-  const configParam =
-    'config=' + encodeURIComponent(JSON.stringify(notebookConfig));
-  const notebookAppSrc = config.notebookAppUrl + '#' + configParam;
-
-  const notebookFrame = document.createElement('iframe');
-
-  // Enable media in annotations to be shown fullscreen
-  notebookFrame.setAttribute('allowfullscreen', '');
-
-  notebookFrame.src = notebookAppSrc;
-  notebookFrame.title = 'Hypothesis annotation notebook';
-  notebookFrame.className = 'notebook-inner';
-
-  return notebookFrame;
-}
+import NotebookModal from './components/NotebookModal';
 
 export default class Notebook {
   /**
@@ -38,110 +10,22 @@ export default class Notebook {
    * @param {Record<string, any>} config
    */
   constructor(element, eventBus, config = {}) {
-    this.element = element;
-    this._emitter = eventBus.createEmitter();
-    this.options = config;
-    this.frame = null;
-
-    /** @type {null|string} */
-    this._groupId = null;
-    /** @type {null|string} */
-    this._prevGroupId = null;
-
     /**
      * Un-styled shadow host for the notebook content.
-     *
      * This isolates the notebook from the page's styles.
      */
     this._outerContainer = document.createElement('hypothesis-notebook');
-    this.element.appendChild(this._outerContainer);
+    element.appendChild(this._outerContainer);
+    this.shadowRoot = createShadowRoot(this._outerContainer);
 
-    /**
-     * Lazily-initialized container for the notebook iframe. This is only created
-     * when the notebook is actually used.
-     *
-     * @type {HTMLElement|null}
-     */
-    this.container = null;
-
-    this._emitter.subscribe('openNotebook', groupId => {
-      this._groupId = groupId;
-      this.open();
-    });
-  }
-
-  _update() {
-    const container = this._initContainer();
-
-    // Create a new iFrame if we don't have one at all yet, or if the
-    // groupId has changed since last use
-    const needIframe =
-      !this.frame || !this._prevGroupId || this._prevGroupId !== this._groupId;
-    this._prevGroupId = this._groupId;
-
-    if (needIframe) {
-      this.frame?.remove();
-      this.frame = createNotebookFrame(this.options, this._groupId);
-      container.appendChild(this.frame);
-    }
-  }
-
-  open() {
-    const container = this._initContainer();
-    this._update();
-    container.classList.add('is-open');
-    container.style.display = '';
-
-    // The overflow CSS property is set to hidden to prevent scrolling of the guest page,
-    // while the notebook is shown as modal. It is restored on the close method.
-    // I believe this hack only works if this.element points to document.body of the guest page.
-    this.originalOverflowStyle = this.element.style.overflow;
-    this.element.style.overflow = 'hidden';
-  }
-
-  close() {
-    if (this.container) {
-      this.container.classList.remove('is-open');
-      this.container.style.display = 'none';
-    }
-    this.element.style.overflow = /** @type {string} */ (this
-      .originalOverflowStyle);
+    render(
+      <NotebookModal eventBus={eventBus} config={config} />,
+      this.shadowRoot
+    );
   }
 
   destroy() {
+    render(null, this.shadowRoot);
     this._outerContainer.remove();
-    this._emitter.destroy();
-  }
-
-  _initContainer() {
-    if (this.container) {
-      return this.container;
-    }
-
-    const shadowRoot = createShadowRoot(this._outerContainer);
-    this.container = document.createElement('div');
-    this.container.style.display = 'none';
-    this.container.className = 'notebook-outer';
-    shadowRoot.appendChild(this.container);
-
-    const onClose = () => {
-      this.close();
-      this._emitter.publish('closeNotebook');
-    };
-
-    render(
-      <div className="Notebook__controller-bar">
-        <Button
-          icon="cancel"
-          className="Notebook__close-button"
-          buttonText="Close"
-          title="Close the Notebook"
-          onClick={onClose}
-        />
-      </div>,
-      this.container
-    );
-
-    return this.container;
   }
 }

src/annotator/test/notebook-test.js

-import Notebook from '../notebook';
+import { useEffect } from 'preact/hooks';
+import { act } from 'preact/test-utils';
+
+import Notebook, { $imports } from '../notebook';
 import { EventBus } from '../util/emitter';
 
 describe('Notebook', () => {
   // `Notebook` instances created by current test
   let notebooks;
+  let container;
+  let cleanUpCallback;
 
   const createNotebook = (config = {}) => {
-    config = { notebookAppUrl: '/base/annotator/test/empty.html', ...config };
-    const element = document.createElement('div');
     const eventBus = new EventBus();
-    const notebook = new Notebook(element, eventBus, config);
+    const notebook = new Notebook(container, eventBus, config);
 
     notebooks.push(notebook);
 
@@ -18,140 +21,54 @@ describe('Notebook', () => {
 
   beforeEach(() => {
     notebooks = [];
-  });
-
-  afterEach(() => {
-    notebooks.forEach(n => n.destroy());
-  });
-
-  describe('notebook container frame', () => {
-    it('is not created until the notebook is shown', () => {
-      const notebook = createNotebook();
-      assert.isNull(notebook.container);
-
-      notebook.open();
-      assert.isNotNull(notebook.container);
-    });
-
-    it('is not created if `hide` is called before notebook is first shown', () => {
-      const notebook = createNotebook();
-      notebook.close();
-      assert.isNull(notebook.container);
-    });
-
-    it('displays when opened', () => {
-      const notebook = createNotebook();
-
-      notebook.open();
-
-      assert.equal(notebook.container.style.display, '');
-      assert.isTrue(notebook.container.classList.contains('is-open'));
-    });
-
-    it('hides when closed', () => {
-      const notebook = createNotebook();
-
-      notebook.open();
-      notebook.close();
-
-      assert.equal(notebook.container.style.display, 'none');
-      assert.isFalse(notebook.container.classList.contains('is-open'));
-    });
-  });
-
-  describe('creating the notebook iframe', () => {
-    it('creates the iframe when the notebook is opened for the first time', () => {
-      const notebook = createNotebook();
-
-      assert.isNull(notebook.frame);
-
-      notebook.open();
+    container = document.createElement('div');
+    cleanUpCallback = sinon.stub();
 
-      assert.isTrue(notebook.frame instanceof Element);
-    });
+    const FakeNotebookModal = () => {
+      useEffect(() => {
+        return () => {
+          cleanUpCallback();
+        };
+      }, []);
+      return <div id="notebook-modal" />;
+    };
 
-    it('sets the iframe source to the configured `notebookAppUrl`', () => {
-      const notebook = createNotebook({
-        notebookAppUrl: 'http://www.example.com/foo/bar',
+    $imports.$mock({
+      './components/NotebookModal': { default: FakeNotebookModal },
     });
-
-      notebook.open();
-
-      // The rest of the config gets added as a hash to the end of the src,
-      // so split that off and look at the string before it
-      assert.equal(
-        notebook.frame.src.split('#')[0],
-        'http://www.example.com/foo/bar'
-      );
   });
 
-    it('does not create a new iframe if opened again with same group ID', () => {
-      const notebook = createNotebook();
-      notebook._groupId = 'mygroup';
-
-      // The first opening will create a new iFrame
-      notebook._emitter.publish('openNotebook', 'myGroup');
-      const removeSpy = sinon.spy(notebook.frame, 'remove');
-      // Open it again — the group hasn't changed so the iframe won't be
-      // replaced
-      notebook._emitter.publish('openNotebook', 'myGroup');
-
-      assert.notCalled(removeSpy);
+  afterEach(() => {
+    notebooks.forEach(n => n.destroy());
+    $imports.$restore();
   });
 
-    it('does not create a new iframe if shown again with same group ID', () => {
+  describe('notebook container', () => {
+    it('creates the container', () => {
+      assert.isFalse(container.hasChildNodes());
       const notebook = createNotebook();
-      notebook._groupId = 'mygroup';
-
-      // First open: creates an iframe
-      notebook._emitter.publish('openNotebook', 'myGroup');
-      const removeSpy = sinon.spy(notebook.frame, 'remove');
-
-      // Open again with another group
-      notebook._emitter.publish('openNotebook', 'anotherGroup');
-
-      // Open again, which will remove the first iframe and create a new one
-      notebook.open();
-      assert.calledOnce(removeSpy);
-    });
+      const shadowRoot = notebook._outerContainer.shadowRoot;
+      assert.isNotNull(shadowRoot);
+      assert.isNotNull(shadowRoot.querySelector('#notebook-modal'));
     });
 
-  describe('responding to user input', () => {
-    it('closes the notebook when close button clicked', () => {
+    it('removes the container', () => {
       const notebook = createNotebook();
-
-      notebook.open();
-
-      const button = notebook.container.getElementsByClassName(
-        'Notebook__close-button'
-      )[0];
-      button.click();
-      assert.equal(notebook.container.style.display, 'none');
-    });
+      notebook.destroy();
+      assert.isFalse(container.hasChildNodes());
     });
 
-  describe('responding to events', () => {
-    it('opens on `openNotebook`', () => {
-      const notebook = createNotebook();
-
-      notebook._emitter.publish('openNotebook');
-
-      assert.equal(notebook.container.style.display, '');
+    it('calls the clean up function of the NotebookModal when the container is removed', () => {
+      // Necessary to run the useEffect for first time and register the cleanup function
+      let notebook;
+      act(() => {
+        notebook = createNotebook();
       });
-  });
-
-  describe('destruction', () => {
-    it('should remove the frame', () => {
-      const notebook = createNotebook();
-      const hostDocument = notebook.element;
-
-      // Make sure the frame is created
-      notebook.open();
-      assert.isNotNull(hostDocument.querySelector('hypothesis-notebook'));
-
+      // Necessary to run the cleanup function of the useEffect
+      act(() => {
         notebook.destroy();
-
-      assert.isNull(hostDocument.querySelector('hypothesis-notebook'));
+      });
+      assert.called(cleanUpCallback);
     });
   });
 });

src/styles/annotator/notebook.scss

@@ -2,7 +2,7 @@
 @use '../mixins/molecules';
 @use "../mixins/buttons";
 
-.notebook-outer {
+.Notebook__outer {
   box-sizing: border-box;
   position: fixed;
   // This large zIndex is used to bring the notebook to the front, so it is not
@@ -11,46 +11,40 @@
   z-index: 2147483647;
   top: 0;
   left: 0;
-  width: 100vw;
-  height: 100vh;
+  right: 0;
+  bottom: 0;
   padding: var.$layout-space;
 
-  &.is-open {
   // TBD: Actual opacity/overlay we'd like to use
   background-color: rgba(0, 0, 0, 0.5);
+
+  &.is-hidden {
+    display: none;
   }
 }
 
-.notebook-inner {
+.Notebook__inner {
+  position: relative;
   box-sizing: border-box;
   @include molecules.panel;
   padding: 0;
   width: 100%;
   height: 100%;
-
-  // FIXME: these properties produce a visual break between the Notebook__controller-bar and the iframe.
-  // A better approach would be if the Notebook__controller-bar and iframe could be children of notebook-inner.
-  border: none;
-  border-top-left-radius: 0;
-  border-top-right-radius: 0;
+  border: 0;
 }
 
-// This container element has the purpose of pushing children to the right side.
-.Notebook__controller-bar {
-  font-size: var.$font-size--large;
-  display: flex;
-  justify-content: flex-end;
-
-  // FIXME: these properties emulates as if the Notebook__controller-bar would be part of the iframe.
-  border-top-left-radius: var.$border-radius;
-  border-top-right-radius: var.$border-radius;
+.Notebook__iframe {
+  width: 100%;
+  height: 100%;
+  border: none;
+  margin-top: 0 !important; // disables the margin-top set by vertical-rhythm mixin
 }
 
 .Notebook__close-button {
   position: absolute;
-  @include buttons.button--labeled(
-    $background-color: var.$grey-2,
-    $active-background-color: var.$grey-3
-  );
+  right: 0;
+  font-size: var.$font-size--large;
+  @include buttons.button--primary;
   margin: var.$layout-space--xsmall;
+  cursor: pointer;
 }