From 5f3dc9c92b69ad7fe18b99429e6d8a17edc58baf Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Wed, 7 Feb 2024 15:45:05 +0100
Subject: [PATCH] Display notebook in a native dialog when possible

---
 package.json                                  |   2 +-
 src/annotator/components/NotebookModal.tsx    | 119 +++++++++++++-----
 .../components/test/NotebookModal-test.js     |  76 +++++++++--
 yarn.lock                                     |   4 +-
 4 files changed, 157 insertions(+), 44 deletions(-)

diff --git a/package.json b/package.json
index a9807a8a9bf..bc69b8019e1 100644
--- a/package.json
+++ b/package.json
@@ -13,7 +13,7 @@
     "@babel/preset-react": "^7.0.0",
     "@babel/preset-typescript": "^7.16.7",
     "@hypothesis/frontend-build": "^3.0.0",
-    "@hypothesis/frontend-shared": "^7.2.0",
+    "@hypothesis/frontend-shared": "^7.3.0",
     "@hypothesis/frontend-testing": "^1.2.0",
     "@npmcli/arborist": "^7.0.0",
     "@octokit/rest": "^20.0.1",
diff --git a/src/annotator/components/NotebookModal.tsx b/src/annotator/components/NotebookModal.tsx
index d8997acadd9..3d7f50ffa43 100644
--- a/src/annotator/components/NotebookModal.tsx
+++ b/src/annotator/components/NotebookModal.tsx
@@ -1,6 +1,11 @@
-import { IconButton, CancelIcon } from '@hypothesis/frontend-shared';
+import {
+  IconButton,
+  CancelIcon,
+  ModalDialog,
+} from '@hypothesis/frontend-shared';
 import classnames from 'classnames';
-import { useEffect, useRef, useState } from 'preact/hooks';
+import type { ComponentChildren } from 'preact';
+import { useEffect, useMemo, useRef, useState } from 'preact/hooks';
 
 import { addConfigFragment } from '../../shared/config-fragment';
 import { createAppConfig } from '../config/app';
@@ -27,7 +32,7 @@ function NotebookIframe({ config, groupId }: NotebookIframeProps) {
   const notebookAppSrc = addConfigFragment(config.notebookAppUrl, {
     ...createAppConfig(config.notebookAppUrl, config),
 
-    // Explicity set the "focused" group
+    // Explicitly set the "focused" group
     group: groupId,
   });
 
@@ -40,9 +45,62 @@ function NotebookIframe({ config, groupId }: NotebookIframeProps) {
     />
   );
 }
+
+/**
+ * Checks if the browser supports native modal dialogs
+ */
+function isModalDialogSupported(document: Document) {
+  const dialog = document.createElement('dialog');
+  return typeof dialog.showModal === 'function';
+}
+
 export type NotebookModalProps = {
   eventBus: EventBus;
   config: NotebookConfig;
+
+  /** Test seam */
+  document_?: Document;
+};
+
+type WrapperProps = { isHidden: boolean; children: ComponentChildren };
+
+const DialogWrapper = ({ isHidden, children }: WrapperProps) => {
+  const dialogRef = useRef<HTMLDialogElement | null>(null);
+
+  useEffect(() => {
+    if (isHidden) {
+      dialogRef.current?.close();
+    } else {
+      dialogRef.current?.showModal();
+    }
+  }, [isHidden]);
+
+  return (
+    <dialog
+      ref={dialogRef}
+      className="relative w-full h-full backdrop:bg-black/50"
+      data-testid="notebook-outer"
+    >
+      {children}
+    </dialog>
+  );
+};
+
+const LegacyWrapper = ({ isHidden, children }: WrapperProps) => {
+  return (
+    <div
+      className={classnames({ hidden: isHidden })}
+      data-testid="notebook-outer"
+    >
+      <ModalDialog
+        variant="custom"
+        size="custom"
+        classes="p-3 relative w-full h-full"
+      >
+        {children}
+      </ModalDialog>
+    </div>
+  );
 };
 
 /**
@@ -51,6 +109,8 @@ export type NotebookModalProps = {
 export default function NotebookModal({
   eventBus,
   config,
+  /* istanbul ignore next - test seam */
+  document_ = document,
 }: NotebookModalProps) {
   // Temporary solution: while there is no mechanism to sync new annotations in
   // the notebook, we force re-rendering of the iframe on every 'openNotebook'
@@ -62,6 +122,15 @@ export default function NotebookModal({
   const originalDocumentOverflowStyle = useRef('');
   const emitterRef = useRef<Emitter | null>(null);
 
+  const isDialogSupported = useMemo(
+    () => isModalDialogSupported(document_),
+    [document_],
+  );
+  const Wrapper = useMemo(
+    () => (isDialogSupported ? DialogWrapper : LegacyWrapper),
+    [isDialogSupported],
+  );
+
   // Stores the original overflow CSS property of document.body and reset it
   // when the component is destroyed
   useEffect(() => {
@@ -106,32 +175,24 @@ export default function NotebookModal({
   }
 
   return (
-    <div
-      className={classnames(
-        'fixed z-max top-0 left-0 right-0 bottom-0 p-3 bg-black/50',
-        { hidden: isHidden },
-      )}
-      data-testid="notebook-outer"
-    >
-      <div className="relative w-full h-full" data-testid="notebook-inner">
-        <div className="absolute right-0 m-3">
-          <IconButton
-            title="Close the Notebook"
-            onClick={onClose}
-            variant="dark"
-            classes={classnames(
-              // Remove the dark variant's background color to avoid
-              // interfering with modal overlays. Re-activate the dark variant's
-              // background color on hover.
-              // See https://github.com/hypothesis/client/issues/3676
-              '!bg-transparent enabled:hover:!bg-grey-3',
-            )}
-          >
-            <CancelIcon className="w-4 h-4" />
-          </IconButton>
-        </div>
-        <NotebookIframe key={iframeKey} config={config} groupId={groupId} />
+    <Wrapper isHidden={isHidden}>
+      <div className="absolute right-0 m-3">
+        <IconButton
+          title="Close the Notebook"
+          onClick={onClose}
+          variant="dark"
+          classes={classnames(
+            // Remove the dark variant's background color to avoid
+            // interfering with modal overlays. Re-activate the dark variant's
+            // background color on hover.
+            // See https://github.com/hypothesis/client/issues/3676
+            '!bg-transparent enabled:hover:!bg-grey-3',
+          )}
+        >
+          <CancelIcon className="w-4 h-4" />
+        </IconButton>
       </div>
-    </div>
+      <NotebookIframe key={iframeKey} config={config} groupId={groupId} />
+    </Wrapper>
   );
 }
diff --git a/src/annotator/components/test/NotebookModal-test.js b/src/annotator/components/test/NotebookModal-test.js
index 12c7a714099..8fc55ff5493 100644
--- a/src/annotator/components/test/NotebookModal-test.js
+++ b/src/annotator/components/test/NotebookModal-test.js
@@ -9,17 +9,24 @@ describe('NotebookModal', () => {
   const notebookURL = 'https://test.hypothes.is/notebook';
 
   let components;
+  let containers;
   let eventBus;
   let emitter;
 
   const outerSelector = '[data-testid="notebook-outer"]';
 
-  const createComponent = config => {
+  const createComponent = (config, fakeDocument) => {
+    const attachTo = document.createElement('div');
+    document.body.appendChild(attachTo);
+    containers.push(attachTo);
+
     const component = mount(
       <NotebookModal
         eventBus={eventBus}
         config={{ notebookAppUrl: notebookURL, ...config }}
+        document_={fakeDocument}
       />,
+      { attachTo },
     );
     components.push(component);
     return component;
@@ -27,6 +34,7 @@ describe('NotebookModal', () => {
 
   beforeEach(() => {
     components = [];
+    containers = [];
     eventBus = new EventBus();
     emitter = eventBus.createEmitter();
 
@@ -43,6 +51,7 @@ describe('NotebookModal', () => {
 
   afterEach(() => {
     components.forEach(component => component.unmount());
+    containers.forEach(container => container.remove());
     $imports.$restore();
   });
 
@@ -114,23 +123,66 @@ describe('NotebookModal', () => {
     assert.equal(document.body.style.overflow, 'hidden');
   });
 
-  it('hides modal on closing', () => {
-    const wrapper = createComponent();
+  context('when modal dialog is not supported', () => {
+    let fakeDocument;
 
-    emitter.publish('openNotebook', 'myGroup');
-    wrapper.update();
+    beforeEach(() => {
+      fakeDocument = {
+        createElement: sinon.stub().returns({}),
+      };
+    });
 
-    let outer = wrapper.find(outerSelector);
-    assert.isFalse(outer.hasClass('hidden'));
+    it('does not render a dialog element', () => {
+      const wrapper = createComponent({}, fakeDocument);
 
-    act(() => {
-      wrapper.find('IconButton').prop('onClick')();
+      emitter.publish('openNotebook', 'myGroup');
+      wrapper.update();
+
+      assert.isFalse(wrapper.exists('dialog'));
     });
-    wrapper.update();
 
-    outer = wrapper.find(outerSelector);
+    it('hides modal on closing', () => {
+      const wrapper = createComponent({}, fakeDocument);
+
+      emitter.publish('openNotebook', 'myGroup');
+      wrapper.update();
 
-    assert.isTrue(outer.hasClass('hidden'));
+      let outer = wrapper.find(outerSelector);
+      assert.isFalse(outer.hasClass('hidden'));
+
+      act(() => {
+        wrapper.find('IconButton').prop('onClick')();
+      });
+      wrapper.update();
+
+      outer = wrapper.find(outerSelector);
+
+      assert.isTrue(outer.hasClass('hidden'));
+    });
+  });
+
+  context('when modal dialog is supported', () => {
+    it('renders a dialog element', () => {
+      const wrapper = createComponent({});
+
+      emitter.publish('openNotebook', 'myGroup');
+      wrapper.update();
+
+      assert.isTrue(wrapper.exists('dialog'));
+    });
+
+    it('shows and closes native dialog', () => {
+      const wrapper = createComponent({});
+      const isDialogOpen = () => wrapper.find('dialog').getDOMNode().open;
+
+      act(() => emitter.publish('openNotebook', 'myGroup'));
+      wrapper.update();
+      assert.isTrue(isDialogOpen());
+
+      act(() => wrapper.find('IconButton').prop('onClick')());
+      wrapper.update();
+      assert.isFalse(isDialogOpen());
+    });
   });
 
   it('resets document scrollability on closing the modal', () => {
diff --git a/yarn.lock b/yarn.lock
index 56ccb50884d..606ba6b798c 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -2695,7 +2695,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@hypothesis/frontend-shared@npm:^7.2.0":
+"@hypothesis/frontend-shared@npm:^7.3.0":
   version: 7.3.0
   resolution: "@hypothesis/frontend-shared@npm:7.3.0"
   dependencies:
@@ -9040,7 +9040,7 @@ __metadata:
     "@babel/preset-react": ^7.0.0
     "@babel/preset-typescript": ^7.16.7
     "@hypothesis/frontend-build": ^3.0.0
-    "@hypothesis/frontend-shared": ^7.2.0
+    "@hypothesis/frontend-shared": ^7.3.0
     "@hypothesis/frontend-testing": ^1.2.0
     "@npmcli/arborist": ^7.0.0
     "@octokit/rest": ^20.0.1